Delete trunk bridges to avoid race with Neutron

During the Zed PTG it was decided that to solve the race condition
between os-vif and Neutron when deleting trunk bridges, os-vif will be
responsible of both the creation and the deletion of the bridge (see Day
2 first topic at [1]). This change adds the code to delete trunk
bridges.

[1] https://lists.openstack.org/pipermail/openstack-discuss/2022-April/028164.html

Change-Id: I7d834a0c31c801e96002f42f86409ba274c234e6

vif_plug_ovs/constants.py

@@ -26,3 +26,5 @@ OVS_DPDK_INTERFACE_TYPE = 'dpdk'
 
 # Neutron dead VLAN.
 DEAD_VLAN = 4095
+
+TRUNK_BR_PREFIX = 'tbr-'

vif_plug_ovs/ovs.py

@@ -357,11 +357,19 @@ class OvsPlugin(plugin.PluginBase):
                 vif=vif,
                 err="This vif type is not supported by this plugin")
 
+    def _is_trunk_bridge(self, bridge_name):
+        return bridge_name.startswith(constants.TRUNK_BR_PREFIX)
+
+    def _delete_bridge_if_trunk(self, vif):
+        if self._is_trunk_bridge(vif.network.bridge):
+            self.ovsdb.delete_ovs_bridge(vif.network.bridge)
+
     def _unplug_vhostuser(self, vif, instance_info):
         self.ovsdb.delete_ovs_vif_port(vif.network.bridge,
             OvsPlugin.gen_port_name(
                 constants.OVS_VHOSTUSER_PREFIX,
                 vif.id))
+        self._delete_bridge_if_trunk(vif)
 
     def _unplug_bridge(self, vif, instance_info):
         """UnPlug using hybrid strategy
@@ -375,11 +383,13 @@ class OvsPlugin(plugin.PluginBase):
         linux_net.delete_bridge(vif.bridge_name, v1_name)
 
         self.ovsdb.delete_ovs_vif_port(vif.network.bridge, v2_name)
+        self._delete_bridge_if_trunk(vif)
 
     def _unplug_vif_windows(self, vif, instance_info):
         """Remove port from OVS."""
         self.ovsdb.delete_ovs_vif_port(vif.network.bridge, vif.id,
                                        delete_netdev=False)
+        self._delete_bridge_if_trunk(vif)
 
     def _unplug_port_bridge(self, vif, instance_info):
         """Create a per-VIF OVS bridge and patch pair."""
@@ -392,6 +402,7 @@ class OvsPlugin(plugin.PluginBase):
         self.ovsdb.delete_ovs_vif_port(port_bridge_name, port_bridge_patch)
         self.ovsdb.delete_ovs_vif_port(port_bridge_name, vif.vif_name)
         self.ovsdb.delete_ovs_bridge(port_bridge_name)
+        self._delete_bridge_if_trunk(vif)
 
     def _unplug_vif_generic(self, vif, instance_info):
         """Remove port from OVS."""
@@ -399,6 +410,7 @@ class OvsPlugin(plugin.PluginBase):
         # Iaf15fa7a678ec2624f7c12f634269c465fbad930 this should be correct
         # so this is not removed.
         self.ovsdb.delete_ovs_vif_port(vif.network.bridge, vif.vif_name)
+        self._delete_bridge_if_trunk(vif)
 
     def _unplug_vf(self, vif):
         """Remove port from OVS."""
@@ -420,6 +432,7 @@ class OvsPlugin(plugin.PluginBase):
             vif.network.bridge, representor, delete_netdev=False)
         if datapath == constants.OVS_DATAPATH_SYSTEM:
             linux_net.set_interface_state(representor, 'down')
+        self._delete_bridge_if_trunk(vif)
 
     def unplug(self, vif, instance_info):
         if not hasattr(vif, "port_profile"):

vif_plug_ovs/tests/functional/base.py

@@ -21,3 +21,14 @@ class VifPlugOvsBaseFunctionalTestCase(os_vif_base.BaseFunctionalTestCase):
 
     COMPONENT_NAME = 'vif_plug_ovs'
     PRIVILEGED_GROUP = 'vif_plug_ovs_privileged'
+
+    def _check_bridge(self, name):
+        return self._ovsdb.br_exists(name).execute()
+
+    def _add_bridge(self, name, may_exist=True, datapath_type=None):
+        self._ovsdb.add_br(name, may_exist=may_exist,
+                           datapath_type=datapath_type).execute()
+        self.assertTrue(self._check_bridge(name))
+
+    def _del_bridge(self, name):
+        self._ovsdb.del_br(name).execute()

vif_plug_ovs/tests/functional/ovsdb/test_ovsdb_lib.py

@@ -81,17 +81,6 @@ class TestOVSDBLib(testscenarios.WithScenarios,
     def _list_ports_in_bridge(self, bridge):
         return self._ovsdb.list_ports(bridge).execute()
 
-    def _check_bridge(self, name):
-        return self._ovsdb.br_exists(name).execute()
-
-    def _add_bridge(self, name, may_exist=True, datapath_type=None):
-        self._ovsdb.add_br(name, may_exist=may_exist,
-                           datapath_type=datapath_type).execute()
-        self.assertTrue(self._check_bridge(name))
-
-    def _del_bridge(self, name):
-        self._ovsdb.del_br(name).execute()
-
     def test__set_mtu_request(self):
         port_name = 'port1-' + self.interface
         self._add_bridge(self.brname)

vif_plug_ovs/tests/functional/test_plugin.py

@@ -0,0 +1,100 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import testscenarios
+
+from oslo_concurrency import processutils
+from oslo_config import cfg
+from oslo_utils import uuidutils
+
+from os_vif import objects
+
+from vif_plug_ovs import constants
+from vif_plug_ovs import ovs
+from vif_plug_ovs.ovsdb import ovsdb_lib
+from vif_plug_ovs import privsep
+from vif_plug_ovs.tests.functional import base
+
+
+CONF = cfg.CONF
+
+
+@privsep.vif_plug.entrypoint
+def run_privileged(*full_args):
+    return processutils.execute(*full_args)[0].rstrip()
+
+
+class TestOVSPlugin(testscenarios.WithScenarios,
+                    base.VifPlugOvsBaseFunctionalTestCase):
+
+    scenarios = [
+        ('native', {'interface': 'native'}),
+        ('vsctl', {'interface': 'vsctl'})
+    ]
+
+    def setUp(self):
+        super(TestOVSPlugin, self).setUp()
+        run_privileged('ovs-vsctl', 'set-manager', 'ptcp:6640')
+        self.plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME)
+        self.flags(ovsdb_interface=self.interface, group='os_vif_ovs')
+
+        self.ovs = ovsdb_lib.BaseOVS(CONF.os_vif_ovs)
+        self._ovsdb = self.ovs.ovsdb
+
+        self.profile_ovs = objects.vif.VIFPortProfileOpenVSwitch(
+            interface_id='e65867e0-9340-4a7f-a256-09af6eb7a3aa',
+            datapath_type='netdev')
+
+        self.subnet_bridge_4 = objects.subnet.Subnet(
+            cidr='101.168.1.0/24',
+            dns=['8.8.8.8'],
+            gateway='101.168.1.1',
+            dhcp_server='191.168.1.1')
+
+        self.subnet_bridge_6 = objects.subnet.Subnet(
+            cidr='101:1db9::/64',
+            gateway='101:1db9::1')
+
+        self.subnets = objects.subnet.SubnetList(
+            objects=[self.subnet_bridge_4,
+                     self.subnet_bridge_6])
+
+        self.network_ovs_trunk = objects.network.Network(
+            id='437c6db5-4e6f-4b43-b64b-ed6a11ee5ba7',
+            bridge='%s01' % constants.TRUNK_BR_PREFIX,
+            subnets=self.subnets,
+            vlan=99)
+
+        self.vif_vhostuser_trunk = objects.vif.VIFVHostUser(
+            id='b679325f-ca89-4ee0-a8be-6db1409b69ea',
+            address='ca:fe:de:ad:be:ef',
+            network=self.network_ovs_trunk,
+            path='/var/run/openvswitch/vhub679325f-ca',
+            mode='client',
+            port_profile=self.profile_ovs)
+
+        self.instance = objects.instance_info.InstanceInfo(
+            name='demo',
+            uuid='f0000000-0000-0000-0000-000000000001')
+
+    def test_plug_unplug_ovs_vhostuser_trunk(self):
+        trunk_bridge = '%s01' % constants.TRUNK_BR_PREFIX
+        self.plugin.plug(self.vif_vhostuser_trunk, self.instance)
+        self.addCleanup(self._del_bridge, trunk_bridge)
+        self.assertTrue(self._check_bridge(trunk_bridge))
+
+        other_bridge = 'br-%s' % uuidutils.generate_uuid()
+        self._add_bridge(other_bridge)
+        self.addCleanup(self._del_bridge, other_bridge)
+        self.plugin.unplug(self.vif_vhostuser_trunk, self.instance)
+        self.assertTrue(self._check_bridge(other_bridge))
+        self.assertFalse(self._check_bridge(trunk_bridge))

vif_plug_ovs/tests/unit/test_plugin.py

@@ -51,6 +51,12 @@ class PluginTest(testtools.TestCase):
             subnets=self.subnets,
             vlan=99)
 
+        self.network_ovs_trunk = objects.network.Network(
+            id='437c6db5-4e6f-4b43-b64b-ed6a11ee5ba7',
+            bridge='%s01' % constants.TRUNK_BR_PREFIX,
+            subnets=self.subnets,
+            vlan=99)
+
         self.network_ovs_mtu = objects.network.Network(
             id='437c6db5-4e6f-4b43-b64b-ed6a11ee5ba7',
             bridge='br0',
@@ -106,6 +112,14 @@ class PluginTest(testtools.TestCase):
             mode='client',
             port_profile=self.profile_ovs)
 
+        self.vif_vhostuser_trunk = objects.vif.VIFVHostUser(
+            id='b679325f-ca89-4ee0-a8be-6db1409b69ea',
+            address='ca:fe:de:ad:be:ef',
+            network=self.network_ovs_trunk,
+            path='/var/run/openvswitch/vhub679325f-ca',
+            mode='client',
+            port_profile=self.profile_ovs)
+
         self.vif_vhostuser_client = objects.vif.VIFVHostUser(
             id='b679325f-ca89-4ee0-a8be-6db1409b69ea',
             address='ca:fe:de:ad:be:ef',
@@ -308,35 +322,44 @@ class PluginTest(testtools.TestCase):
     def test_plug_ovs_bridge_windows(self):
         self._check_plug_ovs_windows(self.vif_ovs_hybrid)
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(ovs, 'sys')
     @mock.patch.object(ovs.OvsPlugin, '_unplug_vif_generic')
-    def test_unplug_ovs_port_bridge_false(self, unplug, mock_sys):
+    def test_unplug_ovs_port_bridge_false(self, unplug, mock_sys,
+                                          delete_ovs_bridge):
         mock_sys.platform = 'linux'
         plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME)
         with mock.patch.object(plugin.config, 'per_port_bridge', False):
             plugin.unplug(self.vif_ovs, self.instance)
             unplug.assert_called_once_with(self.vif_ovs, self.instance)
+        delete_ovs_bridge.assert_not_called()
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(ovs, 'sys')
     @mock.patch.object(ovs.OvsPlugin, '_unplug_port_bridge')
-    def test_unplug_ovs_port_bridge_true(self, unplug, mock_sys):
+    def test_unplug_ovs_port_bridge_true(self, unplug, mock_sys,
+                                         delete_ovs_bridge):
         mock_sys.platform = 'linux'
         plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME)
         with mock.patch.object(plugin.config, 'per_port_bridge', True):
             plugin.unplug(self.vif_ovs, self.instance)
             unplug.assert_called_once_with(self.vif_ovs, self.instance)
+        delete_ovs_bridge.assert_not_called()
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(ovs.OvsPlugin, '_unplug_vif_generic')
-    def test_unplug_vif_generic(self, delete_port):
+    def test_unplug_vif_generic(self, delete_port, delete_ovs_bridge):
         plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME)
         plugin._unplug_vif_generic(self.vif_ovs, self.instance)
         delete_port.assert_called_once()
+        delete_ovs_bridge.assert_not_called()
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_vif_port')
     @mock.patch.object(linux_net, 'delete_bridge')
     @mock.patch.object(ovs, 'sys')
     def test_unplug_ovs_bridge(self, mock_sys, delete_bridge,
-                               delete_ovs_vif_port):
+                               delete_ovs_vif_port, delete_ovs_bridge):
         calls = {
             'delete_bridge': [mock.call('qbrvif-xxx-yyy', 'qvbb679325f-ca')],
             'delete_ovs_vif_port': [mock.call('br0', 'qvob679325f-ca')]
@@ -346,6 +369,7 @@ class PluginTest(testtools.TestCase):
         plugin.unplug(self.vif_ovs_hybrid, self.instance)
         delete_bridge.assert_has_calls(calls['delete_bridge'])
         delete_ovs_vif_port.assert_has_calls(calls['delete_ovs_vif_port'])
+        delete_ovs_bridge.assert_not_called()
 
     @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_vif_port')
     @mock.patch.object(ovs, 'sys')
@@ -356,11 +380,15 @@ class PluginTest(testtools.TestCase):
         delete_ovs_vif_port.assert_called_once_with('br0', vif.id,
                                                     delete_netdev=False)
 
-    def test_unplug_ovs_windows(self):
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
+    def test_unplug_ovs_windows(self, delete_ovs_bridge):
         self._check_unplug_ovs_windows(self.vif_ovs)
+        delete_ovs_bridge.assert_not_called()
 
-    def test_unplug_ovs_bridge_windows(self):
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
+    def test_unplug_ovs_bridge_windows(self, delete_ovs_bridge):
         self._check_unplug_ovs_windows(self.vif_ovs_hybrid)
+        delete_ovs_bridge.assert_not_called()
 
     @mock.patch.object(ovs.OvsPlugin, '_create_vif_port')
     def test_plug_ovs_vhostuser(self, _create_vif_port):
@@ -394,14 +422,30 @@ class PluginTest(testtools.TestCase):
         plugin.plug(self.vif_vhostuser_client, self.instance)
         create_ovs_vif_port.assert_has_calls(calls)
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_vif_port')
-    def test_unplug_ovs_vhostuser(self, delete_ovs_vif_port):
+    def test_unplug_ovs_vhostuser(self, delete_ovs_vif_port,
+                                  delete_ovs_bridge):
         calls = {
             'delete_ovs_vif_port': [mock.call('br0', 'vhub679325f-ca')]
         }
         plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME)
         plugin.unplug(self.vif_vhostuser, self.instance)
         delete_ovs_vif_port.assert_has_calls(calls['delete_ovs_vif_port'])
+        delete_ovs_bridge.assert_not_called()
+
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_vif_port')
+    def test_unplug_ovs_vhostuser_trunk(self, delete_ovs_vif_port,
+                                        delete_ovs_bridge):
+        bridge_name = '%s01' % constants.TRUNK_BR_PREFIX
+        calls = {
+            'delete_ovs_vif_port': [mock.call(bridge_name, 'vhub679325f-ca')]
+        }
+        plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME)
+        plugin.unplug(self.vif_vhostuser_trunk, self.instance)
+        delete_ovs_vif_port.assert_has_calls(calls['delete_ovs_vif_port'])
+        delete_ovs_bridge.assert_called_once_with(bridge_name)
 
     @mock.patch.object(ovsdb_lib.BaseOVS, 'ensure_ovs_bridge')
     @mock.patch.object(linux_net, 'get_ifname_by_pci_address')
@@ -446,6 +490,7 @@ class PluginTest(testtools.TestCase):
         set_interface_state.assert_has_calls(calls['set_interface_state'])
         _create_vif_port.assert_has_calls(calls['_create_vif_port'])
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(linux_net, 'get_ifname_by_pci_address')
     @mock.patch.object(linux_net, 'get_vf_num_by_pci_address')
     @mock.patch.object(linux_net, 'get_representor_port')
@@ -455,7 +500,8 @@ class PluginTest(testtools.TestCase):
                                      set_interface_state,
                                      get_representor_port,
                                      get_vf_num_by_pci_address,
-                                     get_ifname_by_pci_address):
+                                     get_ifname_by_pci_address,
+                                     delete_ovs_bridge):
         calls = {
 
             'get_ifname_by_pci_address': [mock.call('0002:24:12.3',
@@ -481,6 +527,7 @@ class PluginTest(testtools.TestCase):
             calls['get_representor_port'])
         delete_ovs_vif_port.assert_has_calls(calls['delete_ovs_vif_port'])
         set_interface_state.assert_has_calls(calls['set_interface_state'])
+        delete_ovs_bridge.assert_not_called()
 
     @mock.patch.object(ovsdb_lib.BaseOVS, 'ensure_ovs_bridge')
     @mock.patch.object(ovs.OvsPlugin, "_create_vif_port")
@@ -491,12 +538,14 @@ class PluginTest(testtools.TestCase):
             ensure_bridge.assert_called_once()
             create_port.assert_called_once()
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(ovs.OvsPlugin, '_unplug_vif_generic')
-    def test_unplug_vif_ovs_smart_nic(self, delete_port):
+    def test_unplug_vif_ovs_smart_nic(self, delete_port, delete_ovs_bridge):
         plugin = ovs.OvsPlugin.load(constants.PLUGIN_NAME)
         with mock.patch.object(plugin.config, 'per_port_bridge', False):
             plugin.unplug(self.vif_ovs_smart_nic, self.instance)
             delete_port.assert_called_once()
+        delete_ovs_bridge.assert_not_called()
 
     @mock.patch.object(linux_net, 'get_dpdk_representor_port_name')
     @mock.patch.object(ovsdb_lib.BaseOVS, 'ensure_ovs_bridge')
@@ -542,10 +591,12 @@ class PluginTest(testtools.TestCase):
         _create_vif_port.assert_has_calls(
             calls['_create_vif_port'])
 
+    @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_bridge')
     @mock.patch.object(linux_net, 'get_dpdk_representor_port_name')
     @mock.patch.object(ovsdb_lib.BaseOVS, 'delete_ovs_vif_port')
     def test_unplug_ovs_vf_dpdk(self, delete_ovs_vif_port,
-                                get_dpdk_representor_port_name):
+                                get_dpdk_representor_port_name,
+                                delete_ovs_bridge):
         devname = 'vfrb679325f-ca'
         get_dpdk_representor_port_name.return_value = devname
         calls = {
@@ -558,6 +609,7 @@ class PluginTest(testtools.TestCase):
         get_dpdk_representor_port_name.assert_has_calls(
             calls['get_dpdk_representor_port_name'])
         delete_ovs_vif_port.assert_has_calls(calls['delete_ovs_vif_port'])
+        delete_ovs_bridge.assert_not_called()
 
     @mock.patch.object(ovsdb_lib.BaseOVS, 'create_patch_port_pair')
     @mock.patch.object(ovsdb_lib.BaseOVS, 'ensure_ovs_bridge')