From 379a9faf6206039903555ce7e3fc4221e5f06a7a Mon Sep 17 00:00:00 2001 From: Arjun Baindur Date: Mon, 30 Jul 2018 15:31:50 -0700 Subject: [PATCH] Change duplicate OVS bridge datapath-ids The native OVS/ofctl controllers talk to the bridges using a datapath-id, instead of the bridge name. The datapath ID is auto-generated based on the MAC address of the bridge's NIC. In the case where bridges are on VLAN interfaces, they would have the same MACs, therefore the same datapath-id, causing flows for one physical bridge to be programmed on each other. The datapath-id is a 64-bit field, with lower 48 bits being the MAC. We set the upper 12 unused bits to identify each unique physical bridge This could also be fixed manually using ovs-vsctl set, but it might be beneficial to automate this in the code. ovs-vsctl set bridge other-config:datapath-id= You can change this yourself using above command. You can view/verify current datapath-id via ovs-vsctl get Bridge br-vlan datapath-id "00006ea5a4b38a4a" (please note that other-config is needed in the set, but not get) Closes-Bug: #1697243 Co-Authored-By: Rodolfo Alonso Hernandez Change-Id: I575ddf0a66e2cfe745af3874728809cf54e37745 --- neutron/agent/common/ovs_lib.py | 5 +++++ .../openvswitch/agent/ovs_neutron_agent.py | 22 +++++++++++++++++++ .../agent/test_ovs_neutron_agent.py | 21 +++++++++++++++++- .../openvswitch/agent/test_ovs_tunnel.py | 4 +++- 4 files changed, 50 insertions(+), 2 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index caccdb52183..a7d6d3a8101 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -921,6 +921,11 @@ class OVSBridge(BaseOVS): self.set_controller_field( 'controller_burst_limit', controller_burst_limit) + def set_datapath_id(self, datapath_id): + dpid_cfg = {'datapath-id': datapath_id} + self.set_db_attribute('Bridge', self.br_name, 'other_config', dpid_cfg, + check_error=True) + def __enter__(self): self.create() return self diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py index b72bc1eac9e..082be6bf29d 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -1143,6 +1143,25 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin, self.setup_physical_bridges(bridge_mappings) return sync + def _check_bridge_datapath_id(self, bridge, datapath_ids_set): + """Check for bridges with duplicate datapath-id + + Bottom 48 bits auto-derived from MAC of NIC. Upper 12 bits free, + so we OR it with (bridge # << 48) to create a unique ID + It must be exactly 64 bits, else OVS will reject it - zfill + + :param bridge: (OVSPhysicalBridge) bridge + :param datapath_ids_set: (set) used datapath ids in OVS + """ + dpid = int(bridge.get_datapath_id(), 16) + dpid_hex = format(dpid, '0x').zfill(16) + if dpid_hex in datapath_ids_set: + dpid_hex = format( + dpid + (len(datapath_ids_set) << 48), '0x').zfill(16) + bridge.set_datapath_id(dpid_hex) + LOG.info('Bridge %s datapath-id = 0x%s', bridge.br_name, dpid_hex) + datapath_ids_set.add(dpid_hex) + def setup_physical_bridges(self, bridge_mappings): '''Setup the physical network bridges. @@ -1154,6 +1173,7 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin, self.phys_brs = {} self.int_ofports = {} self.phys_ofports = {} + datapath_ids_set = set() ip_wrapper = ip_lib.IPWrapper() ovs = ovs_lib.BaseOVS() ovs_bridges = ovs.get_bridges() @@ -1171,6 +1191,8 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin, 'bridge': bridge}) sys.exit(1) br = self.br_phys_cls(bridge) + self._check_bridge_datapath_id(br, datapath_ids_set) + # The bridge already exists, so create won't recreate it, but will # handle things like changing the datapath_type br.create() diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index 8d899fad8b0..9262780f0a5 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -1319,6 +1319,7 @@ class TestOvsNeutronAgent(object): mock.patch.object(sys, "exit"),\ mock.patch.object(self.agent, 'br_phys_cls') as phys_br_cls,\ mock.patch.object(self.agent, 'int_br') as int_br,\ + mock.patch.object(self.agent, '_check_bridge_datapath_id'),\ mock.patch.object(ovs_lib.BaseOVS, 'get_bridges'): devex_fn.return_value = True parent = mock.MagicMock() @@ -1398,6 +1399,7 @@ class TestOvsNeutronAgent(object): mock.patch.object(utils, "execute") as utilsexec_fn,\ mock.patch.object(self.agent, 'br_phys_cls') as phys_br_cls,\ mock.patch.object(self.agent, 'int_br') as int_br,\ + mock.patch.object(self.agent, '_check_bridge_datapath_id'),\ mock.patch.object(ip_lib.IPWrapper, "add_veth") as addveth_fn,\ mock.patch.object(ip_lib.IpLinkCommand, "delete") as linkdel_fn,\ @@ -1436,7 +1438,8 @@ class TestOvsNeutronAgent(object): mock.patch.object(self.agent, 'br_phys_cls') as phys_br_cls,\ mock.patch.object(self.agent, 'int_br') as int_br,\ mock.patch.object(self.agent.int_br, 'db_get_val', - return_value='veth'),\ + return_value='veth'), \ + mock.patch.object(self.agent, '_check_bridge_datapath_id'), \ mock.patch.object(ovs_lib.BaseOVS, 'get_bridges'): phys_br = phys_br_cls() parent = mock.MagicMock() @@ -2342,6 +2345,22 @@ class TestOvsNeutronAgent(object): 'OVS') self.assertRaises(ValueError, self._make_agent) + def test__check_bridge_datapath_id(self): + datapath_id = u'0000622486fa3f42' + datapath_ids_set = set() + for i in range(5): + dpid = format((i << 48) + int(datapath_id, 16), '0x').zfill(16) + bridge = mock.Mock() + bridge.br_name = 'bridge_%s' % i + bridge.get_datapath_id = mock.Mock(return_value=datapath_id) + self.agent._check_bridge_datapath_id(bridge, datapath_ids_set) + self.assertEqual(i + 1, len(datapath_ids_set)) + self.assertIn(dpid, datapath_ids_set) + if i == 0: + bridge.set_datapath_id.assert_not_called() + else: + bridge.set_datapath_id.assert_called_once_with(dpid) + class TestOvsNeutronAgentOFCtl(TestOvsNeutronAgent, ovs_test_base.OVSOFCtlTestBase): diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py index 06d831f93eb..8d44e18e0c3 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py @@ -189,7 +189,9 @@ class TunnelTest(object): lambda bridge: bridge if bridge in self.ovs_bridges else None) self.execute = mock.patch('neutron.agent.common.utils.execute').start() - + self.mock_check_bridge_datapath_id = mock.patch.object( + self.mod_agent.OVSNeutronAgent, + '_check_bridge_datapath_id').start() self._define_expected_calls() def _define_expected_calls(self, arp_responder=False):