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

[Rodolfo Alonso]

== Original Bug Description ==

Snippet: https://paste.opendev.org/show/810168/

I can't provide a link to a CI execution, I saw this error in an internal CI. I'm still investigating when this could happen.

== Ubuntu SRU Details ==

[Impact]

During subnet deletion the check in _remove_subnet_dhcp_options() results in the following traceback (taken from pastebin above in case it disappears) if dhcp_options['subnet'] is an empty dictionary:

ExternalNetworksRBACTestJSON-2078932943-project-admin] Mechanism driver 'ovn' failed in delete_subnet_postcommit: KeyError: 'uuid'
 Traceback (most recent call last):
   File "/opt/stack/neutron/neutron/plugins/ml2/managers.py", line 482, in _call_on_drivers
     getattr(driver.obj, method_name)(context)
   File "/opt/stack/neutron/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py", line 637, in delete_subnet_postcommit
     self._ovn_client.delete_subnet(context._plugin_context,
   File "/opt/stack/neutron/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py", line 2103, in delete_subnet
     self._remove_subnet_dhcp_options(subnet_id, txn)
   File "/opt/stack/neutron/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py", line 1971, in _remove_subnet_dhcp_options
     dhcp_options['subnet']['uuid']))
 KeyError: 'uuid'

The fix ensures this check handles a dictionary correctly.

[Test Case]

In case we don't have a recreate for this:

1) lxc launch ubuntu-daily:focal f1 && lxc exec f1 /bin/bash
2) sudo add-apt-repository -p proposed
3) sudo apt install python3-neutron
4) cd /usr/lib/python3/dist-packages
5) python3 -m unittest neutron.tests.unit.plugins.ml2.drivers.ovn.mech_driver.test_mech_driver.TestOVNMechanismDriver.test_remove_subnet_dhcp_options_in_ovn_ipv4
6) re-run the test in step #5 after adding 'pdb.set_trace()' to the line before the check in neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
   this way we can see what dhcp_options['subnet'] is set to, ensure the check behaves correctly, and try another run with dhcp_options['subnet'] = {}
7) sudo add-apt-repository -r -p proposed

[Regression Potential]

This is a minimal change that is backward compatible with the previous check. The new check can still handle 'not None' in addition to handling an empty dictionary correctly. This has been fixed in Ubuntu Victoria packages (and above) since 2022-01-12, and has been fixed in the upstream stable/ussuri branch since 2021-10-25.

[Jakub Libosvar (libosvar)]

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.

[Rodolfo Alonso (rodolfo-alonso-hernandez)]

Hello Jakub:

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

[Rodolfo Alonso (rodolfo-alonso-hernandez)]

(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.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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)

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[Corey Bryant (corey.bryant)]

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

[Andreas Hasenack (ahasenack)]

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.

[Corey Bryant (corey.bryant)]

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!

[nikhil kshirsagar (nkshirsagar)]

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...

[Rodolfo Alonso (rodolfo-alonso-hernandez)]

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.

[nikhil kshirsagar (nkshirsagar)]

Hi Rodolfo,

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

regards,
Nikhil.

[nikhil kshirsagar (nkshirsagar)]

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

[Rodolfo Alonso (rodolfo-alonso-hernandez)]

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.

[nikhil kshirsagar (nkshirsagar)]

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.

[Rafael Lopez (rafael.lopez)]

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)

[nikhil kshirsagar (nkshirsagar)]

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.

[Launchpad Janitor (janitor)]

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

[Chris Halse Rogers (raof)]

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.

[Corey Bryant (corey.bryant)]

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.

[Corey Bryant (corey.bryant)]

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).

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron ussuri-eol release.