Merge "OVN: Always try and create a metadata port on subnets" into stable/yoga

Author: Zuul

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -2318,7 +2318,7 @@ def create_subnet(self, context, subnet, network):
             mport_updated = False
             if subnet['ip_version'] == const.IP_VERSION_4:
                 mport_updated = self.update_metadata_port(
-                    context, network['id'], subnet=subnet)
+                    context, network, subnet=subnet)
             if subnet['ip_version'] == const.IP_VERSION_6 or not mport_updated:
                 # NOTE(ralonsoh): if IPv4 but the metadata port has not been
                 # updated, the DHPC options register has not been created.
@@ -2338,7 +2338,7 @@ def update_subnet(self, context, subnet, network, txn=None):
             subnet['id'])['subnet']
 
         if subnet['enable_dhcp'] or ovn_subnet:
-            self.update_metadata_port(context, network['id'], subnet=subnet)
+            self.update_metadata_port(context, network, subnet=subnet)
 
         check_rev_cmd = self._nb_idl.check_revision_number(
             subnet['id'], subnet, ovn_const.TYPE_SUBNETS)
@@ -2434,8 +2434,9 @@ def create_metadata_port(self, context, network):
         if not ovn_conf.is_ovn_metadata_enabled():
             return
 
-        if self._find_metadata_port(context, network['id']):
-            return
+        metadata_port = self._find_metadata_port(context, network['id'])
+        if metadata_port:
+            return metadata_port
 
         # Create a neutron port for DHCP/metadata services
         filters = {'network_id': [network['id']]}
@@ -2450,16 +2451,19 @@ def create_metadata_port(self, context, network):
                          }
                 }
         # TODO(boden): rehome create_port into neutron-lib
-        p_utils.create_port(self._plugin, context, port)
+        return p_utils.create_port(self._plugin, context, port)
 
-    def update_metadata_port(self, context, network_id, subnet=None):
+    def update_metadata_port(self, context, network, subnet=None):
         """Update metadata port.
 
         This function will allocate an IP address for the metadata port of
         the given network in all its IPv4 subnets or the given subnet. Returns
         "True" if the metadata port has been updated and "False" if OVN
-        metadata is disabled or the metadata port does not exist.
+        metadata is disabled or the metadata port does not exist or
+        cannot be created.
         """
+        network_id = network['id']
+
         def update_metadata_port_fixed_ips(metadata_port, add_subnet_ids,
                                            del_subnet_ids):
             wanted_fixed_ips = [
@@ -2478,11 +2482,11 @@ def update_metadata_port_fixed_ips(metadata_port, add_subnet_ids,
         if not ovn_conf.is_ovn_metadata_enabled():
             return False
 
-        # Retrieve the metadata port of this network
-        metadata_port = self._find_metadata_port(context, network_id)
+        # Retrieve or create the metadata port of this network
+        metadata_port = self.create_metadata_port(context, network)
         if not metadata_port:
-            LOG.error("Metadata port couldn't be found for network %s",
-                      network_id)
+            LOG.error("Metadata port could not be found or created "
+                      "for network %s", network_id)
             return False
 
         port_subnet_ids = set(ip['subnet_id'] for ip in

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py

@@ -951,7 +951,7 @@ def _sync_metadata_ports(self, ctx, db_ports):
                 try:
                     # Make sure that this port has an IP address in all the
                     # subnets
-                    self._ovn_client.update_metadata_port(ctx, net['id'])
+                    self._ovn_client.update_metadata_port(ctx, net)
                 except n_exc.IpAddressGenerationFailure:
                     LOG.error('Could not allocate IP addresses for '
                               'metadata port in network %s', net['id'])

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py

@@ -995,6 +995,7 @@ class TestMetadataPorts(base.TestOVNFunctionalBase):
 
     def setUp(self, *args, **kwargs):
         super().setUp(*args, **kwargs)
+        self.plugin = self.mech_driver._plugin
         self._ovn_client = self.mech_driver._ovn_client
         self.meta_regex = re.compile(r'%s,(\d+\.\d+\.\d+\.\d+)' %
                                      constants.METADATA_V4_CIDR)
@@ -1017,7 +1018,7 @@ def _list_ports_ovn(self, net_id=None):
         res = self._list_ports(self.fmt, net_id=net_id)
         return self.deserialize(self.fmt, res)['ports']
 
-    def _check_metadata_port(self, net_id, fixed_ip):
+    def _check_metadata_port(self, net_id, fixed_ip, fail=True):
         for port in self._list_ports_ovn(net_id=net_id):
             if ovn_client.OVNClient.is_metadata_port(port):
                 self.assertEqual(net_id, port['network_id'])
@@ -1027,13 +1028,17 @@ def _check_metadata_port(self, net_id, fixed_ip):
                     self.assertEqual([], port['fixed_ips'])
                 return port['id']
 
-        self.fail('Metadata port is not present in network %s or data is not '
-                  'correct' % self.n1_id)
+        if fail:
+            self.fail('Metadata port is not present in network %s or data is '
+                      'not correct' % self.n1_id)
 
     def _check_subnet_dhcp_options(self, subnet_id, cidr):
-        # This method checks the DHCP options CIDR and returns, if exits, the
-        # metadata port IP address, included in the classless static routes.
+        # This method checks DHCP options for a subnet ID, and if they exist,
+        # verifies the CIDR matches. Returns the metadata port IP address
+        # if it is included in the classless static routes, else returns None.
         dhcp_opts = self._ovn_client._nb_idl.get_subnet_dhcp_options(subnet_id)
+        if not dhcp_opts['subnet']:
+            return
         self.assertEqual(cidr, dhcp_opts['subnet']['cidr'])
         routes = dhcp_opts['subnet']['options'].get('classless_static_route')
         if not routes:
@@ -1062,6 +1067,35 @@ def test_subnet_ipv4(self):
         fixed_ip = {'subnet_id': subnet['id'], 'ip_address': metatada_ip}
         self._check_metadata_port(self.n1_id, fixed_ip)
 
+    def test_update_subnet_ipv4(self):
+        self._create_network_ovn(metadata_enabled=True)
+        subnet = self._create_subnet_ovn('10.0.0.0/24')
+        metatada_ip = self._check_subnet_dhcp_options(subnet['id'],
+                                                      '10.0.0.0/24')
+        fixed_ip = {'subnet_id': subnet['id'], 'ip_address': metatada_ip}
+        port_id = self._check_metadata_port(self.n1_id, fixed_ip)
+
+        # Disable DHCP, port should still be present
+        subnet['enable_dhcp'] = False
+        self._ovn_client.update_subnet(self.context, subnet,
+                                       self.n1['network'])
+        port_id = self._check_metadata_port(self.n1_id, None)
+        self.assertIsNone(self._check_subnet_dhcp_options(subnet['id'], []))
+
+        # Delete metadata port
+        self.plugin.delete_port(self.context, port_id)
+        port_id = self._check_metadata_port(self.n1_id, None, fail=False)
+        self.assertIsNone(port_id)
+
+        # Enable DHCP, metadata port should have been re-created
+        subnet['enable_dhcp'] = True
+        self._ovn_client.update_subnet(self.context, subnet,
+                                       self.n1['network'])
+        metatada_ip = self._check_subnet_dhcp_options(subnet['id'],
+                                                      '10.0.0.0/24')
+        fixed_ip = {'subnet_id': subnet['id'], 'ip_address': metatada_ip}
+        port_id = self._check_metadata_port(self.n1_id, fixed_ip)
+
     def test_subnet_ipv4_no_metadata(self):
         self._create_network_ovn(metadata_enabled=False)
         subnet = self._create_subnet_ovn('10.0.0.0/24')

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py

@@ -32,6 +32,7 @@
 from neutron_lib import context
 from neutron_lib import exceptions as n_exc
 from neutron_lib.plugins import directory
+from neutron_lib.plugins import utils as p_utils
 from neutron_lib.tests import tools
 from neutron_lib.utils import net as n_net
 from oslo_concurrency import processutils
@@ -1736,7 +1737,8 @@ def test_update_subnet_postcommit_enable_dhcp(self):
             self.mech_driver.update_subnet_postcommit(context)
             esd.assert_called_once_with(
                 context.current, context.network.current, mock.ANY)
-            umd.assert_called_once_with(mock.ANY, 'id', subnet=subnet)
+            umd.assert_called_once_with(mock.ANY, context.network.current,
+                                        subnet=subnet)
 
     def test_update_subnet_postcommit_disable_dhcp(self):
         self.mech_driver.nb_ovn.get_subnet_dhcp_options.return_value = {
@@ -1752,7 +1754,8 @@ def test_update_subnet_postcommit_disable_dhcp(self):
                 'update_metadata_port') as umd:
             self.mech_driver.update_subnet_postcommit(context)
             dsd.assert_called_once_with(context.current['id'], mock.ANY)
-            umd.assert_called_once_with(mock.ANY, 'id', subnet=subnet)
+            umd.assert_called_once_with(mock.ANY, context.network.current,
+                                        subnet=subnet)
 
     def test_update_subnet_postcommit_update_dhcp(self):
         self.mech_driver.nb_ovn.get_subnet_dhcp_options.return_value = {
@@ -1769,7 +1772,8 @@ def test_update_subnet_postcommit_update_dhcp(self):
             self.mech_driver.update_subnet_postcommit(context)
             usd.assert_called_once_with(
                 context.current, context.network.current, mock.ANY)
-            umd.assert_called_once_with(mock.ANY, 'id', subnet=subnet)
+            umd.assert_called_once_with(mock.ANY, context.network.current,
+                                        subnet=subnet)
 
     def test__get_port_options(self):
         with mock.patch.object(self.mech_driver._plugin, 'get_subnets') as \
@@ -1955,9 +1959,10 @@ def test_update_metadata_port_with_subnet(self):
             mock_metaport.return_value = {'fixed_ips': fixed_ips,
                                           'id': 'metadata_id'}
             mock_get_subnets.return_value = [{'id': 'subnet1'}]
+            network = {'id': 'net_id'}
             subnet = {'id': 'subnet1', 'enable_dhcp': True}
             self.mech_driver._ovn_client.update_metadata_port(
-                self.context, 'net_id', subnet=subnet)
+                self.context, network, subnet=subnet)
             mock_update_port.assert_not_called()
 
             # Subnet without DHCP, present in port.
@@ -1967,7 +1972,7 @@ def test_update_metadata_port_with_subnet(self):
             mock_get_subnets.return_value = [{'id': 'subnet1'}]
             subnet = {'id': 'subnet1', 'enable_dhcp': False}
             self.mech_driver._ovn_client.update_metadata_port(
-                self.context, 'net_id', subnet=subnet)
+                self.context, network, subnet=subnet)
             port = {'id': 'metadata_id',
                     'port': {'network_id': 'net_id', 'fixed_ips': []}}
             mock_update_port.assert_called_once_with(mock.ANY, 'metadata_id',
@@ -1980,7 +1985,7 @@ def test_update_metadata_port_with_subnet(self):
             mock_get_subnets.return_value = []
             subnet = {'id': 'subnet1', 'enable_dhcp': True}
             self.mech_driver._ovn_client.update_metadata_port(
-                self.context, 'net_id', subnet=subnet)
+                self.context, network, subnet=subnet)
             fixed_ips = [{'subnet_id': 'subnet1'}]
             port = {'id': 'metadata_id',
                     'port': {'network_id': 'net_id', 'fixed_ips': fixed_ips}}
@@ -1994,7 +1999,7 @@ def test_update_metadata_port_with_subnet(self):
             mock_get_subnets.return_value = []
             subnet = {'id': 'subnet1', 'enable_dhcp': False}
             self.mech_driver._ovn_client.update_metadata_port(
-                self.context, 'net_id', subnet=subnet)
+                self.context, network, subnet=subnet)
             mock_update_port.assert_not_called()
 
     def test_update_metadata_port_no_subnet(self):
@@ -2011,10 +2016,11 @@ def test_update_metadata_port_no_subnet(self):
             mock_get_subnets.return_value = [{'id': 'subnet1'},
                                              {'id': 'subnet2'}]
             fixed_ips = [{'subnet_id': 'subnet1', 'ip_address': 'ip_add1'}]
+            network = {'id': 'net_id'}
             mock_metaport.return_value = {'fixed_ips': fixed_ips,
                                           'id': 'metadata_id'}
             self.mech_driver._ovn_client.update_metadata_port(self.context,
-                                                              'net_id')
+                                                              network)
             port = {'id': 'metadata_id',
                     'port': {'network_id': 'net_id', 'fixed_ips': fixed_ips}}
             fixed_ips.append({'subnet_id': 'subnet2'})
@@ -2025,10 +2031,11 @@ def test_update_metadata_port_no_subnet(self):
             # Port with IP in subnet1; subnet1 with DHCP, subnet2 without DHCP.
             mock_get_subnets.return_value = [{'id': 'subnet1'}]
             fixed_ips = [{'subnet_id': 'subnet1', 'ip_address': 'ip_add1'}]
+            network = {'id': 'net_id'}
             mock_metaport.return_value = {'fixed_ips': fixed_ips,
                                           'id': 'metadata_id'}
             self.mech_driver._ovn_client.update_metadata_port(self.context,
-                                                              'net_id')
+                                                              network)
             mock_update_port.assert_not_called()
 
             # Port with IP in subnet1; subnet1 without DHCP.
@@ -2037,13 +2044,51 @@ def test_update_metadata_port_no_subnet(self):
             mock_metaport.return_value = {'fixed_ips': fixed_ips,
                                           'id': 'metadata_id'}
             self.mech_driver._ovn_client.update_metadata_port(self.context,
-                                                              'net_id')
+                                                              network)
             port = {'id': 'metadata_id',
                     'port': {'network_id': 'net_id', 'fixed_ips': []}}
             mock_update_port.assert_called_once_with(
                 mock.ANY, 'metadata_id', port)
             mock_update_port.reset_mock()
 
+    def test_update_metadata_port_no_port(self):
+        ovn_conf.cfg.CONF.set_override('ovn_metadata_enabled', True,
+                                       group='ovn')
+
+        with mock.patch.object(
+                self.mech_driver._ovn_client, '_find_metadata_port') as \
+                mock_find_metaport, \
+                mock.patch.object(self.mech_driver._plugin, 'get_subnets') as \
+                mock_get_subnets, \
+                mock.patch.object(p_utils, 'create_port') as \
+                mock_create_port:
+            # Subnet with DHCP, no port, port created.
+            network = {'id': 'net_id', 'project_id': 'project_id-foo'}
+            subnet = {'id': 'subnet1', 'enable_dhcp': True}
+            fixed_ips = [{'subnet_id': 'subnet1', 'ip_address': 'ip_add1'}]
+            port = {'id': 'metadata_id',
+                    'network_id': 'net_id',
+                    'device_owner': const.DEVICE_OWNER_DISTRIBUTED,
+                    'device_id': 'ovnmeta-%s' % 'net_id',
+                    'fixed_ips': fixed_ips}
+            mock_get_subnets.return_value = [subnet]
+            mock_find_metaport.return_value = None
+
+            # Subnet with DHCP, no port, port create failure.
+            mock_create_port.return_value = None
+            ret_status = self.mech_driver._ovn_client.update_metadata_port(
+                self.context, network, subnet=subnet)
+            self.assertFalse(ret_status)
+            mock_create_port.assert_called_once()
+
+            # Subnet with DHCP, no port, port created successfully.
+            mock_create_port.reset_mock()
+            mock_create_port.return_value = port
+            ret_status = self.mech_driver._ovn_client.update_metadata_port(
+                self.context, network, subnet=subnet)
+            self.assertTrue(ret_status)
+            mock_create_port.assert_called_once()
+
     @mock.patch.object(provisioning_blocks, 'is_object_blocked')
     @mock.patch.object(provisioning_blocks, 'provisioning_complete')
     def test_notify_dhcp_updated(self, mock_prov_complete, mock_is_obj_block):

releasenotes/notes/ovn-recreate-metadata-port-76e2c0e651267aa0.yaml

@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    Fix an issue in the OVN driver where network metadata could
+    become unavailable if the metadata port was ever deleted, even
+    if accidental. To re-create the port, a user can now disable,
+    then enable, DHCP for one of the subnets associated with the
+    network using the Neutron API. This will try and create the
+    port, similar to what happens in the DHCP agent for ML2/OVS.
+    For more information, see bug `2015377
+    <https://bugs.launchpad.net/ubuntu/+source/neutron/+bug/2015377>`_.