diff --git a/doc/source/contributor/internals/ovn/metadata_api.rst b/doc/source/contributor/internals/ovn/metadata_api.rst index 5524980caf5..1c34fdd3c2d 100644 --- a/doc/source/contributor/internals/ovn/metadata_api.rst +++ b/doc/source/contributor/internals/ovn/metadata_api.rst @@ -147,15 +147,8 @@ In neutron-ovn-metadata-agent. Southbound database and look for all rows with the ``chassis`` column set to the host the agent is running on. For all those entries, make sure a metadata proxy instance is spawned for every ``datapath`` (Neutron - network) those ports are attached to. The agent will keep record of the - list of networks it currently has proxies running on by updating the - ``external-ids`` key ``neutron-metadata-proxy-networks`` of the OVN - ``Chassis`` record in the OVN Southbound database that corresponds to this - host. As an example, this key would look like - ``neutron-metadata-proxy-networks=NET1_UUID,NET4_UUID`` meaning that this - chassis is hosting one or more VM's connected to networks 1 and 4 so we - should have a metadata proxy instance running for each. Ensure any running - metadata proxies no longer needed are torn down. + network) those ports are attached to. Ensure any running metadata proxies + no longer needed are torn down. * Open and maintain a connection to the OVN Northbound database (using the ovsdbapp library). On first connection, and anytime a reconnect happens: @@ -218,9 +211,6 @@ Launching a metadata proxy includes: * Starting haproxy in this network namespace. -* Add the network UUID to ``external-ids:neutron-metadata-proxy-networks`` on - the Chassis table for our chassis in OVN Southbound database. - Tearing down a metadata proxy includes: * Removing the network UUID from our chassis. @@ -239,18 +229,12 @@ have to deal with the complexity of it (haproxy instances, network namespaces, etcetera). In this case, the agent would not create the neutron ports needed for metadata. -There could be a race condition when the first VM for a certain network boots -on a hypervisor if it does so before the metadata proxy instance has been -spawned. - Right now, the ``vif-plugged`` event to Nova is sent out when the up column in the OVN Northbound database's Logical_Switch_Port table changes to True, -indicating that the VIF is now up. To overcome this race condition we want -to wait until all network UUID's to which this VM is connected to are present -in ``external-ids:neutron-metadata-proxy-networks`` on the Chassis table -for our chassis in OVN Southbound database. This will delay the event to Nova -until the metadata proxy instance is up and running on the host ensuring the -VM will be able to get the metadata on boot. +indicating that the VIF is now up. There could be a race condition when the +first VM for a certain network boots on a hypervisor if it does so before the +metadata proxy instance has been spawned. Fortunately, retries on cloud-init +should eventually fetch metadata even when this might happen. Alternatives Considered ----------------------- diff --git a/neutron/agent/ovn/metadata/agent.py b/neutron/agent/ovn/metadata/agent.py index d1094ee61da..8de1dabb15d 100644 --- a/neutron/agent/ovn/metadata/agent.py +++ b/neutron/agent/ovn/metadata/agent.py @@ -335,8 +335,6 @@ class MetadataAgent(object): This function will shutdown metadata proxy if it's running and delete the VETH pair, the OVS port and the namespace. """ - self.update_chassis_metadata_networks(datapath, remove=True) - # TODO(dalvarez): Remove this in Y cycle when we are sure that all # namespaces will be created with the Neutron network UUID and not # anymore with the OVN datapath UUID. @@ -515,7 +513,6 @@ class MetadataAgent(object): self.conf, bind_address=n_const.METADATA_V4_IP, network_id=net_name) - self.update_chassis_metadata_networks(net_name) return namespace def ensure_all_networks_provisioned(self): @@ -541,30 +538,3 @@ class MetadataAgent(object): namespaces.append(netns) return namespaces - - # NOTE(lucasagomes): Even tho the metadata agent is a multi-process - # application, there's only one Southbound database IDL instance in - # the agent which handles the OVSDB events therefore we do not need - # the external=True parameter in the @synchronized decorator. - @lockutils.synchronized(CHASSIS_METADATA_LOCK) - def update_chassis_metadata_networks(self, datapath, remove=False): - """Update metadata networks hosted in this chassis. - - Add or remove a datapath from the list of current datapaths that - we're currently serving metadata. - """ - current_dps = self.sb_idl.get_chassis_metadata_networks(self.chassis) - updated = False - if remove: - if datapath in current_dps: - current_dps.remove(datapath) - updated = True - else: - if datapath not in current_dps: - current_dps.append(datapath) - updated = True - - if updated: - with self.sb_idl.create_transaction(check_error=True) as txn: - txn.add(self.sb_idl.set_chassis_metadata_networks( - self.chassis, current_dps)) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py index 229ea0f85be..d80dd53b444 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -43,7 +43,6 @@ from neutron.common.ovn import acl as ovn_acl from neutron.common.ovn import constants as ovn_const from neutron.common.ovn import extensions as ovn_extensions from neutron.common.ovn import utils as ovn_utils -from neutron.common import utils as n_utils from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf from neutron.db import ovn_hash_ring_db from neutron.db import ovn_revision_numbers_db @@ -64,11 +63,6 @@ import neutron.wsgi LOG = log.getLogger(__name__) -METADATA_READY_WAIT_TIMEOUT = 15 - - -class MetadataServiceReadyWaitTimeoutException(Exception): - pass class OVNPortUpdateError(n_exc.BadRequest): @@ -993,7 +987,6 @@ class OVNMechanismDriver(api.MechanismDriver): LOG.info("OVN reports status up for port: %s", port_id) self._update_dnat_entry_if_needed(port_id) - self._wait_for_metadata_provisioned_if_needed(port_id) admin_context = n_context.get_admin_context() provisioning_blocks.provisioning_complete( @@ -1088,61 +1081,6 @@ class OVNMechanismDriver(api.MechanismDriver): if phynet in phynets} segment_service_db.map_segment_to_hosts(context, segment.id, hosts) - def _wait_for_metadata_provisioned_if_needed(self, port_id): - """Wait for metadata service to be provisioned. - - Wait until metadata service has been setup for this port in the chassis - it resides. If metadata is disabled or DHCP is not enabled for its - subnets, this function will return right away. - """ - if ovn_conf.is_ovn_metadata_enabled() and self._sb_ovn: - # Wait until metadata service has been setup for this port in the - # chassis it resides. - result = ( - self._sb_ovn.get_logical_port_chassis_and_datapath(port_id)) - if not result: - LOG.warning("Logical port %s doesn't exist in OVN", port_id) - return - chassis, datapath = result - if not chassis: - LOG.warning("Logical port %s is not bound to a " - "chassis", port_id) - return - - # Check if the port belongs to some IPv4 subnet with DHCP enabled. - context = n_context.get_admin_context() - port = self._plugin.get_port(context, port_id) - port_subnet_ids = set( - ip['subnet_id'] for ip in port['fixed_ips'] if - n_utils.get_ip_version(ip['ip_address']) == const.IP_VERSION_4) - if not port_subnet_ids: - # The port doesn't belong to any IPv4 subnet - return - - subnets = self._plugin.get_subnets(context, filters=dict( - network_id=[port['network_id']], ip_version=[4], - enable_dhcp=True)) - - subnet_ids = set( - s['id'] for s in subnets if s['id'] in port_subnet_ids) - if not subnet_ids: - return - - try: - n_utils.wait_until_true( - lambda: datapath in - self._sb_ovn.get_chassis_metadata_networks(chassis), - timeout=METADATA_READY_WAIT_TIMEOUT, - exception=MetadataServiceReadyWaitTimeoutException) - except MetadataServiceReadyWaitTimeoutException: - # If we reach this point it means that metadata agent didn't - # provision the datapath for this port on its chassis. Either - # the agent is not running or it crashed. We'll complete the - # provisioning block though. - LOG.warning("Metadata service is not ready for port %s, check" - " neutron-ovn-metadata-agent status/logs.", - port_id) - def patch_plugin_merge(self, method_name, new_fn, op=operator.add): old_method = getattr(self._plugin, method_name) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index 1e7a7f7cbdd..5f15bb9a699 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -839,25 +839,6 @@ class OvsdbSbOvnIdl(sb_impl_idl.OvnSbApiIdlImpl, Backend): ('type', '=', 'localport')) return next(iter(cmd.execute(check_error=True)), None) - def get_chassis_metadata_networks(self, chassis_name): - """Return a list with the metadata networks the chassis is hosting.""" - try: - chassis = self.lookup('Chassis', chassis_name) - except idlutils.RowNotFound: - LOG.warning("Couldn't find Chassis named %s in OVN while looking " - "for metadata networks", chassis_name) - return [] - proxy_networks = chassis.external_ids.get( - 'neutron-metadata-proxy-networks', None) - return proxy_networks.split(',') if proxy_networks else [] - - def set_chassis_metadata_networks(self, chassis, networks): - nets = ','.join(networks) if networks else '' - # TODO(twilson) This could just use DbSetCommand - return cmd.UpdateChassisExtIdsCommand( - self, chassis, {'neutron-metadata-proxy-networks': nets}, - if_exists=True) - def set_chassis_neutron_description(self, chassis, description, agent_type): desc_key = (ovn_const.OVN_AGENT_METADATA_DESC_KEY diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py index e3fb7ad0b6e..b6845760fc5 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl.py @@ -123,13 +123,6 @@ class TestSbApi(BaseOvnIdlTest): val = str(uuid.uuid4()) self.assertIsNone(self.api.get_metadata_port_network(val)) - def test_set_get_chassis_metadata_networks(self): - name = self.data['chassis'][0]['name'] - nets = [str(uuid.uuid4()) for _ in range(3)] - self.api.set_chassis_metadata_networks(name, nets).execute( - check_error=True) - self.assertEqual(nets, self.api.get_chassis_metadata_networks(name)) - def _create_bound_port_with_ip(self): chassis, switch, port, binding = self._add_switch_port( self.data['chassis'][0]['name']) diff --git a/neutron/tests/unit/agent/ovn/metadata/test_agent.py b/neutron/tests/unit/agent/ovn/metadata/test_agent.py index cfd73a1db84..2b749665083 100644 --- a/neutron/tests/unit/agent/ovn/metadata/test_agent.py +++ b/neutron/tests/unit/agent/ovn/metadata/test_agent.py @@ -178,10 +178,7 @@ class TestMetadataAgent(base.BaseTestCase): Check that the VETH pair, OVS port and namespace associated to this namespace are deleted and the metadata proxy is destroyed. """ - with mock.patch.object(self.agent, - 'update_chassis_metadata_networks'),\ - mock.patch.object( - ip_netns, 'exists', return_value=True),\ + with mock.patch.object(ip_netns, 'exists', return_value=True),\ mock.patch.object( ip_lib, 'device_exists', return_value=True),\ mock.patch.object( @@ -235,9 +232,6 @@ class TestMetadataAgent(base.BaseTestCase): ip_wrap, 'add_veth', return_value=[ip_lib.IPDevice('ip1'), ip_lib.IPDevice('ip2')]) as add_veth,\ - mock.patch.object( - self.agent, - 'update_chassis_metadata_networks') as update_chassis,\ mock.patch.object( driver.MetadataDriver, 'spawn_monitored_metadata_proxy') as spawn_mdp, \ @@ -273,53 +267,4 @@ class TestMetadataAgent(base.BaseTestCase): spawn_mdp.assert_called_once_with( mock.ANY, 'namespace', 80, mock.ANY, bind_address=n_const.METADATA_V4_IP, network_id='1') - # Check that the chassis has been updated with the datapath. - update_chassis.assert_called_once_with('1') mock_checksum.assert_called_once_with('namespace') - - def _test_update_chassis_metadata_networks_helper( - self, dp, remove, expected_dps, txn_called=True): - current_dps = ['0', '1', '2'] - with mock.patch.object(self.agent.sb_idl, - 'get_chassis_metadata_networks', - return_value=current_dps),\ - mock.patch.object(self.agent.sb_idl, - 'set_chassis_metadata_networks', - retrurn_value=True),\ - mock.patch.object(self.agent.sb_idl, - 'create_transaction') as create_txn_mock: - - self.agent.update_chassis_metadata_networks(dp, remove=remove) - updated_dps = self.agent.sb_idl.get_chassis_metadata_networks( - self.agent.chassis) - - self.assertEqual(updated_dps, expected_dps) - self.assertEqual(create_txn_mock.called, txn_called) - - def test_update_chassis_metadata_networks_add(self): - dp = '4' - remove = False - expected_dps = ['0', '1', '2', '4'] - self._test_update_chassis_metadata_networks_helper( - dp, remove, expected_dps) - - def test_update_chassis_metadata_networks_remove(self): - dp = '2' - remove = True - expected_dps = ['0', '1'] - self._test_update_chassis_metadata_networks_helper( - dp, remove, expected_dps) - - def test_update_chassis_metadata_networks_add_dp_exists(self): - dp = '2' - remove = False - expected_dps = ['0', '1', '2'] - self._test_update_chassis_metadata_networks_helper( - dp, remove, expected_dps, txn_called=False) - - def test_update_chassis_metadata_networks_remove_no_dp(self): - dp = '3' - remove = True - expected_dps = ['0', '1', '2'] - self._test_update_chassis_metadata_networks_helper( - dp, remove, expected_dps, txn_called=False) diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py index 29c3336c290..b85bc228799 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_impl_idl_ovn.py @@ -807,51 +807,3 @@ class TestNBImplIdlOvn(TestDBImplIdlOvn): lb_row = self._find_ovsdb_fake_row(self.lb_table, 'name', 'lb_2') lb = self.nb_ovn_idl.get_floatingip_in_nat_or_lb(fip_id) self.assertEqual(lb['_uuid'], lb_row.uuid) - - -class TestSBImplIdlOvn(TestDBImplIdlOvn): - - fake_set = { - 'chassis': [ - {'name': 'fake-chassis', - 'external_ids': {'neutron-metadata-proxy-networks': 'fake-id'}}], - } - - def setUp(self): - super(TestSBImplIdlOvn, self).setUp() - self.chassis_table = fakes.FakeOvsdbTable.create_one_ovsdb_table() - - self._tables = {} - self._tables['Chassis'] = self.chassis_table - - with mock.patch.object(impl_idl_ovn.OvsdbSbOvnIdl, 'from_worker', - return_value=mock.Mock()): - with mock.patch.object(ovs_idl.Backend, 'autocreate_indices', - create=True): - impl_idl_ovn.OvsdbSbOvnIdl.ovsdb_connection = None - self.sb_ovn_idl = impl_idl_ovn.OvsdbSbOvnIdl(mock.MagicMock()) - - self.sb_ovn_idl.idl.tables = self._tables - - def _load_sb_db(self): - fake_chassis = TestSBImplIdlOvn.fake_set['chassis'] - self._load_ovsdb_fake_rows(self.chassis_table, fake_chassis) - - @mock.patch.object(impl_idl_ovn.LOG, 'warning') - def test_get_chassis_metadata_networks_chassis_empty(self, mock_log): - chassis_name = 'fake-chassis' - result = self.sb_ovn_idl.get_chassis_metadata_networks(chassis_name) - - mock_log.assert_called_once_with( - "Couldn't find Chassis named %s in OVN while looking " - "for metadata networks", chassis_name) - self.assertEqual([], result) - - @mock.patch.object(impl_idl_ovn.LOG, 'warning') - def test_get_chassis_metadata_networks(self, mock_log): - chassis_name = 'fake-chassis' - self._load_sb_db() - result = self.sb_ovn_idl.get_chassis_metadata_networks(chassis_name) - - self.assertFalse(mock_log.called) - self.assertEqual(['fake-id'], result) 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 3bff20c276a..5d9093a40b3 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 @@ -42,7 +42,6 @@ from neutron.common.ovn import acl as ovn_acl from neutron.common.ovn import constants as ovn_const from neutron.common.ovn import hash_ring_manager from neutron.common.ovn import utils as ovn_utils -from neutron.common import utils as n_utils from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf from neutron.db import db_base_plugin_v2 from neutron.db import ovn_revision_numbers_db @@ -960,9 +959,6 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): 'provisioning_complete') as pc, \ mock.patch.object(self.mech_driver, '_update_dnat_entry_if_needed') as ude, \ - mock.patch.object( - self.mech_driver, - '_wait_for_metadata_provisioned_if_needed') as wmp, \ mock.patch.object(self.mech_driver, '_should_notify_nova', return_value=is_compute_port): self.mech_driver.set_port_status_up(port1['port']['id']) @@ -973,7 +969,6 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): provisioning_blocks.L2_AGENT_ENTITY ) ude.assert_called_once_with(port1['port']['id']) - wmp.assert_called_once_with(port1['port']['id']) # If the port does NOT bellong to compute, do not notify Nova # about it's status changes @@ -1064,31 +1059,6 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): ) ude.assert_called_once_with(port1['port']['id'], False) - def _test__wait_for_metadata_provisioned_if_needed(self, enable_dhcp, - wait_expected): - with self.network(tenant_id='test') as net1, \ - self.subnet(network=net1, - enable_dhcp=enable_dhcp) as subnet1, \ - self.port(subnet=subnet1, set_context=True, - tenant_id='test') as port1, \ - mock.patch.object(n_utils, 'wait_until_true') as wut, \ - mock.patch.object(ovn_conf, 'is_ovn_metadata_enabled', - return_value=True): - self.mech_driver._wait_for_metadata_provisioned_if_needed( - port1['port']['id']) - if wait_expected: - self.assertEqual(1, wut.call_count) - else: - wut.assert_not_called() - - def test__wait_for_metadata_provisioned_if_needed(self): - self._test__wait_for_metadata_provisioned_if_needed( - enable_dhcp=True, wait_expected=True) - - def test__wait_for_metadata_provisioned_if_needed_not_needed(self): - self._test__wait_for_metadata_provisioned_if_needed( - enable_dhcp=False, wait_expected=False) - def test_bind_port_unsupported_vnic_type(self): fake_port = fakes.FakePort.create_one_port( attrs={'binding:vnic_type': 'unknown'}).info()