From 47b9511ec5da9b8c8721316f7671bc794c848147 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Fri, 7 Apr 2023 17:06:40 -0400 Subject: [PATCH 1/2] OVN: Always try and create a metadata port on subnets When a subnet is updated, for example, to disable then re-enable DHCP on it, if there is no metadata port it will just return without trying to allocate an IP, leaving DHCP unusable on the subnet. This could happen if an admin, even accidentally, deletes the DHCP port on a subnet while DHCP is disabled. This also makes OVN behave like ML2/OVS, which will re-create the DHCP port when the enable_dhcp flag is changed to false and back to true. Change-Id: I943f2fb4db9dc33dc372f844d6133faff415befe Closes-bug: #2015377 (cherry picked from commit 267efd298479d66c64d55a76bd21c9664080f76a) (cherry picked from commit 0681f8b3ad43aafa6e6af521f2b0aaa5923041a1) --- .../ovn/mech_driver/ovsdb/ovn_client.py | 26 ++++---- .../ovn/mech_driver/ovsdb/ovn_db_sync.py | 2 +- .../ovn/mech_driver/test_mech_driver.py | 44 +++++++++++-- .../ovn/mech_driver/test_mech_driver.py | 65 ++++++++++++++++--- ...create-metadata-port-76e2c0e651267aa0.yaml | 11 ++++ 5 files changed, 121 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/ovn-recreate-metadata-port-76e2c0e651267aa0.yaml 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 19cb2e81330..67d76a461a6 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 @@ -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 diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py index d8bf3b74646..2c7482b16a6 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py +++ b/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']) diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 18f3f0d554a..ee92f68dbdb 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/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') 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 aee657c781e..d8141c978bf 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 @@ -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): diff --git a/releasenotes/notes/ovn-recreate-metadata-port-76e2c0e651267aa0.yaml b/releasenotes/notes/ovn-recreate-metadata-port-76e2c0e651267aa0.yaml new file mode 100644 index 00000000000..dfe077945bb --- /dev/null +++ b/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 + `_. From 9e088512b499ab7ac4e8c0008579606674b29185 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Thu, 19 Jan 2023 20:11:09 +0100 Subject: [PATCH 2/2] Honor debug mode in keepalived-state-change script logs This patch removes the config option "debug" override done during the script initialization. Closes-Bug: #2003534 Change-Id: I403d73a1f35cb6314c814f25628a83d3e111e0fe (cherry picked from commit 1a3bdff18abef093c0633bd39187f5a0e582535f) --- neutron/agent/l3/keepalived_state_change.py | 1 - .../tests/functional/agent/l3/test_keepalived_state_change.py | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/neutron/agent/l3/keepalived_state_change.py b/neutron/agent/l3/keepalived_state_change.py index eb0650b6cf5..49d63a29fbb 100644 --- a/neutron/agent/l3/keepalived_state_change.py +++ b/neutron/agent/l3/keepalived_state_change.py @@ -168,7 +168,6 @@ def handle_sigterm(self, signum, frame): def configure(conf): config.init(sys.argv[1:]) conf.set_override('log_dir', cfg.CONF.conf_dir) - conf.set_override('debug', True) conf.set_override('use_syslog', True) config.setup_logging() privileged.default.set_client_mode(False) diff --git a/neutron/tests/functional/agent/l3/test_keepalived_state_change.py b/neutron/tests/functional/agent/l3/test_keepalived_state_change.py index 65ae136bb3f..07f67ad77b8 100644 --- a/neutron/tests/functional/agent/l3/test_keepalived_state_change.py +++ b/neutron/tests/functional/agent/l3/test_keepalived_state_change.py @@ -85,7 +85,9 @@ def _generate_cmd_opts(self, monitor_interface=None, cidr=None): '--pid_file=%s' % self.pid_file, '--state_path=%s' % self.conf_dir, '--user=%s' % os.geteuid(), - '--group=%s' % os.getegid()] + '--group=%s' % os.getegid(), + '--debug', + ] def _search_in_file(self, file_name, text): def text_in_file():