[OVN] Mech driver fails to delete DHCP options during subnet deletion

Is there any impact other than stale data in the DB? I would assume if subnet is deleted and new created, it's gonna have a different ID and will not cause any issues.

Hello Jakub:

If the driver call raises an exception, it will never end correctly, including

(sorry, I submitted an unfinished comment and I can't modify it)

Hello Jakub:

If the driver call raises an exception, it will never end correctly, including the version bump. This happens when the subnet has no DHCP options. In any case, this must be considered and properly handled.

Regards.

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/815280

Reviewed: https://review.opendev.org/c/openstack/neutron/+/815280
Committed: https://opendev.org/openstack/neutron/commit/95c2801da855cada741693b0dc31192e7baa111c
Submitter: "Zuul (22348)"
Branch: master

commit 95c2801da855cada741693b0dc31192e7baa111c
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Mon Oct 25 11:12:14 2021 +0000

    Check subnet in "_remove_subnet_dhcp_options"

    Since [1], "get_subnet_dhcp_options" returns always a dictionary
    in "subnet" instead of None. This patch checks not only that "subnet"
    is None but also the dictionary is not empty.

    [1]https://review.opendev.org/c/openstack/neutron/+/807692

    Closes-Bug: #1948466

    Change-Id: Ie93cf3e47e09b3e5051be1ffad512251775b0492

Fix proposed to branch: stable/xena
Review: https://review.opendev.org/c/openstack/neutron/+/822798

Fix proposed to branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/neutron/+/822799

Fix proposed to branch: stable/victoria
Review: https://review.opendev.org/c/openstack/neutron/+/822800

Fix proposed to branch: stable/ussuri
Review: https://review.opendev.org/c/openstack/neutron/+/822801

Reviewed: https://review.opendev.org/c/openstack/neutron/+/822800
Committed: https://opendev.org/openstack/neutron/commit/79cdace512307c9b0c92f56af9abc656371a6f31
Submitter: "Zuul (22348)"
Branch: stable/victoria

commit 79cdace512307c9b0c92f56af9abc656371a6f31
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Mon Oct 25 11:12:14 2021 +0000

    Check subnet in "_remove_subnet_dhcp_options"

    Since [1], "get_subnet_dhcp_options" returns always a dictionary
    in "subnet" instead of None. This patch checks not only that "subnet"
    is None but also the dictionary is not empty.

    [1]https://review.opendev.org/c/openstack/neutron/+/807692

    Closes-Bug: #1948466

    Change-Id: Ie93cf3e47e09b3e5051be1ffad512251775b0492
    (cherry picked from commit 95c2801da855cada741693b0dc31192e7baa111c)

Reviewed: https://review.opendev.org/c/openstack/neutron/+/822801
Committed: https://opendev.org/openstack/neutron/commit/9151f4a3966e7416b8449945e9b4735dd92b2a60
Submitter: "Zuul (22348)"
Branch: stable/ussuri

commit 9151f4a3966e7416b8449945e9b4735dd92b2a60
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Mon Oct 25 11:12:14 2021 +0000

    Check subnet in "_remove_subnet_dhcp_options"

    Since [1], "get_subnet_dhcp_options" returns always a dictionary
    in "subnet" instead of None. This patch checks not only that "subnet"
    is None but also the dictionary is not empty.

    [1]https://review.opendev.org/c/openstack/neutron/+/807692

    Closes-Bug: #1948466

    Change-Id: Ie93cf3e47e09b3e5051be1ffad512251775b0492
    (cherry picked from commit 95c2801da855cada741693b0dc31192e7baa111c)

Reviewed: https://review.opendev.org/c/openstack/neutron/+/822798
Committed: https://opendev.org/openstack/neutron/commit/3e9dcaee50ec69fb6e71a59de4ad936e297f5a4f
Submitter: "Zuul (22348)"
Branch: stable/xena

commit 3e9dcaee50ec69fb6e71a59de4ad936e297f5a4f
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Mon Oct 25 11:12:14 2021 +0000

    Check subnet in "_remove_subnet_dhcp_options"

    Since [1], "get_subnet_dhcp_options" returns always a dictionary
    in "subnet" instead of None. This patch checks not only that "subnet"
    is None but also the dictionary is not empty.

    [1]https://review.opendev.org/c/openstack/neutron/+/807692

    Closes-Bug: #1948466

    Change-Id: Ie93cf3e47e09b3e5051be1ffad512251775b0492
    (cherry picked from commit 95c2801da855cada741693b0dc31192e7baa111c)

Reviewed: https://review.opendev.org/c/openstack/neutron/+/822799
Committed: https://opendev.org/openstack/neutron/commit/1aba78896b6daa2a786a4445c9ea6eb46d8aa145
Submitter: "Zuul (22348)"
Branch: stable/wallaby

commit 1aba78896b6daa2a786a4445c9ea6eb46d8aa145
Author: Rodolfo Alonso Hernandez <email address hidden>
Date: Mon Oct 25 11:12:14 2021 +0000

    Check subnet in "_remove_subnet_dhcp_options"

    Since [1], "get_subnet_dhcp_options" returns always a dictionary
    in "subnet" instead of None. This patch checks not only that "subnet"
    is None but also the dictionary is not empty.

    [1]https://review.opendev.org/c/openstack/neutron/+/807692

    Closes-Bug: #1948466

    Change-Id: Ie93cf3e47e09b3e5051be1ffad512251775b0492
    (cherry picked from commit 95c2801da855cada741693b0dc31192e7baa111c)

This issue was fixed in the openstack/neutron 19.1.0 release.

This issue was fixed in the openstack/neutron 17.3.0 release.

This issue was fixed in the openstack/neutron 18.2.0 release.

This issue was fixed in the openstack/neutron 20.0.0.0rc1 release candidate.

This issue was fixed in the openstack/networking-ovn train-eol release.

A new package version with this fix has been uploaded to the focal unapproved queue for the SRU team to review.
https://launchpad.net/ubuntu/focal/+queue?queue_state=1&queue_text=neutron

Hello Rodolfo, or anyone else affected,

Accepted neutron into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/neutron/2:16.4.2-0ubuntu6.3 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Rodolfo, or anyone else affected,

Accepted neutron into ussuri-proposed. The package will build now and be available in the Ubuntu Cloud Archive in a few hours, and then in the -proposed repository.

Please help us by testing this new package. To enable the -proposed repository:

  sudo add-apt-repository cloud-archive:ussuri-proposed
  sudo apt-get update

Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-ussuri-needed to verification-ussuri-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-ussuri-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

I've tested the proposed package on focal. Testing details are uploaded. Without the fix I do see the issue mentioned in this LP, with an empty directory hardcoded, like so,

    def _remove_subnet_dhcp_options(self, subnet_id, txn):
        dhcp_options = self._nb_idl.get_subnet_dhcp_options(
            subnet_id, with_ports=True)

        dhcp_options['subnet'] = {} <--
        print(dhcp_options['subnet'])
        if dhcp_options['subnet'] is not None:
            txn.add(self._nb_idl.delete_dhcp_options(
                dhcp_options['subnet']['uuid']))

root@juju-3b1260-fixtestnew-6:/usr/lib/python3/dist-packages# python3 -m unittest neutron.tests.unit.plugins.ml2.drivers.ovn.mech_driver.test_mech_driver.TestOVNMechanismDriver.test_remove_subnet_dhcp_options_in_ovn_ipv4
/usr/lib/python3/dist-packages/neutron_lib/worker.py:68: DeprecationWarning: PY_SSIZE_T_CLEAN will be required for '#' formats
  self._parent_proctitle = setproctitle.getproctitle()
{}
E
======================================================================
ERROR: test_remove_subnet_dhcp_options_in_ovn_ipv4 (neutron.tests.unit.plugins.ml2.drivers.ovn.mech_driver.test_mech_driver.TestOVNMechanismDriver)
neutron.tests.unit.plugins.ml2.drivers.ovn.mech_driver.test_mech_driver.TestOVNMechanismDriver.test_remove_subnet_dhcp_options_in_ovn_ipv4
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/neutron/tests/base.py", line 182, in func
    return f(self, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py", line 1368, in test_remove_subnet_dhcp_options_in_ovn_ipv4
    self._test_remove_subnet_dhcp_options_in_ovn(4)
  File "/usr/lib/python3/dist-packages/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py", line 1356, in _test_remove_subnet_dhcp_options_in_ovn
    self.mech_driver._ovn_client._remove_subnet_dhcp_options(
  File "/usr/lib/python3/dist-packages/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py", line 1893, in _remove_subnet_dhcp_options
    dhcp_options['subnet']['uuid']))
KeyError: 'uuid'

----------------------------------------------------------------------
Ran 1 test in 3.235s

With the fix, the KeyError is resolved, the unit test fails though, and I am assuming this failure is expected due to the hardcoded null value while the test itself does,

        # Check deleting DHCP_Options rows
        delete_dhcp_calls = [mock.call('subnet-uuid'), mock.call('port1-uuid')] <-- passing subnet-uuid
        self.assertEqual(
            len(delete_dhcp_calls),
            self.mech_driver._nb_ovn.delete_dhcp_options.call_count)
        self.mech_driver._nb_ovn.delete_dhcp_options.assert_has_calls(
            delete_dhcp_calls, any_order=True)

the code looks like,

    def _remove_subnet_dhcp_options(self, subnet_id, txn):
        dhcp_options = self._nb_idl.get_subnet_dhcp_options(
            subnet_id, with_ports=True)

        dhcp_options['subnet'] = {} <-- hard coded to empty
        print(dhcp_options['subnet...

Hello Nikhil:

Please review your environment and package versions. This unit test is passing in the CI every day several times. Use tox to create a virtual environment that will install the required python package versions.

Regards.

Hi Rodolfo,

** deleted testing request since a customer as well as Rafael have verified this fix **

regards,
Nikhil.

Hi Rodolfo,

I think the issue is I'm testing the fix using the hard coded empty directory like this,

        dhcp_options['subnet'] = {}
        print(dhcp_options['subnet'])
        if dhcp_options['subnet']:

Is there in fact a unit test that can test the empty directory instead of me hardcoding it into the unit test that is designed to pass the non empty directory?

Also, I am not sure which package versions you refer to. I've updated to the proposed packages on neutron-api node, here are the versions I'm testing,

# dpkg -l | grep -i neutron
ii neutron-common 2:16.4.2-0ubuntu6.3 all Neutron is a virtual network service for Openstack - common
ii neutron-fwaas-common 1:16.0.0-0ubuntu0.20.04.1 all Firewall-as-a-Service driver for OpenStack Neutron
ii neutron-plugin-ml2 2:16.4.2-0ubuntu6.3 all Neutron is a virtual network service for Openstack - ML2 plugin
ii neutron-server 2:16.4.2-0ubuntu6.3 all Neutron is a virtual network service for Openstack - server
ii python3-neutron 2:16.4.2-0ubuntu6.3 all Neutron is a virtual network service for Openstack - Python library
ii python3-neutron-dynamic-routing 2:16.0.0-0ubuntu0.20.04.1 all OpenStack Neutron Dynamic Routing - Python 3 library
ii python3-neutron-fwaas 1:16.0.0-0ubuntu0.20.04.1 all Firewall-as-a-Service driver for OpenStack Neutron
ii python3-neutron-lib 2.3.0-0ubuntu1 all Neutron shared routines and utilities - Python 3.x
ii python3-neutronclient 1:7.1.1-0ubuntu1 all client API library for Neutron - Python 3.x

Do you mean the python3 version?

# python3 --version
Python 3.8.10

Hello Nikhil:

Ok, now I get it, you changed the UT. If you think that is valuable to test when "dhcp_options['subnet']={}", please feel free to send a patch adding this new test.

Regards.

As per discussion today with Brian Haley, a customer has tested the 2:16.4.2-0ubuntu6.3 code, and validated that it does fix the issue, and the keyerror is no longer seen. (SF case - 00365728 )

So I am marking the verification flags as done, this one should be good to be pushed to updates.

Hey Nikhil,

FWIW, I was able to get the test running using tox, and added your print plus some code to the unit test to set empty subnet dict and verify the fix (see attachment for test result).

ubuntu@sought-jackal:~/tmp/neutron$ git diff
diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
index 96c4457d19..ccf46481b1 100644
--- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
+++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
@@ -1909,6 +1909,7 @@ class OVNClient(object):
         dhcp_options = self._nb_idl.get_subnet_dhcp_options(
             subnet_id, with_ports=True)

+ print(dhcp_options['subnet'])
         if dhcp_options['subnet']:
             txn.add(self._nb_idl.delete_dhcp_options(
                 dhcp_options['subnet']['uuid']))
diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py
index bfd9100942..bd21a10488 100644
--- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py
+++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py
@@ -1362,6 +1362,23 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
         self.mech_driver._nb_ovn.delete_dhcp_options.assert_has_calls(
             delete_dhcp_calls, any_order=True)

+ # reset call count
+ self.mech_driver._nb_ovn.delete_dhcp_options.call_count = 0
+ # test empty subnet
+ opts = {'subnet': {},
+ 'ports': [{'uuid': 'port1-uuid'}]}
+ self.mech_driver._nb_ovn.get_subnet_dhcp_options.return_value = opts
+ self.mech_driver._ovn_client._remove_subnet_dhcp_options(
+ 'subnet-id', mock.Mock())
+
+ # Check deleting DHCP_Options rows
+ # we expect call count of 1 since subnet is empty but ports isn't
+ delete_dhcp_calls = [mock.call('subnet-uuid'), mock.call('port1-uuid')]
+ self.assertEqual(1,
+ self.mech_driver._nb_ovn.delete_dhcp_options.call_count)
+ self.mech_driver._nb_ovn.delete_dhcp_options.assert_has_calls(
+ delete_dhcp_calls, any_order=True)
+
     def test_remove_subnet_dhcp_options_in_ovn_ipv4(self):
         self._test_remove_subnet_dhcp_options_in_ovn(4)

Thanks Raf for testing using the unit test. Do you want to submit the unit test upstream?

I've already marked this bug's verifications as done.

-Nikhil.

This bug was fixed in the package neutron - 2:16.4.2-0ubuntu6.3

---------------
neutron (2:16.4.2-0ubuntu6.3) focal; urgency=medium

  * d/p/check-subnet-in-remove-subnet-dhcp-options.patch: Ensure dhcp_options
    subnet check handles dictionary correctly (LP: #1948466).
  * d/p/ovn-fix-untrusted-port-security-enabled-check.patch: Fix logic for
    check that wraps adding of port to drop port group (LP: #1939723).

 -- Corey Bryant <email address hidden> Mon, 21 Aug 2023 15:29:46 -0400

The verification of the Stable Release Update for neutron has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

The verification of the Stable Release Update for neutron has completed successfully and the package has now been released to -updates. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package neutron - 2:16.4.2-0ubuntu6.3~cloud0
---------------

 neutron (2:16.4.2-0ubuntu6.3~cloud0) bionic-ussuri; urgency=medium
 .
   * New update for the Ubuntu Cloud Archive.
 .
 neutron (2:16.4.2-0ubuntu6.3) focal; urgency=medium
 .
   * d/p/check-subnet-in-remove-subnet-dhcp-options.patch: Ensure dhcp_options
     subnet check handles dictionary correctly (LP: #1948466).
   * d/p/ovn-fix-untrusted-port-security-enabled-check.patch: Fix logic for
     check that wraps adding of port to drop port group (LP: #1939723).