From 982c22dd46bb651c4c5c614262f1a9d68a96cd70 Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Thu, 23 Sep 2021 16:04:00 +0100 Subject: [PATCH] [OVN] Fix updating network segmentation ID The ML2/OVN driver wasn't handling updates to the segmentation ID for a given network. This patch fixes this problem. This patch extends the _update_segmentation_id() method to check on drivers which does not inherits from AgentMechanismDriverBase, which is the case of OVN (which inherits from MechanismDriver). A new method is now called for those drivers to get a list of supported VIF types, called get_supported_vif_types(). Closes-Bug: #1944708 Change-Id: Ibe08bfbc2efc55b9d628cdd0605941b7486186b6 Signed-off-by: Lucas Alvares Gomes --- .../drivers/ovn/mech_driver/mech_driver.py | 14 +++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 9 +++ neutron/plugins/ml2/plugin.py | 28 ++++++--- .../ovn/mech_driver/test_mech_driver.py | 58 +++++++++++++++---- neutron/tests/unit/plugins/ml2/test_plugin.py | 31 ++++++++++ .../ovn-update-vlan-id-749d8f17999243f5.yaml | 7 +++ 6 files changed, 128 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/ovn-update-vlan-id-749d8f17999243f5.yaml 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 e74d02a2bee..c03f383da66 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -188,6 +188,16 @@ class OVNMechanismDriver(api.MechanismDriver): def sb_ovn(self, val): self._sb_ovn = 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', '') + if dp_type == ovn_const.CHASSIS_DATAPATH_NETDEV: + vif_types.add(portbindings.VIF_TYPE_VHOST_USER) + else: + vif_types.add(portbindings.VIF_TYPE_OVS) + return list(vif_types) + def check_vlan_transparency(self, context): """OVN driver vlan transparency support.""" vlan_transparency_network_types = [ @@ -222,6 +232,10 @@ class OVNMechanismDriver(api.MechanismDriver): def supported_extensions(self, extensions): return set(ovn_extensions.ML2_SUPPORTED_API_EXTENSIONS) & extensions + @staticmethod + def provider_network_attribute_updates_supported(): + return [provider_net.SEGMENTATION_ID] + def subscribe(self): registry.subscribe(self.pre_fork_initialize, resources.PROCESS, 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 9d4c6ab7fe1..4cdb1dd2a9a 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 @@ -1965,6 +1965,15 @@ class OVNClient(object): if ovn_ls_azs != neutron_net_azs: self.sync_ha_chassis_group(context, network['id'], txn) + # Update the segment tags, if any + segments = segments_db.get_network_segments(context, network['id']) + for segment in segments: + tag = segment.get(segment_def.SEGMENTATION_ID) + tag = [] if tag is None else tag + lport_name = utils.ovn_provnet_port_name(segment['id']) + txn.add(self._nb_idl.set_lswitch_port(lport_name=lport_name, + tag=tag, if_exists=True)) + self._qos_driver.update_network(txn, network, original_network) if check_rev_cmd.result == ovn_const.TXN_COMMITTED: diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index aaa321756e8..c4ce9061b5a 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -949,16 +949,26 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, vif_types = [portbindings.VIF_TYPE_UNBOUND, portbindings.VIF_TYPE_BINDING_FAILED] for mech_driver in self.mechanism_manager.ordered_mech_drivers: - if (isinstance(mech_driver.obj, - mech_agent.AgentMechanismDriverBase) and - provider_net.SEGMENTATION_ID in mech_driver.obj. + if (provider_net.SEGMENTATION_ID in mech_driver.obj. provider_network_attribute_updates_supported()): - agent_type = mech_driver.obj.agent_type - agents = self.get_agents( - context, filters={'agent_type': [agent_type]}) - for agent in agents: - vif_types.append( - mech_driver.obj.get_supported_vif_type(agent)) + if isinstance(mech_driver.obj, + mech_agent.AgentMechanismDriverBase): + agent_type = mech_driver.obj.agent_type + agents = self.get_agents( + context, filters={'agent_type': [agent_type]}) + for agent in agents: + vif_types.append( + mech_driver.obj.get_supported_vif_type(agent)) + else: + # For non-AgentMechanismDriverBase drivers such as OVN + # which inherits from MechanismDriver + # TODO(lucasagomes): Perhaps we should + # include get_supported_vif_types() as part of the + # MechanismDriver's api in neutron-lib and remove this + # check in the future ? + if getattr(mech_driver.obj, 'get_supported_vif_types'): + vif_types.extend( + mech_driver.obj.get_supported_vif_types()) if ports_obj.Port.check_network_ports_by_binding_types( context, network['id'], vif_types, negative_search=True): 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 c4f965001a4..940e7cbc775 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 @@ -2644,6 +2644,40 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): self.assertFalse( self.mech_driver.responsible_for_ports_allocation(context)) + def test_update_network_segmentation_id(self): + new_vlan_tag = 123 + net_arg = {pnet.NETWORK_TYPE: 'vlan', + pnet.PHYSICAL_NETWORK: 'physnet1', + pnet.SEGMENTATION_ID: '1'} + net = self._make_network(self.fmt, 'net1', True, + arg_list=(pnet.NETWORK_TYPE, + pnet.PHYSICAL_NETWORK, + pnet.SEGMENTATION_ID,), + **net_arg)['network'] + # Make sure the network was created with 1 VLAN segment + segments = segments_db.get_network_segments(self.context, net['id']) + segment = segments[0] + self.assertEqual(len(segments), 1) + self.assertEqual(segment['segmentation_id'], 1) + + # Issue an update to the network changing the segmentation_id + data = {'network': {pnet.SEGMENTATION_ID: new_vlan_tag}} + req = self.new_update_request('networks', data, net['id']) + res = self.deserialize(self.fmt, req.get_response(self.api)) + self.assertEqual(new_vlan_tag, res['network'][pnet.SEGMENTATION_ID]) + + # Assert the tag was changed in the Neutron database + segments = segments_db.get_network_segments(self.context, net['id']) + segment = segments[0] + self.assertEqual(len(segments), 1) + self.assertEqual(segment['segmentation_id'], new_vlan_tag) + + # Assert the tag was changed in the OVN database + expected_call = mock.call( + lport_name=ovn_utils.ovn_provnet_port_name(segment['id']), + tag=new_vlan_tag, if_exists=True) + self.nb_ovn.set_lswitch_port.assert_has_calls([expected_call]) + class OVNMechanismDriverTestCase(MechDriverSetupBase, test_plugin.Ml2PluginV2TestCase): @@ -2989,6 +3023,10 @@ class TestOVNMechanismDriverSegment(MechDriverSetupBase, segment_id=self.seg_2['id']) as subnet: self.sub_2 = subnet + # TODO(lucasagomes): This test should use .assert_has_calls() + # to validate if the method was called with the correct values instead + # of peeking at call_args_list otherwise every time there's a new + # call to the mocked method the indexes will change def test_create_segments_subnet_metadata_ip_allocation(self): self._test_segments_helper() ovn_nb_api = self.mech_driver.nb_ovn @@ -2997,34 +3035,34 @@ class TestOVNMechanismDriverSegment(MechDriverSetupBase, # created subnet. self.assertIn( '10.0.1.2', - ovn_nb_api.set_lswitch_port.call_args_list[0][1]['addresses'][0]) + ovn_nb_api.set_lswitch_port.call_args_list[2][1]['addresses'][0]) # Assert that the second subnet address also has been allocated for # metadata port. self.assertIn( '10.0.2.2', - ovn_nb_api.set_lswitch_port.call_args_list[1][1]['addresses'][0]) + ovn_nb_api.set_lswitch_port.call_args_list[6][1]['addresses'][0]) # Assert also that the first subnet address is in this update self.assertIn( '10.0.1.2', - ovn_nb_api.set_lswitch_port.call_args_list[1][1]['addresses'][0]) + ovn_nb_api.set_lswitch_port.call_args_list[6][1]['addresses'][0]) self.assertEqual( - ovn_nb_api.set_lswitch_port.call_count, 2) + ovn_nb_api.set_lswitch_port.call_count, 7) # Make sure both updates where on same metadata port args_list = ovn_nb_api.set_lswitch_port.call_args_list self.assertEqual( 'ovnmeta-%s' % self.net['network']['id'], - args_list[1][1]['external_ids']['neutron:device_id']) + args_list[6][1]['external_ids']['neutron:device_id']) self.assertEqual( - args_list[1][1]['external_ids']['neutron:device_id'], - args_list[0][1]['external_ids']['neutron:device_id']) + args_list[6][1]['external_ids']['neutron:device_id'], + args_list[2][1]['external_ids']['neutron:device_id']) self.assertEqual( - args_list[1][1]['external_ids']['neutron:device_owner'], - args_list[0][1]['external_ids']['neutron:device_owner']) + args_list[6][1]['external_ids']['neutron:device_owner'], + args_list[2][1]['external_ids']['neutron:device_owner']) self.assertEqual( const.DEVICE_OWNER_DISTRIBUTED, - args_list[1][1]['external_ids']['neutron:device_owner']) + args_list[6][1]['external_ids']['neutron:device_owner']) def test_create_segments_mixed_allocation_prohibited(self): self._test_segments_helper() diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index b55291d5e10..36308843e10 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -533,6 +533,37 @@ class TestMl2NetworksV2(test_plugin.TestNetworksV2, portbindings.VIF_TYPE_BINDING_FAILED], negative_search=True) + @mock.patch.object(ml2_plugin, 'isinstance') + @mock.patch.object(port_obj.Port, 'check_network_ports_by_binding_types') + def test__update_segmentation_id_non_AgentMechanismDriverBase( + self, mock_check_network, mock_isinstance): + plugin = directory.get_plugin() + + mock_check_network.return_value = False + mock_isinstance.return_value = False + mock_update_net_segment = mock.patch.object( + plugin.type_manager, 'update_network_segment').start() + + fake_vif_types = ['fake-vif-type0', 'fake-vif-type1'] + fake_driver = mock.Mock() + fake_driver.obj.get_supported_vif_types.return_value = ( + fake_vif_types) + fake_driver.obj.provider_network_attribute_updates_supported.\ + return_value = {pnet.SEGMENTATION_ID: 1000} + plugin.mechanism_manager.ordered_mech_drivers = [fake_driver] + + with self.network() as net: + net_data = {pnet.SEGMENTATION_ID: 1000} + plugin._update_segmentation_id(self.context, net['network'], + net_data) + + # Assert the new method get_supported_vif_types() has been called + fake_driver.obj.get_supported_vif_types.assert_called_once_with() + + # Assert the update method has been called + mock_update_net_segment.assert_called_once_with( + self.context, net['network'], net_data, mock.ANY) + def test_update_network_with_empty_body(self): with self.network() as network: network_id = network["network"]["id"] diff --git a/releasenotes/notes/ovn-update-vlan-id-749d8f17999243f5.yaml b/releasenotes/notes/ovn-update-vlan-id-749d8f17999243f5.yaml new file mode 100644 index 00000000000..870e89e5ed0 --- /dev/null +++ b/releasenotes/notes/ovn-update-vlan-id-749d8f17999243f5.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes an issue in the ML2/OVN driver where the network + segment tag was not being updated in the OVN Northbound + database. For more information, see bug `1944708 + `_.