Browse Source

Do not modify passed by reference variables in mechanism_driver

Test test_update_port_with_empty_data [0] from TestOVNMechanismDriverPortsV2
class in neutron is runned agains:
neutron.tests.unit.plugins.ml2.drivers.mechanism_test.TestMechanismDriver
which in fact during update_port_postcommit() does nothing with
the port, only validates it [1].

In networking-ovn this test is runned against real mechanism driver:
networking_ovn.ml2.mech_driver.OVNMechanismDriver. This ends with
little difference - to port dict 'network' information is added
during call of update_port_postcommit(). The port data itself
remains the same.
We shouldn't modify passed by reference variables there. So doing
deepcopy on all provided data.

Here comes also the question if this test inheritance is the
right way.

[0] https://review.opendev.org/#/c/673486
[1] https://github.com/openstack/neutron/blob/master/neutron/tests/unit/plugins/ml2/drivers/mechanism_test.py#L208

Closes-Bug: #1839434
Change-Id: I1ad224960173fe896f6eb8049a188d9e82874068
(cherry picked from commit b2b5e89bca)
(cherry picked from commit 36bd132b40)
changes/07/675607/1
Maciej Józefczyk 1 month ago
parent
commit
5b382bbfe7

+ 5
- 4
networking_ovn/ml2/mech_driver.py View File

@@ -12,6 +12,7 @@
12 12
 #    under the License.
13 13
 #
14 14
 
15
+import copy
15 16
 import threading
16 17
 
17 18
 from neutron_lib.api.definitions import portbindings
@@ -471,7 +472,7 @@ class OVNMechanismDriver(api.MechanismDriver):
471 472
         drastically affect performance.  Raising an exception will
472 473
         result in the deletion of the resource.
473 474
         """
474
-        port = context.current
475
+        port = copy.deepcopy(context.current)
475 476
         port['network'] = context.network.current
476 477
         self._ovn_client.create_port(port)
477 478
         self._notify_dhcp_updated(port['id'])
@@ -524,9 +525,9 @@ class OVNMechanismDriver(api.MechanismDriver):
524 525
         state. It is up to the mechanism driver to ignore state or
525 526
         state changes that it does not know or care about.
526 527
         """
527
-        port = context.current
528
+        port = copy.deepcopy(context.current)
528 529
         port['network'] = context.network.current
529
-        original_port = context.original
530
+        original_port = copy.deepcopy(context.original)
530 531
         original_port['network'] = context.network.current
531 532
         self._ovn_client.update_port(port, port_object=original_port)
532 533
         self._notify_dhcp_updated(port['id'])
@@ -543,7 +544,7 @@ class OVNMechanismDriver(api.MechanismDriver):
543 544
         expected, and will not prevent the resource from being
544 545
         deleted.
545 546
         """
546
-        port = context.current
547
+        port = copy.deepcopy(context.current)
547 548
         port['network'] = context.network.current
548 549
         self._ovn_client.delete_port(port['id'], port_object=port)
549 550
 

+ 12
- 4
networking_ovn/tests/unit/ml2/test_mech_driver.py View File

@@ -12,6 +12,8 @@
12 12
 #    under the License.
13 13
 #
14 14
 
15
+import copy
16
+
15 17
 import mock
16 18
 from webob import exc
17 19
 
@@ -1465,10 +1467,10 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
1465 1467
         fake_port = fakes.FakePort.create_one_port(
1466 1468
             attrs={'status': const.PORT_STATUS_DOWN}).info()
1467 1469
         fake_ctx = mock.Mock(current=fake_port)
1468
-
1469 1470
         self.mech_driver.create_port_postcommit(fake_ctx)
1470
-
1471
-        mock_create_port.assert_called_once_with(fake_port)
1471
+        passed_fake_port = copy.deepcopy(fake_port)
1472
+        passed_fake_port['network'] = fake_ctx.network.current
1473
+        mock_create_port.assert_called_once_with(passed_fake_port)
1472 1474
         mock_notify_dhcp.assert_called_once_with(fake_port['id'])
1473 1475
 
1474 1476
     @mock.patch.object(mech_driver.OVNMechanismDriver,
@@ -1481,8 +1483,14 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
1481 1483
             attrs={'status': const.PORT_STATUS_ACTIVE}).info()
1482 1484
         fake_ctx = mock.Mock(current=fake_port, original=fake_port)
1483 1485
         self.mech_driver.update_port_postcommit(fake_ctx)
1486
+
1487
+        passed_fake_port = copy.deepcopy(fake_port)
1488
+        passed_fake_port['network'] = fake_ctx.network.current
1489
+        passed_fake_port_orig = copy.deepcopy(fake_ctx.original)
1490
+        passed_fake_port_orig['network'] = fake_ctx.network.current
1491
+
1484 1492
         mock_update_port.assert_called_once_with(
1485
-            fake_port, port_object=fake_ctx.original)
1493
+            passed_fake_port, port_object=passed_fake_port_orig)
1486 1494
         mock_notify_dhcp.assert_called_once_with(fake_port['id'])
1487 1495
 
1488 1496
     def _test__update_dnat_entry_if_needed(self, up=True):

Loading…
Cancel
Save