ovn: first tear down old metadata namespaces, then deploy new

While the reverse order may work, it's considered invalid by OVN and not
guaranteed to work properly since OVN may not necessarily know which of
two ports is the one to configure.

This configuration also triggered a bug in OVN where tearing down a port
after deploying a new one resulted in removing flows that serve the
port.

There is a patch up for review for OVN [1] to better handle multiple
assignment of the same port, but it doesn't make the setup any more
valid.

[1] http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/

Closes-Bug: #1997092
Change-Id: Ic7dbc4e8b00423e58f69646a9e3cedc6f72d6c63
(cherry picked from commit 3093aaa)

Author: Ihar Hrachyshka

neutron/agent/ovn/metadata/agent.py

@@ -319,6 +319,12 @@ def _get_ovn_bridge(self):
                         "br-int instead.")
             return 'br-int'
 
+    def get_networks(self):
+        ports = self.sb_idl.get_ports_on_chassis(self.chassis)
+        return {(str(p.datapath.uuid),
+            ovn_utils.get_network_name_from_datapath(p.datapath))
+            for p in self._vif_ports(ports)}
+
     @_sync_lock
     def sync(self):
         """Agent sync.
@@ -327,16 +333,26 @@ def sync(self):
         chassis are serving metadata. Also, it will tear down those namespaces
         which were serving metadata but are no longer needed.
         """
-        metadata_namespaces = self.ensure_all_networks_provisioned()
+
+        # first, clean up namespaces that should no longer deploy
         system_namespaces = tuple(
             ns.decode('utf-8') if isinstance(ns, bytes) else ns
             for ns in ip_lib.list_network_namespaces())
+        nets = self.get_networks()
+        metadata_namespaces = [
+            self._get_namespace_name(net[1])
+            for net in nets
+        ]
         unused_namespaces = [ns for ns in system_namespaces if
                              ns.startswith(NS_PREFIX) and
                              ns not in metadata_namespaces]
         for ns in unused_namespaces:
             self.teardown_datapath(self._get_datapath_name(ns))
 
+        # now that all obsolete namespaces are cleaned up, deploy required
+        # networks
+        self.ensure_all_networks_provisioned(nets)
+
     @staticmethod
     def _get_veth_name(datapath):
         return ['{}{}{}'.format(n_const.TAP_DEVICE_PREFIX,
@@ -428,8 +444,6 @@ def provision_datapath(self, datapath, net_name):
         and assign the IP addresses to the interface corresponding to the
         metadata port of the network. It will also remove existing IP
         addresses that are no longer needed.
-
-        :return: The metadata namespace name of this datapath
         """
         LOG.debug("Provisioning metadata for network %s", net_name)
         port = self.sb_idl.get_metadata_port_network(datapath)
@@ -537,28 +551,13 @@ def provision_datapath(self, datapath, net_name):
             self.conf, bind_address=n_const.METADATA_V4_IP,
             network_id=net_name)
 
-        return namespace
+    def ensure_all_networks_provisioned(self, nets):
+        """Ensure that all requested datapaths are provisioned.
 
-    def ensure_all_networks_provisioned(self):
-        """Ensure that all datapaths are provisioned.
-
-        This function will make sure that all datapaths with ports bound to
-        our chassis have its namespace, VETH pair and OVS port created and
-        metadata proxy is up and running.
-
-        :return: A list with the namespaces that are currently serving
-        metadata
+        This function will make sure that requested datapaths have their
+        namespaces, VETH pair and OVS ports created and metadata proxies are up
+        and running.
         """
-        # Retrieve all VIF ports in our Chassis
-        ports = self.sb_idl.get_ports_on_chassis(self.chassis)
-        nets = {(str(p.datapath.uuid),
-            ovn_utils.get_network_name_from_datapath(p.datapath))
-            for p in self._vif_ports(ports)}
-        namespaces = []
         # Make sure that all those datapaths are serving metadata
         for datapath, net_name in nets:
-            netns = self.provision_datapath(datapath, net_name)
-            if netns:
-                namespaces.append(netns)
-
-        return namespaces
+            self.provision_datapath(datapath, net_name)

neutron/tests/unit/agent/ovn/metadata/test_agent.py

@@ -71,19 +71,29 @@ def setUp(self):
         self.agent.chassis = 'chassis'
         self.agent.ovn_bridge = 'br-int'
 
+        self.ports = []
+        for i in range(0, 3):
+            self.ports.append(makePort(datapath=DatapathInfo(uuid=str(i),
+                external_ids={'name': 'neutron-%d' % i})))
+        self.agent.sb_idl.get_ports_on_chassis.return_value = self.ports
+
     def test_sync(self):
+
         with mock.patch.object(
                 self.agent, 'ensure_all_networks_provisioned') as enp,\
                 mock.patch.object(
                     ip_lib, 'list_network_namespaces') as lnn,\
                 mock.patch.object(
                     self.agent, 'teardown_datapath') as tdp:
-            enp.return_value = ['ovnmeta-1', 'ovnmeta-2']
             lnn.return_value = ['ovnmeta-1', 'ovnmeta-2']
 
             self.agent.sync()
 
-            enp.assert_called_once_with()
+            enp.assert_called_once_with({
+                (p.datapath.uuid, p.datapath.uuid)
+                for p in self.ports
+            })
+
             lnn.assert_called_once_with()
             tdp.assert_not_called()
 
@@ -95,18 +105,20 @@ def test_sync_teardown_namespace(self):
                     ip_lib, 'list_network_namespaces') as lnn,\
                 mock.patch.object(
                     self.agent, 'teardown_datapath') as tdp:
-            enp.return_value = ['ovnmeta-1', 'ovnmeta-2']
             lnn.return_value = ['ovnmeta-1', 'ovnmeta-2', 'ovnmeta-3',
                                 'ns1', 'ns2']
 
             self.agent.sync()
 
-            enp.assert_called_once_with()
+            enp.assert_called_once_with({
+                (p.datapath.uuid, p.datapath.uuid)
+                for p in self.ports
+            })
             lnn.assert_called_once_with()
             tdp.assert_called_once_with('3')
 
-    def test_ensure_all_networks_provisioned(self):
-        """Test networks are provisioned.
+    def test_get_networks(self):
+        """Test which networks are provisioned.
 
         This test simulates that this chassis has the following ports:
             * datapath '0': 1 port
@@ -115,61 +127,37 @@ def test_ensure_all_networks_provisioned(self):
             * datapath '3': 1 port with type 'external'
             * datapath '5': 1 port with type 'unknown'
 
-        It is expected that only datapaths '0', '1' and '2' are provisioned
-        once.
+        It is expected that only datapaths '0', '1' and '2' are scheduled for
+        provisioning.
         """
 
-        ports = []
-        for i in range(0, 3):
-            ports.append(makePort(datapath=DatapathInfo(uuid=str(i),
-                external_ids={'name': 'neutron-%d' % i})))
-        ports.append(makePort(datapath=DatapathInfo(uuid='1',
+        self.ports.append(makePort(datapath=DatapathInfo(uuid='1',
             external_ids={'name': 'neutron-1'})))
-        ports.append(makePort(datapath=DatapathInfo(uuid='3',
+        self.ports.append(makePort(datapath=DatapathInfo(uuid='3',
             external_ids={'name': 'neutron-3'}), type='external'))
-        ports.append(makePort(datapath=DatapathInfo(uuid='5',
+        self.ports.append(makePort(datapath=DatapathInfo(uuid='5',
             external_ids={'name': 'neutron-5'}), type='unknown'))
 
-        with mock.patch.object(self.agent, 'provision_datapath',
-                               return_value=None) as pdp,\
-                mock.patch.object(self.agent.sb_idl, 'get_ports_on_chassis',
-                                  return_value=ports):
-            self.agent.ensure_all_networks_provisioned()
-
-            expected_calls = [mock.call(str(i), str(i)) for i in range(0, 4)]
-            self.assertEqual(sorted(expected_calls),
-                             sorted(pdp.call_args_list))
+        expected_networks = {(str(i), str(i)) for i in range(0, 4)}
+        self.assertEqual(expected_networks, self.agent.get_networks())
 
     def test_update_datapath_provision(self):
-        ports = []
-        for i in range(0, 3):
-            ports.append(makePort(datapath=DatapathInfo(uuid=str(i),
-                external_ids={'name': 'neutron-%d' % i})))
-        ports.append(makePort(datapath=DatapathInfo(uuid='3',
+        self.ports.append(makePort(datapath=DatapathInfo(uuid='3',
             external_ids={'name': 'neutron-3'}), type='external'))
 
         with mock.patch.object(self.agent, 'provision_datapath',
                                return_value=None) as pdp,\
-                mock.patch.object(self.agent, 'teardown_datapath') as tdp,\
-                mock.patch.object(self.agent.sb_idl, 'get_ports_on_chassis',
-                                  return_value=ports):
+                mock.patch.object(self.agent, 'teardown_datapath') as tdp:
             self.agent.update_datapath('1', 'a')
             self.agent.update_datapath('3', 'b')
             expected_calls = [mock.call('1', 'a'), mock.call('3', 'b')]
             pdp.assert_has_calls(expected_calls)
             tdp.assert_not_called()
 
     def test_update_datapath_teardown(self):
-        ports = []
-        for i in range(0, 3):
-            ports.append(makePort(datapath=DatapathInfo(uuid=str(i),
-                external_ids={'name': 'neutron-%d' % i})))
-
         with mock.patch.object(self.agent, 'provision_datapath',
                                return_value=None) as pdp,\
-                mock.patch.object(self.agent, 'teardown_datapath') as tdp,\
-                mock.patch.object(self.agent.sb_idl, 'get_ports_on_chassis',
-                                  return_value=ports):
+                mock.patch.object(self.agent, 'teardown_datapath') as tdp:
             self.agent.update_datapath('5', 'a')
             tdp.assert_called_once_with('5', 'a')
             pdp.assert_not_called()