From 8a4c62d094a5419d878c38aa9017e40931a08f21 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Fri, 23 Sep 2022 09:11:25 +0200 Subject: [PATCH 1/3] Since OVN 20.06, config is stored in "Chassis.other_config" Since OVN 20.06 [1], the OVN configuration is stored in "Chassis.other_config". Since OVN 22.09, the "Chassis" configuration stored in "Chassis.other_config" will not be replicated to "Chassis.external_ids". The ML2/OVN plugin tries to retrieve the "Chassis" configuration from the "other_config" field first; if this field does not exist (in OVN versions before 20.06), the plugin will use "external_ids" field instead. Neutron will be compatible with the different OVN versions (with and without "other_config" field). [1]https://github.com/ovn-org/ovn/commit/74d90c2223d0a8c123823fb849b4c2de58c296e4 [2]https://github.com/ovn-org/ovn/commit/51309429cc3032a0cb422603e7bbda4905ca01ae NOTE: this patch is similar to [1], but in this case neutron keeps compatibility with the different OVN versions (with and without "other_config" field). Since [2], the Neutron CI has a new job that uses the OVN/OVS packages distributed by the operating system installed by the CI (in this case, Ubuntu 20.04 and OVN 20.03). [1]https://review.opendev.org/c/openstack/neutron/+/859642 [2]https://review.opendev.org/c/openstack/neutron/+/860636 conflicting files in cherry pick: - neutron/tests/functional/base.py - neutron/tests/functional/services/ovn_l3/test_plugin.py Closes-Bug: #1990229 Change-Id: I54c8fd4d065ae537f396408df16832b158ee8998 (cherry picked from commit 536498a29a4e7662a4d0b1bb923e2521509ad77a) --- neutron/common/ovn/utils.py | 12 +++++- .../ml2/drivers/ovn/agent/neutron_agent.py | 19 +++++---- .../drivers/ovn/mech_driver/mech_driver.py | 8 ++-- .../mech_driver/ovsdb/extensions/placement.py | 3 +- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 8 ++-- .../ovn/mech_driver/ovsdb/ovsdb_monitor.py | 41 +++++++++++++------ neutron/tests/functional/base.py | 11 +++-- .../ovsdb/extensions/test_placement.py | 10 ++--- .../ovn/mech_driver/ovsdb/test_impl_idl.py | 10 ++--- .../mech_driver/ovsdb/test_ovsdb_monitor.py | 9 ++-- .../ovn/mech_driver/test_mech_driver.py | 6 +-- .../functional/services/ovn_l3/test_plugin.py | 11 +++-- neutron/tests/unit/common/ovn/test_utils.py | 32 +++++++-------- neutron/tests/unit/fake_resources.py | 13 +++--- .../drivers/ovn/agent/test_neutron_agent.py | 2 +- .../mech_driver/ovsdb/test_impl_idl_ovn.py | 2 +- .../mech_driver/ovsdb/test_ovsdb_monitor.py | 39 ++++++++++-------- .../ovn/mech_driver/test_mech_driver.py | 1 + ...chassis-other-config-7db15b9d10bf7f04.yaml | 10 +++++ 19 files changed, 150 insertions(+), 97 deletions(-) create mode 100644 releasenotes/notes/ovn-chassis-other-config-7db15b9d10bf7f04.yaml diff --git a/neutron/common/ovn/utils.py b/neutron/common/ovn/utils.py index 011849b07de..9031b501fe3 100644 --- a/neutron/common/ovn/utils.py +++ b/neutron/common/ovn/utils.py @@ -625,8 +625,8 @@ def compute_address_pairs_diff(ovn_port, neutron_port): def get_ovn_cms_options(chassis): """Return the list of CMS options in a Chassis.""" - return [opt.strip() for opt in chassis.external_ids.get( - constants.OVN_CMS_OPTIONS, '').split(',')] + return [opt.strip() for opt in get_ovn_chassis_other_config(chassis).get( + constants.OVN_CMS_OPTIONS, '').split(',')] def is_gateway_chassis(chassis): @@ -815,3 +815,11 @@ def create_neutron_pg_drop(): }] OvsdbClientTransactCommand.run(command) + + +def get_ovn_chassis_other_config(chassis): + # NOTE(ralonsoh): LP#1990229 to be removed when min OVN version is 22.09 + try: + return chassis.other_config + except AttributeError: + return chassis.external_ids diff --git a/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py b/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py index d6b70201820..c0a234019bf 100644 --- a/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py +++ b/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py @@ -89,7 +89,8 @@ def as_dict(self): 'configurations': { 'chassis_name': self.chassis.name, 'bridge-mappings': - self.chassis.external_ids.get('ovn-bridge-mappings', '')}, + ovn_utils.get_ovn_chassis_other_config(self.chassis).get( + 'ovn-bridge-mappings', '')}, 'start_flag': True, 'agent_type': self.agent_type, 'id': self.agent_id, @@ -141,9 +142,9 @@ class ControllerAgent(NeutronAgent): @staticmethod # it is by default, but this makes pep8 happy def __new__(cls, chassis_private, driver): - external_ids = cls.chassis_from_private(chassis_private).external_ids - if ('enable-chassis-as-gw' in - external_ids.get('ovn-cms-options', [])): + _chassis = cls.chassis_from_private(chassis_private) + other_config = ovn_utils.get_ovn_chassis_other_config(_chassis) + if 'enable-chassis-as-gw' in other_config.get('ovn-cms-options', []): cls = ControllerGatewayAgent return super().__new__(cls) @@ -166,8 +167,9 @@ def description(self): def update(self, chassis_private, clear_down=False): super().update(chassis_private, clear_down) - external_ids = self.chassis_from_private(chassis_private).external_ids - if 'enable-chassis-as-gw' in external_ids.get('ovn-cms-options', []): + _chassis = self.chassis_from_private(chassis_private) + other_config = ovn_utils.get_ovn_chassis_other_config(_chassis) + if 'enable-chassis-as-gw' in other_config.get('ovn-cms-options', []): self.__class__ = ControllerGatewayAgent @@ -176,9 +178,10 @@ class ControllerGatewayAgent(ControllerAgent): def update(self, chassis_private, clear_down=False): super().update(chassis_private, clear_down) - external_ids = self.chassis_from_private(chassis_private).external_ids + _chassis = self.chassis_from_private(chassis_private) + other_config = ovn_utils.get_ovn_chassis_other_config(_chassis) if ('enable-chassis-as-gw' not in - external_ids.get('ovn-cms-options', [])): + other_config.get('ovn-cms-options', [])): self.__class__ = ControllerAgent 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 6f6faca40be..37118ec0894 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -179,7 +179,8 @@ def sb_ovn(self, val): def get_supported_vif_types(self): vif_types = set() for ch in self.sb_ovn.chassis_list().execute(check_error=True): - dp_type = ch.external_ids.get('datapath-type', '') + other_config = ovn_utils.get_ovn_chassis_other_config(ch) + dp_type = other_config.get('datapath-type', '') if dp_type == ovn_const.CHASSIS_DATAPATH_NETDEV: vif_types.add(portbindings.VIF_TYPE_VHOST_USER) else: @@ -961,8 +962,9 @@ def bind_port(self, context): 'agent': agent}) return chassis = agent.chassis - datapath_type = chassis.external_ids.get('datapath-type', '') - iface_types = chassis.external_ids.get('iface-types', '') + other_config = ovn_utils.get_ovn_chassis_other_config(chassis) + datapath_type = other_config.get('datapath-type', '') + iface_types = other_config.get('iface-types', '') iface_types = iface_types.split(',') if iface_types else [] chassis_physnets = self.sb_ovn._get_chassis_physnets(chassis) for segment_to_bind in context.segments_to_bind: diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/placement.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/placement.py index eab14543263..f372807fcad 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/placement.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/placement.py @@ -42,7 +42,8 @@ def _parse_ovn_cms_options(chassis): def _parse_bridge_mappings(chassis): - bridge_mappings = chassis.external_ids.get('ovn-bridge-mappings', '') + other_config = ovn_utils.get_ovn_chassis_other_config(chassis) + bridge_mappings = other_config.get('ovn-bridge-mappings', '') bridge_mappings = helpers.parse_mappings(bridge_mappings.split(','), unique_values=False) return {k: [v] for k, v in bridge_mappings.items()} 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 c1ff8626c34..250936252ee 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 @@ -830,7 +830,8 @@ def from_worker(cls, worker_class, driver=None): return cls(conn) def _get_chassis_physnets(self, chassis): - bridge_mappings = chassis.external_ids.get('ovn-bridge-mappings', '') + other_config = utils.get_ovn_chassis_other_config(chassis) + bridge_mappings = other_config.get('ovn-bridge-mappings', '') mapping_dict = helpers.parse_mappings(bridge_mappings.split(',')) return list(mapping_dict.keys()) @@ -848,7 +849,8 @@ def get_gateway_chassis_from_cms_options(self, name_only=True): return [ch.name if name_only else ch for ch in self.chassis_list().execute(check_error=True) if ovn_const.CMS_OPT_CHASSIS_AS_GW in - ch.external_ids.get(ovn_const.OVN_CMS_OPTIONS, '').split(',')] + utils.get_ovn_chassis_other_config(ch).get( + ovn_const.OVN_CMS_OPTIONS, '').split(',')] def get_chassis_and_physnets(self): chassis_info_dict = {} @@ -867,7 +869,7 @@ def get_chassis_by_card_serial_from_cms_options(self, if ('{}={}' .format(ovn_const.CMS_OPT_CARD_SERIAL_NUMBER, card_serial_number) - in ch.external_ids.get( + in utils.get_ovn_chassis_other_config(ch).get( ovn_const.OVN_CMS_OPTIONS, '').split(',')): return ch msg = _('Chassis with %s %s %s does not exist' diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py index 21866b77983..ea74781c8df 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py @@ -172,12 +172,21 @@ def handle_ha_chassis_group_changes(self, event, row, old): def match_fn(self, event, row, old): if event != self.ROW_UPDATE: return True - # NOTE(lucasgomes): If the external_ids column wasn't updated - # (meaning, Chassis "gateway" status didn't change) just returns - if not hasattr(old, 'external_ids') and event == self.ROW_UPDATE: + + # NOTE(ralonsoh): LP#1990229 to be removed when min OVN version is + # 22.09 + other_config = ('other_config' if hasattr(row, 'other_config') else + 'external_ids') + # NOTE(lucasgomes): If the other_config/external_ids column wasn't + # updated (meaning, Chassis "gateway" status didn't change) just + # returns + if not hasattr(old, other_config) and event == self.ROW_UPDATE: return False - if (old.external_ids.get('ovn-bridge-mappings') != - row.external_ids.get('ovn-bridge-mappings')): + old_br_mappings = utils.get_ovn_chassis_other_config(old).get( + 'ovn-bridge-mappings') + new_br_mappings = utils.get_ovn_chassis_other_config(row).get( + 'ovn-bridge-mappings') + if old_br_mappings != new_br_mappings: return True # Check if either the Gateway status or Availability Zones has # changed in the Chassis @@ -192,8 +201,9 @@ def match_fn(self, event, row, old): def run(self, event, row, old): host = row.hostname phy_nets = [] + new_other_config = utils.get_ovn_chassis_other_config(row) if event != self.ROW_DELETE: - bridge_mappings = row.external_ids.get('ovn-bridge-mappings', '') + bridge_mappings = new_other_config.get('ovn-bridge-mappings', '') mapping_dict = helpers.parse_mappings(bridge_mappings.split(',')) phy_nets = list(mapping_dict) @@ -208,9 +218,10 @@ def run(self, event, row, old): if event == self.ROW_DELETE: kwargs['event_from_chassis'] = row.name elif event == self.ROW_UPDATE: - old_mappings = old.external_ids.get('ovn-bridge-mappings', + old_other_config = utils.get_ovn_chassis_other_config(old) + old_mappings = old_other_config.get('ovn-bridge-mappings', set()) or set() - new_mappings = row.external_ids.get('ovn-bridge-mappings', + new_mappings = new_other_config.get('ovn-bridge-mappings', set()) or set() if old_mappings: old_mappings = set(old_mappings.split(',')) @@ -339,11 +350,17 @@ class ChassisAgentTypeChangeEvent(ChassisEvent): events = (BaseEvent.ROW_UPDATE,) def match_fn(self, event, row, old=None): - if not getattr(old, 'external_ids', False): + # NOTE(ralonsoh): LP#1990229 to be removed when min OVN version is + # 22.09 + other_config = ('other_config' if hasattr(row, 'other_config') else + 'external_ids') + if not getattr(old, other_config, False): return False - agent_type_change = n_agent.NeutronAgent.chassis_from_private( - row).external_ids.get('ovn-cms-options', []) != ( - old.external_ids.get('ovn-cms-options', [])) + chassis = n_agent.NeutronAgent.chassis_from_private(row) + new_other_config = utils.get_ovn_chassis_other_config(chassis) + old_other_config = utils.get_ovn_chassis_other_config(old) + agent_type_change = new_other_config.get('ovn-cms-options', []) != ( + old_other_config.get('ovn-cms-options', [])) return agent_type_change def run(self, event, row, old): diff --git a/neutron/tests/functional/base.py b/neutron/tests/functional/base.py index ade4c7af93e..66019db34a2 100644 --- a/neutron/tests/functional/base.py +++ b/neutron/tests/functional/base.py @@ -423,7 +423,8 @@ def restart(self): self._start_ovn_northd() def add_fake_chassis(self, host, physical_nets=None, external_ids=None, - name=None, enable_chassis_as_gw=False): + name=None, enable_chassis_as_gw=False, + other_config=None): def append_cms_options(ext_ids, value): if 'ovn-cms-options' not in ext_ids: ext_ids['ovn-cms-options'] = value @@ -432,14 +433,15 @@ def append_cms_options(ext_ids, value): physical_nets = physical_nets or [] external_ids = external_ids or {} + other_config = other_config or {} if enable_chassis_as_gw: - append_cms_options(external_ids, 'enable-chassis-as-gw') + append_cms_options(other_config, 'enable-chassis-as-gw') bridge_mapping = ",".join(["%s:br-provider%s" % (phys_net, i) for i, phys_net in enumerate(physical_nets)]) if name is None: name = uuidutils.generate_uuid() - external_ids['ovn-bridge-mappings'] = bridge_mapping + other_config['ovn-bridge-mappings'] = bridge_mapping # We'll be using different IP addresses every time for the Encap of # the fake chassis as the SB schema doesn't allow to have two entries # with same (ip,type) pairs as of OVS 2.11. This shouldn't have any @@ -450,7 +452,8 @@ def append_cms_options(ext_ids, value): self._counter += 1 chassis = self.sb_api.chassis_add( name, ['geneve'], '172.24.4.%d' % self._counter, - external_ids=external_ids, hostname=host).execute(check_error=True) + external_ids=external_ids, hostname=host, + other_config=other_config).execute(check_error=True) if self.sb_api.is_table_present('Chassis_Private'): nb_cfg_timestamp = timeutils.utcnow_ts() * 1000 self.sb_api.db_create( diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_placement.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_placement.py index 5e808fe7eaf..327718d7c93 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_placement.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_placement.py @@ -72,7 +72,7 @@ def setUp(self, maintenance_worker=False, service_plugins=None): self.mock_send_batch = mock.patch.object( placement_extension, '_send_deferred_batch').start() - def _build_external_ids(self, bandwidths, inventory_defaults, hypervisors): + def _build_other_config(self, bandwidths, inventory_defaults, hypervisors): options = [] if bandwidths: options.append(n_const.RP_BANDWIDTHS + '=' + bandwidths) @@ -85,17 +85,17 @@ def _build_external_ids(self, bandwidths, inventory_defaults, hypervisors): def _create_chassis(self, host, name, physical_nets=None, bandwidths=None, inventory_defaults=None, hypervisors=None): - external_ids = self._build_external_ids(bandwidths, inventory_defaults, + other_config = self._build_other_config(bandwidths, inventory_defaults, hypervisors) self.add_fake_chassis(host, physical_nets=physical_nets, - external_ids=external_ids, name=name) + other_config=other_config, name=name) def _update_chassis(self, name, bandwidths=None, inventory_defaults=None, hypervisors=None): - external_ids = self._build_external_ids(bandwidths, inventory_defaults, + other_config = self._build_other_config(bandwidths, inventory_defaults, hypervisors) self.sb_api.db_set( - 'Chassis', name, ('external_ids', external_ids) + 'Chassis', name, ('other_config', other_config) ).execute(check_error=True) def _check_placement_config(self, expected_chassis): 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 d139385ea34..4c5b0be23f1 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 @@ -52,11 +52,11 @@ def setUp(self): super(TestSbApi, self).setUp() self.data = { 'chassis': [ - {'external_ids': {'ovn-bridge-mappings': + {'other_config': {'ovn-bridge-mappings': 'public:br-ex,private:br-0'}}, - {'external_ids': {'ovn-bridge-mappings': + {'other_config': {'ovn-bridge-mappings': 'public:br-ex,public2:br-ex2'}}, - {'external_ids': {'ovn-bridge-mappings': + {'other_config': {'ovn-bridge-mappings': 'public:br-ex'}}, ] } @@ -70,7 +70,7 @@ def load_test_data(self): txn.add(self.api.chassis_add( chassis['name'], ['geneve'], chassis['hostname'], hostname=chassis['hostname'], - external_ids=chassis['external_ids'])) + other_config=chassis['other_config'])) def test_get_chassis_hostname_and_physnets(self): mapping = self.api.get_chassis_hostname_and_physnets() @@ -97,7 +97,7 @@ def test_get_chassis_and_physnets(self): def test_multiple_physnets_in_one_bridge(self): self.data = { 'chassis': [ - {'external_ids': {'ovn-bridge-mappings': 'p1:br-ex,p2:br-ex'}} + {'other_config': {'ovn-bridge-mappings': 'p1:br-ex,p2:br-ex'}} ] } self.load_test_data() diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py index 71e4eda29f9..d63d54ca749 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py @@ -437,9 +437,8 @@ def setUp(self): chassis_name, self.mech_driver.agent_chassis_table) self.mech_driver.sb_ovn.idl.notify_handler.watch_event(row_event) self.chassis_name = self.add_fake_chassis( - self.FAKE_CHASSIS_HOST, - external_ids={'ovn-cms-options': 'enable-chassis-as-gw'}, - name=chassis_name) + self.FAKE_CHASSIS_HOST, name=chassis_name, + enable_chassis_as_gw=True) self.assertTrue(row_event.wait()) n_utils.wait_until_true( lambda: len(list(neutron_agent.AgentCache())) == 1) @@ -447,11 +446,11 @@ def setUp(self): def test_agent_change_controller(self): self.assertEqual(neutron_agent.ControllerGatewayAgent, type(neutron_agent.AgentCache()[self.chassis_name])) - self.sb_api.db_set('Chassis', self.chassis_name, ('external_ids', + self.sb_api.db_set('Chassis', self.chassis_name, ('other_config', {'ovn-cms-options': ''})).execute(check_error=True) n_utils.wait_until_true(lambda: neutron_agent.AgentCache()[self.chassis_name]. - chassis.external_ids['ovn-cms-options'] == '') + chassis.other_config['ovn-cms-options'] == '') self.assertEqual(neutron_agent.ControllerAgent, type(neutron_agent.AgentCache()[self.chassis_name])) 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 88e8e3c37ac..e3de3b5e3b4 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 @@ -66,16 +66,16 @@ def setUp(self): self.add_fake_chassis(self.ovs_host) self.add_fake_chassis( self.dpdk_host, - external_ids={'datapath-type': 'netdev', + other_config={'datapath-type': 'netdev', 'iface-types': 'dummy,dummy-internal,dpdkvhostuser'}) self.add_fake_chassis( self.invalid_dpdk_host, - external_ids={'datapath-type': 'netdev', + other_config={'datapath-type': 'netdev', 'iface-types': 'dummy,dummy-internal,geneve,vxlan'}) self.add_fake_chassis( self.smartnic_dpu_host, - external_ids={ovn_const.OVN_CMS_OPTIONS: '{}={}'.format( + other_config={ovn_const.OVN_CMS_OPTIONS: '{}={}'.format( ovn_const.CMS_OPT_CARD_SERIAL_NUMBER, self.smartnic_dpu_serial)}) self.n1 = self._make_network(self.fmt, 'n1', True) diff --git a/neutron/tests/functional/services/ovn_l3/test_plugin.py b/neutron/tests/functional/services/ovn_l3/test_plugin.py index be7fb6c3aef..c7e9fd7445d 100644 --- a/neutron/tests/functional/services/ovn_l3/test_plugin.py +++ b/neutron/tests/functional/services/ovn_l3/test_plugin.py @@ -131,7 +131,7 @@ def test_gateway_chassis_with_cms_and_bridge_mappings(self): # Test if chassis3 is selected as candidate or not. self.chassis3 = self.add_fake_chassis( 'ovs-host3', physical_nets=['physnet1'], - external_ids={'ovn-cms-options': 'enable-chassis-as-gw'}) + other_config={'ovn-cms-options': 'enable-chassis-as-gw'}) self._check_gateway_chassis_candidates([self.chassis3]) def test_gateway_chassis_with_cms_and_no_bridge_mappings(self): @@ -139,7 +139,7 @@ def test_gateway_chassis_with_cms_and_no_bridge_mappings(self): # chassis3 is having enable-chassis-as-gw, but no bridge mappings. self.chassis3 = self.add_fake_chassis( 'ovs-host3', - external_ids={'ovn-cms-options': 'enable-chassis-as-gw'}) + other_config={'ovn-cms-options': 'enable-chassis-as-gw'}) ovn_client = self.l3_plugin._ovn_client ext1 = self._create_ext_network( 'ext1', 'vlan', 'physnet1', 1, "10.0.0.1", "10.0.0.0/24") @@ -484,7 +484,7 @@ def _get_result_dict(): self.skipTest('L3 HA not supported') ovn_client = self.l3_plugin._ovn_client chassis4 = self.add_fake_chassis( - 'ovs-host4', physical_nets=['physnet4'], external_ids={ + 'ovs-host4', physical_nets=['physnet4'], other_config={ 'ovn-cms-options': 'enable-chassis-as-gw'}) ovn_client._ovn_scheduler = l3_sched.OVNGatewayLeastLoadedScheduler() ext1 = self._create_ext_network( @@ -516,7 +516,7 @@ def _get_result_dict(): # Add another chassis as a gateway chassis chassis5 = self.add_fake_chassis( - 'ovs-host5', physical_nets=['physnet4'], external_ids={ + 'ovs-host5', physical_nets=['physnet4'], other_config={ 'ovn-cms-options': 'enable-chassis-as-gw'}) # Add a node as compute node. Compute node wont be # used to schedule the router gateway ports therefore @@ -546,8 +546,7 @@ def test_gateway_chassis_rebalance_max_chassis(self): chassis_list.append( self.add_fake_chassis( 'ovs-host%s' % i, physical_nets=['physnet1'], - external_ids={ - 'ovn-cms-options': 'enable-chassis-as-gw'})) + other_config={'ovn-cms-options': 'enable-chassis-as-gw'})) ext1 = self._create_ext_network( 'ext1', 'vlan', 'physnet1', 1, "10.0.0.1", "10.0.0.0/24") diff --git a/neutron/tests/unit/common/ovn/test_utils.py b/neutron/tests/unit/common/ovn/test_utils.py index a12874a9a18..c6960a88d36 100644 --- a/neutron/tests/unit/common/ovn/test_utils.py +++ b/neutron/tests/unit/common/ovn/test_utils.py @@ -62,33 +62,31 @@ def test_get_system_dns_resolvers(self): def test_is_gateway_chassis(self): chassis = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ - 'external_ids': {'ovn-cms-options': 'enable-chassis-as-gw'}}) + 'other_config': {'ovn-cms-options': 'enable-chassis-as-gw'}}) non_gw_chassis_0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ - 'external_ids': {'ovn-cms-options': ''}}) - non_gw_chassis_1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={}) - non_gw_chassis_2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ - 'external_ids': {}}) + 'other_config': {'ovn-cms-options': ''}}) + non_gw_chassis_1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ + 'other_config': {}}) self.assertTrue(utils.is_gateway_chassis(chassis)) self.assertFalse(utils.is_gateway_chassis(non_gw_chassis_0)) self.assertFalse(utils.is_gateway_chassis(non_gw_chassis_1)) - self.assertFalse(utils.is_gateway_chassis(non_gw_chassis_2)) def test_get_chassis_availability_zones_no_azs(self): chassis = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ - 'external_ids': {'ovn-cms-options': 'enable-chassis-as-gw'}}) + 'other_config': {'ovn-cms-options': 'enable-chassis-as-gw'}}) self.assertEqual(set(), utils.get_chassis_availability_zones(chassis)) def test_get_chassis_availability_zones_one_az(self): chassis = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ - 'external_ids': {'ovn-cms-options': + 'other_config': {'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az0'}}) self.assertEqual( {'az0'}, utils.get_chassis_availability_zones(chassis)) def test_get_chassis_availability_zones_multiple_az(self): chassis = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ - 'external_ids': { + 'other_config': { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az0:az1 :az2:: :'}}) self.assertEqual( @@ -97,7 +95,7 @@ def test_get_chassis_availability_zones_multiple_az(self): def test_get_chassis_availability_zones_malformed(self): chassis = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ - 'external_ids': {'ovn-cms-options': + 'other_config': {'ovn-cms-options': 'enable-chassis-as-gw,availability-zones:az0'}}) self.assertEqual( set(), utils.get_chassis_availability_zones(chassis)) @@ -156,16 +154,16 @@ def test_parse_ovn_lb_port_forwarding(self): def test_get_chassis_in_azs(self): ch0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ 'name': 'ch0', - 'external_ids': { + 'other_config': { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az0:az1:az2'}}) ch1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ 'name': 'ch1', - 'external_ids': { + 'other_config': { 'ovn-cms-options': 'enable-chassis-as-gw'}}) ch2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ 'name': 'ch2', - 'external_ids': { + 'other_config': { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az1:az5'}}) @@ -183,21 +181,21 @@ def test_get_chassis_in_azs(self): def test_get_gateway_chassis_without_azs(self): ch0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ 'name': 'ch0', - 'external_ids': { + 'other_config': { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az0:az1:az2'}}) ch1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ 'name': 'ch1', - 'external_ids': { + 'other_config': { 'ovn-cms-options': 'enable-chassis-as-gw'}}) ch2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ 'name': 'ch2', - 'external_ids': { + 'other_config': { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az1:az5'}}) ch3 = fakes.FakeOvsdbRow.create_one_ovsdb_row(attrs={ 'name': 'ch3', - 'external_ids': {}}) + 'other_config': {}}) chassis_list = [ch0, ch1, ch2, ch3] self.assertEqual( diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index 984600735ac..9228f161325 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -868,20 +868,23 @@ def create(attrs=None, az_list=None, chassis_as_gw=False, cms_opts.append('%s=%s' % (ovn_const.CMS_OPT_CARD_SERIAL_NUMBER, card_serial_number)) - external_ids = {} + # NOTE(ralonsoh): LP#1990229, once min OVN version >= 20.06, the CMS + # options and the bridge mappings should be stored only in + # "other_config". + other_config = {} if cms_opts: - external_ids[ovn_const.OVN_CMS_OPTIONS] = ','.join(cms_opts) + other_config[ovn_const.OVN_CMS_OPTIONS] = ','.join(cms_opts) if bridge_mappings: - external_ids['ovn-bridge-mappings'] = ','.join(bridge_mappings) + other_config['ovn-bridge-mappings'] = ','.join(bridge_mappings) chassis_attrs = { 'encaps': [], - 'external_ids': external_ids, + 'external_ids': '', 'hostname': '', 'name': uuidutils.generate_uuid(), 'nb_cfg': 0, - 'other_config': {}, + 'other_config': other_config, 'transport_zones': [], 'vtep_logical_switches': []} diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/agent/test_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/agent/test_neutron_agent.py index ed434cab186..cd92c937534 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/agent/test_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/agent/test_neutron_agent.py @@ -31,7 +31,7 @@ def setUp(self): self.names_ref = [] for i in range(10): # Add 10 agents. chassis_private = fakes.FakeOvsdbRow.create_one_ovsdb_row( - attrs={'name': 'chassis' + str(i)}) + attrs={'name': 'chassis' + str(i), 'other_config': {}}) self.agent_cache.update(ovn_const.OVN_CONTROLLER_AGENT, chassis_private) self.names_ref.append('chassis' + str(i)) 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 8dba2de7be8..1ea2572d6e0 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 @@ -800,7 +800,7 @@ class TestSBImplIdlOvnBase(TestDBImplIdlOvn): 'chassis': [ { 'hostname': 'fake-smartnic-dpu-chassis.fqdn', - 'external_ids': { + 'other_config': { ovn_const.OVN_CMS_OPTIONS: ( 'firstoption,' 'card-serial-number=fake-serial,' diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py index dda8cb33beb..c1d4279fcd4 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py @@ -81,6 +81,9 @@ "name": {"type": "string"}, "hostname": {"type": "string"}, "external_ids": { + "type": {"key": "string", "value": "string", + "min": 0, "max": "unlimited"}}, + "other_config": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, "isRoot": True, @@ -520,7 +523,7 @@ def setUp(self): self.row_json = { "name": "fake-name", "hostname": "fake-hostname", - "external_ids": ['map', [["ovn-bridge-mappings", + "other_config": ['map', [["ovn-bridge-mappings", "fake-phynet1:fake-br1"]]] } self._mock_hash_ring = mock.patch.object( @@ -544,14 +547,18 @@ def _test_chassis_helper(self, event, new_row_json, old_row_json=None): self.sb_idl.notify_handler.notify_loop() def test_chassis_create_event(self): - self._test_chassis_helper('create', self.row_json) + old_row_json = {'other_config': ['map', []]} + self._test_chassis_helper('create', self.row_json, + old_row_json=old_row_json) self.mech_driver.update_segment_host_mapping.assert_called_once_with( 'fake-hostname', ['fake-phynet1']) self.l3_plugin.schedule_unhosted_gateways.assert_called_once_with( event_from_chassis=None) def test_chassis_delete_event(self): - self._test_chassis_helper('delete', self.row_json) + old_row_json = {'other_config': ['map', []]} + self._test_chassis_helper('delete', self.row_json, + old_row_json=old_row_json) self.mech_driver.update_segment_host_mapping.assert_called_once_with( 'fake-hostname', []) self.l3_plugin.schedule_unhosted_gateways.assert_called_once_with( @@ -559,7 +566,7 @@ def test_chassis_delete_event(self): def test_chassis_update_event(self): old_row_json = copy.deepcopy(self.row_json) - old_row_json['external_ids'][1][0][1] = ( + old_row_json['other_config'][1][0][1] = ( "fake-phynet2:fake-br2") self._test_chassis_helper('update', self.row_json, old_row_json) self.mech_driver.update_segment_host_mapping.assert_called_once_with( @@ -568,9 +575,9 @@ def test_chassis_update_event(self): event_from_chassis=None) def test_chassis_update_event_reschedule_not_needed(self): - self.row_json['external_ids'][1].append(['foo_field', 'foo_value_new']) + self.row_json['other_config'][1].append(['foo_field', 'foo_value_new']) old_row_json = copy.deepcopy(self.row_json) - old_row_json['external_ids'][1][1][1] = ( + old_row_json['other_config'][1][1][1] = ( "foo_value") self._test_chassis_helper('update', self.row_json, old_row_json) self.mech_driver.update_segment_host_mapping.assert_not_called() @@ -578,14 +585,14 @@ def test_chassis_update_event_reschedule_not_needed(self): def test_chassis_update_event_reschedule_lost_physnet(self): old_row_json = copy.deepcopy(self.row_json) - self.row_json['external_ids'][1][0][1] = '' + self.row_json['other_config'][1][0][1] = '' self._test_chassis_helper('update', self.row_json, old_row_json) self.l3_plugin.schedule_unhosted_gateways.assert_called_once_with( event_from_chassis='fake-name') def test_chassis_update_event_reschedule_add_physnet(self): old_row_json = copy.deepcopy(self.row_json) - self.row_json['external_ids'][1][0][1] += ',foo_physnet:foo_br' + self.row_json['other_config'][1][0][1] += ',foo_physnet:foo_br' self._test_chassis_helper('update', self.row_json, old_row_json) self.mech_driver.update_segment_host_mapping.assert_called_once_with( 'fake-hostname', ['fake-phynet1', 'foo_physnet']) @@ -594,7 +601,7 @@ def test_chassis_update_event_reschedule_add_physnet(self): def test_chassis_update_event_reschedule_add_and_remove_physnet(self): old_row_json = copy.deepcopy(self.row_json) - self.row_json['external_ids'][1][0][1] = 'foo_physnet:foo_br' + self.row_json['other_config'][1][0][1] = 'foo_physnet:foo_br' self._test_chassis_helper('update', self.row_json, old_row_json) self.mech_driver.update_segment_host_mapping.assert_called_once_with( 'fake-hostname', ['foo_physnet']) @@ -603,7 +610,7 @@ def test_chassis_update_event_reschedule_add_and_remove_physnet(self): def test_chassis_update_empty_no_external_ids(self): old_row_json = copy.deepcopy(self.row_json) - old_row_json.pop('external_ids') + old_row_json.pop('other_config') with mock.patch( 'neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' 'ovsdb_monitor.ChassisEvent.' @@ -636,10 +643,10 @@ def test_handle_ha_chassis_group_changes_create_not_gw(self): def _test_handle_ha_chassis_group_changes_create(self, event): # Chassis - ext_ids = { + other_config = { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az-0'} row = fakes.FakeOvsdbTable.create_one_ovsdb_table( - attrs={'name': 'SpongeBob', 'external_ids': ext_ids}) + attrs={'name': 'SpongeBob', 'other_config': other_config}) # HA Chassis ch0 = fakes.FakeOvsdbTable.create_one_ovsdb_table( attrs={'priority': 10}) @@ -671,10 +678,10 @@ def test_handle_ha_chassis_group_changes_create(self): def _test_handle_ha_chassis_group_changes_delete(self, event): # Chassis - ext_ids = { + other_config = { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az-0'} row = fakes.FakeOvsdbTable.create_one_ovsdb_table( - attrs={'name': 'SpongeBob', 'external_ids': ext_ids}) + attrs={'name': 'SpongeBob', 'other_config': other_config}) # HA Chassis ha_ch = fakes.FakeOvsdbTable.create_one_ovsdb_table( attrs={'priority': 10}) @@ -698,10 +705,10 @@ def test_handle_ha_chassis_group_changes_delete(self): def test_handle_ha_chassis_group_changes_update_no_changes(self): # Assert nothing was done because the update didn't # change the gateway chassis status or the availability zones - ext_ids = { + other_config = { 'ovn-cms-options': 'enable-chassis-as-gw,availability-zones=az-0'} new = fakes.FakeOvsdbTable.create_one_ovsdb_table( - attrs={'name': 'SpongeBob', 'external_ids': ext_ids}) + attrs={'name': 'SpongeBob', 'other_config': other_config}) old = new self.assertIsNone(self.event.handle_ha_chassis_group_changes( self.event.ROW_UPDATE, new, old)) 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 43f0214cd16..660ac27665d 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 @@ -105,6 +105,7 @@ def _add_chassis_agent(self, nb_cfg, agent_type, chassis_private=None): chassis_private.nb_cfg_timestamp, mock.Mock): del chassis_private.nb_cfg_timestamp chassis_private.external_ids = {} + chassis_private.other_config = {} if agent_type == ovn_const.OVN_METADATA_AGENT: chassis_private.external_ids.update({ ovn_const.OVN_AGENT_METADATA_SB_CFG_KEY: nb_cfg, diff --git a/releasenotes/notes/ovn-chassis-other-config-7db15b9d10bf7f04.yaml b/releasenotes/notes/ovn-chassis-other-config-7db15b9d10bf7f04.yaml new file mode 100644 index 00000000000..8123a03be6d --- /dev/null +++ b/releasenotes/notes/ovn-chassis-other-config-7db15b9d10bf7f04.yaml @@ -0,0 +1,10 @@ +--- +other: + - | + Since OVN 20.06, the "Chassis" register configuration is stored in the + "other_config" field and replicated into "external_ids". This replication + is stopped in OVN 22.09. The ML2/OVN plugin tries to retrieve the "Chassis" + configuration from the "other_config" field first; if this field does not + exist (in OVN versions before 20.06), the plugin will use "external_ids" + field instead. Neutron will be compatible with the different OVN versions + (with and without "other_config" field). From 0e427ecc495404e3c2dcb1e32479b89acb8a6b76 Mon Sep 17 00:00:00 2001 From: Maximilian Stinsky Date: Thu, 24 Nov 2022 11:40:04 +0100 Subject: [PATCH 2/3] Fix duplicated routes exceptions Since the train release neutron adds routes with protocol static. Keepalived also adds the same routes with different protocols depending on the keepalived version. This can result in duplicated routes inside network namespaces. On l3 agent restarts those duplicate routes then prevent the l3 agent from updating its router state because it runs into 'RTNETLINK answers: File exists expections' when it tries to execute 'ip route' commands. This patch adds the same protocol static to each virtual route of keepalived's configuration so network namespaces do not run into duplicated routes anymore. Closes-Bug: #1956846 Change-Id: Ic35b5d4b9110b832c10345c45ec62c0923237cfd (cherry picked from commit c813b658d0e5e0c00093f90f849fcf67ddca16cf) --- neutron/agent/linux/keepalived.py | 5 +++ .../tests/functional/agent/l3/framework.py | 8 ++-- .../tests/unit/agent/linux/test_keepalived.py | 44 ++++++++++--------- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/neutron/agent/linux/keepalived.py b/neutron/agent/linux/keepalived.py index 910d0be96e2..a42e42ad1f7 100644 --- a/neutron/agent/linux/keepalived.py +++ b/neutron/agent/linux/keepalived.py @@ -152,6 +152,11 @@ def build_config(self): LOG.warning("keepalived_use_no_track cfg option is True but " "keepalived on host seems to not support this " "option") + # NOTE(mstinsky): neutron and keepalived are adding the same routes on + # primary routers. With this we ensure that both are adding the routes + # with the same procotol and prevent duplicated routes which result in + # neutron exception for ip route commands. + output += ' protocol static' return output diff --git a/neutron/tests/functional/agent/l3/framework.py b/neutron/tests/functional/agent/l3/framework.py index af476b7085a..a5dbfb62068 100644 --- a/neutron/tests/functional/agent/l3/framework.py +++ b/neutron/tests/functional/agent/l3/framework.py @@ -78,11 +78,11 @@ %(int_port_ipv6)s dev %(internal_device_name)s scope link no_track } virtual_routes { - 0.0.0.0/0 via %(default_gateway_ip)s dev %(ex_device_name)s no_track - 8.8.8.0/24 via 19.4.4.4 no_track - %(extra_subnet_cidr)s dev %(ex_device_name)s scope link no_track + 0.0.0.0/0 via %(default_gateway_ip)s dev %(ex_device_name)s no_track protocol static + 8.8.8.0/24 via 19.4.4.4 no_track protocol static + %(extra_subnet_cidr)s dev %(ex_device_name)s scope link no_track protocol static } -}""" +}""" # noqa: E501 # pylint: disable=line-too-long def get_ovs_bridge(br_name): diff --git a/neutron/tests/unit/agent/linux/test_keepalived.py b/neutron/tests/unit/agent/linux/test_keepalived.py index 28474f9dc89..169f075ff95 100644 --- a/neutron/tests/unit/agent/linux/test_keepalived.py +++ b/neutron/tests/unit/agent/linux/test_keepalived.py @@ -164,7 +164,7 @@ class KeepalivedConfTestCase(KeepalivedBaseTestCase, 192.168.55.0/24 dev eth10 no_track } virtual_routes { - 0.0.0.0/0 via 192.168.1.1 dev eth1 no_track + 0.0.0.0/0 via 192.168.1.1 dev eth1 no_track protocol static } } vrrp_instance VR_2 { @@ -247,7 +247,7 @@ class KeepalivedConfWithoutNoTrackTestCase(KeepalivedConfTestCase): 192.168.55.0/24 dev eth10 } virtual_routes { - 0.0.0.0/0 via 192.168.1.1 dev eth1 + 0.0.0.0/0 via 192.168.1.1 dev eth1 protocol static } } vrrp_instance VR_2 { @@ -323,11 +323,11 @@ def test_remove_routes_on_interface(self): def test_build_config(self): expected = """ virtual_routes { - 0.0.0.0/0 via 1.0.0.254 dev eth0 no_track - ::/0 via fe80::3e97:eff:fe26:3bfa/64 dev eth1 no_track - 10.0.0.0/8 via 1.0.0.1 no_track - 20.0.0.0/8 via 2.0.0.2 no_track - 30.0.0.0/8 dev eth0 scope link no_track + 0.0.0.0/0 via 1.0.0.254 dev eth0 no_track protocol static + ::/0 via fe80::3e97:eff:fe26:3bfa/64 dev eth1 no_track protocol static + 10.0.0.0/8 via 1.0.0.1 no_track protocol static + 20.0.0.0/8 via 2.0.0.2 no_track protocol static + 30.0.0.0/8 dev eth0 scope link no_track protocol static }""" with mock.patch.object( keepalived, '_is_keepalived_use_no_track_supported', @@ -337,11 +337,11 @@ def test_build_config(self): def _get_no_track_less_expected_config(self): expected = """ virtual_routes { - 0.0.0.0/0 via 1.0.0.254 dev eth0 - ::/0 via fe80::3e97:eff:fe26:3bfa/64 dev eth1 - 10.0.0.0/8 via 1.0.0.1 - 20.0.0.0/8 via 2.0.0.2 - 30.0.0.0/8 dev eth0 scope link + 0.0.0.0/0 via 1.0.0.254 dev eth0 protocol static + ::/0 via fe80::3e97:eff:fe26:3bfa/64 dev eth1 protocol static + 10.0.0.0/8 via 1.0.0.1 protocol static + 20.0.0.0/8 via 2.0.0.2 protocol static + 30.0.0.0/8 dev eth0 scope link protocol static }""" return expected @@ -395,7 +395,7 @@ def _test_remove_addresses_by_interface(self, no_track_value): 192.168.1.0/24 dev eth1%(no_track)s } virtual_routes { - 0.0.0.0/0 via 192.168.1.1 dev eth1%(no_track)s + 0.0.0.0/0 via 192.168.1.1 dev eth1%(no_track)s protocol static } } vrrp_instance VR_2 { @@ -416,7 +416,7 @@ def _test_remove_addresses_by_interface(self, no_track_value): 192.168.3.0/24 dev eth6%(no_track)s 192.168.55.0/24 dev eth10%(no_track)s } - }""" % {'no_track': no_track_value}) + }""" % {'no_track': no_track_value}) # noqa: E501 # pylint: disable=line-too-long self.assertEqual(expected, config.get_config_str()) @@ -501,8 +501,9 @@ def test_virtual_route_with_dev(self): return_value=True): route = keepalived.KeepalivedVirtualRoute( n_consts.IPv4_ANY, '1.2.3.4', 'eth0') - self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0 no_track', - route.build_config()) + self.assertEqual( + '0.0.0.0/0 via 1.2.3.4 dev eth0 no_track protocol static', + route.build_config()) def test_virtual_route_with_dev_no_track_not_supported(self): with mock.patch.object( @@ -510,14 +511,14 @@ def test_virtual_route_with_dev_no_track_not_supported(self): return_value=False): route = keepalived.KeepalivedVirtualRoute( n_consts.IPv4_ANY, '1.2.3.4', 'eth0') - self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0', + self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0 protocol static', route.build_config()) def test_virtual_route_with_dev_without_no_track(self): cfg.CONF.set_override('keepalived_use_no_track', False) route = keepalived.KeepalivedVirtualRoute(n_consts.IPv4_ANY, '1.2.3.4', 'eth0') - self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0', + self.assertEqual('0.0.0.0/0 via 1.2.3.4 dev eth0 protocol static', route.build_config()) def test_virtual_route_without_dev(self): @@ -525,7 +526,7 @@ def test_virtual_route_without_dev(self): keepalived, '_is_keepalived_use_no_track_supported', return_value=True): route = keepalived.KeepalivedVirtualRoute('50.0.0.0/8', '1.2.3.4') - self.assertEqual('50.0.0.0/8 via 1.2.3.4 no_track', + self.assertEqual('50.0.0.0/8 via 1.2.3.4 no_track protocol static', route.build_config()) def test_virtual_route_without_dev_no_track_not_supported(self): @@ -533,13 +534,14 @@ def test_virtual_route_without_dev_no_track_not_supported(self): keepalived, '_is_keepalived_use_no_track_supported', return_value=False): route = keepalived.KeepalivedVirtualRoute('50.0.0.0/8', '1.2.3.4') - self.assertEqual('50.0.0.0/8 via 1.2.3.4', + self.assertEqual('50.0.0.0/8 via 1.2.3.4 protocol static', route.build_config()) def test_virtual_route_without_dev_without_no_track(self): cfg.CONF.set_override('keepalived_use_no_track', False) route = keepalived.KeepalivedVirtualRoute('50.0.0.0/8', '1.2.3.4') - self.assertEqual('50.0.0.0/8 via 1.2.3.4', route.build_config()) + self.assertEqual('50.0.0.0/8 via 1.2.3.4 protocol static', + route.build_config()) class KeepalivedTrackScriptTestCase(KeepalivedBaseTestCase): From 7672b0e76a32a9e6ceaa92b1bbddcc150030b5d5 Mon Sep 17 00:00:00 2001 From: Fernando Royo Date: Thu, 1 Dec 2022 11:47:58 +0100 Subject: [PATCH 3/3] Dont raise RouterInterfaceNotFound on overlap check router ports A corner case of the fix done in [1] could happend if, as a race scenario, parallel requests evaluate other ports that could be deleted during the process if they had already determined a overlapping, in that case a RouterInterfaceNotFound exception was raised and the request finished with that exception and a 404 status code. This patch removes the exception due to a port not found, because if the port is not found, the related subnet should not participate in the overlap evaluation, so it makes no sense to break the process for a port that no longer exists. It also improves the previous validation to perform the overlapping check, being performed only when we have at least more than one subnet, as the overlapping check with only one subnet did not make sense. Closes-Bug: #1998226 [1] https://review.opendev.org/c/openstack/neutron/+/859143 Change-Id: If4afe6f525c46f9cf7f02d8aae27dfc56144fd62 (cherry picked from commit 92efd8e45bef761fc464c932f1fd60d1c4d7e828) --- neutron/db/l3_db.py | 13 ++++-- neutron/tests/base.py | 12 +++++- neutron/tests/fullstack/test_l3_agent.py | 50 +++++++++++++++--------- 3 files changed, 51 insertions(+), 24 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 96a304dae12..a21b8131829 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -1025,10 +1025,15 @@ def _add_router_port(self, context, port_id, router, device_owner): subnets_id.extend([fixed_ip['subnet_id'] for fixed_ip in port['fixed_ips']]) else: - raise l3_exc.RouterInterfaceNotFound( - router_id=router.id, port_id=rp.port_id) - - if subnets_id: + # due to race conditions maybe the port under analysis is + # deleted, so instead returning a RouterInterfaceNotFound + # we continue the analysis avoiding that port + LOG.debug("Port %s could not be found, it might have been " + "deleted concurrently. Will not be checked for " + "an overlapping router interface.", + rp.port_id) + + if len(subnets_id) > 1: id_filter = {'id': subnets_id} subnets = self._core_plugin.get_subnets(context.elevated(), filters=id_filter) diff --git a/neutron/tests/base.py b/neutron/tests/base.py index 14aa07a77ff..6f8bf486594 100644 --- a/neutron/tests/base.py +++ b/neutron/tests/base.py @@ -492,7 +492,11 @@ def setup_rootwrap(self): root_helper_daemon=get_rootwrap_daemon_cmd()) def _simulate_concurrent_requests_process_and_raise(self, calls, args): + self._simulate_concurrent_requests_process(calls, args, + raise_on_exception=True) + def _simulate_concurrent_requests_process(self, calls, args, + raise_on_exception=False): class SimpleThread(threading.Thread): def __init__(self, q): super(SimpleThread, self).__init__() @@ -529,10 +533,16 @@ def get_exception(self): t.start() q.join() + threads_exceptions = [] for t in threads: e = t.get_exception() if e: - raise e + if raise_on_exception: + raise e + else: + threads_exceptions.append(e) + + return threads_exceptions class PluginFixture(fixtures.Fixture): diff --git a/neutron/tests/fullstack/test_l3_agent.py b/neutron/tests/fullstack/test_l3_agent.py index 84603266dc5..1a39efa844b 100644 --- a/neutron/tests/fullstack/test_l3_agent.py +++ b/neutron/tests/fullstack/test_l3_agent.py @@ -270,25 +270,37 @@ def _is_filter_set(direction): def _test_concurrent_router_subnet_attachment_overlapping_cidr(self, ha=False): tenant_id = uuidutils.generate_uuid() - subnet_cidr = '10.100.0.0/24' - network1 = self.safe_client.create_network( - tenant_id, name='foo-network1') - subnet1 = self.safe_client.create_subnet( - tenant_id, network1['id'], subnet_cidr) - network2 = self.safe_client.create_network( - tenant_id, name='foo-network2') - subnet2 = self.safe_client.create_subnet( - tenant_id, network2['id'], subnet_cidr) + subnet_cidr = '10.200.0.0/24' + # to have many port interactions where race conditions would happen + # deleting ports meanwhile find operations to evaluate the overlapping + subnets = 10 + + funcs = [] + args = [] router = self.safe_client.create_router(tenant_id, ha=ha) - funcs = [self.safe_client.add_router_interface, - self.safe_client.add_router_interface] - args = [(router['id'], subnet1['id']), (router['id'], subnet2['id'])] - self.assertRaises( - exceptions.BadRequest, - self._simulate_concurrent_requests_process_and_raise, - funcs, - args) + for i in range(subnets): + network_tmp = self.safe_client.create_network( + tenant_id, name='foo-network' + str(i)) + subnet_tmp = self.safe_client.create_subnet( + tenant_id, network_tmp['id'], subnet_cidr) + funcs.append(self.safe_client.add_router_interface) + args.append((router['id'], subnet_tmp['id'])) + + exception_requests = self._simulate_concurrent_requests_process( + funcs, args) + + if not all(type(e) == exceptions.BadRequest + for e in exception_requests): + self.fail('Unexpected exception adding interfaces to router from ' + 'different subnets overlapping') + + if not len(exception_requests) >= (subnets - 1): + self.fail('If we have tried to associate %s subnets overlapping ' + 'cidr to the router, we should have received at least ' + '%s or %s rejected requests, but we have only received ' + '%s', (str(subnets), str(subnets - 1), str(subnets), + str(len(exception_requests)))) class TestLegacyL3Agent(TestL3Agent): @@ -441,7 +453,7 @@ def test_external_subnet_changed(self): def test_router_fip_qos_after_admin_state_down_up(self): self._router_fip_qos_after_admin_state_down_up() - def test_concurrent_router_subnet_attachment_overlapping_cidr_(self): + def test_concurrent_router_subnet_attachment_overlapping_cidr(self): self._test_concurrent_router_subnet_attachment_overlapping_cidr() @@ -601,6 +613,6 @@ def test_external_subnet_changed(self): def test_router_fip_qos_after_admin_state_down_up(self): self._router_fip_qos_after_admin_state_down_up(ha=True) - def test_concurrent_router_subnet_attachment_overlapping_cidr_(self): + def test_concurrent_router_subnet_attachment_overlapping_cidr(self): self._test_concurrent_router_subnet_attachment_overlapping_cidr( ha=True)