From 0de7a26cfadd7c5b0312acd2ff4d671bfaf1c661 Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Tue, 31 May 2022 12:46:47 +0100 Subject: [PATCH] Support 802.3ad port groups on Cumulus devices Since Ic3e10d19315b776662188f41c552fe0676a12782, multiple links in a port group are configured. This typically works for bond modes that do not require switch-side configuration, such as active/passive, TLB and ALB. In some cases this may also work for 802.3ad link aggregates, if local_link_connection.port_id in the ports is set to the name of the port group interface. However some switches require different commands to be used when configuring port groups vs switch port interfaces. For example, NVIDIA Cumulus switches require to use 'net add bond...' instead of 'net add interface ...'. This change adds support for devices that require different commands to configure port groups, and provides an implementation for NVIDIA Cumulus switches. Closes-Bug: #1976382 Related-Bug: #1759000 Change-Id: I0693c495170aa821a2f571038f387c50a2f6c599 --- networking_generic_switch/devices/__init__.py | 8 + .../devices/netmiko_devices/__init__.py | 49 ++++++ .../devices/netmiko_devices/cumulus.py | 16 ++ .../generic_switch_mech.py | 68 +++++--- .../tests/unit/netmiko/test_cumulus.py | 45 ++++++ .../tests/unit/test_devices.py | 12 ++ .../tests/unit/test_generic_switch_mech.py | 152 ++++++++++++++++++ .../cumulus-802.3ad-da9bffe131995f98.yaml | 4 + 8 files changed, 332 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/cumulus-802.3ad-da9bffe131995f98.yaml diff --git a/networking_generic_switch/devices/__init__.py b/networking_generic_switch/devices/__init__.py index 1feb79cb..7ad5b61e 100644 --- a/networking_generic_switch/devices/__init__.py +++ b/networking_generic_switch/devices/__init__.py @@ -151,3 +151,11 @@ def plug_port_to_network(self, port_id, segmentation_id): @abc.abstractmethod def delete_port(self, port_id, segmentation_id): pass + + def plug_bond_to_network(self, bond_id, segmentation_id): + # Fall back to interface method. + return self.plug_port_to_network(bond_id, segmentation_id) + + def unplug_bond_from_network(self, bond_id, segmentation_id): + # Fall back to interface method. + return self.delete_port(bond_id, segmentation_id) diff --git a/networking_generic_switch/devices/netmiko_devices/__init__.py b/networking_generic_switch/devices/netmiko_devices/__init__.py index 00843730..6620e973 100644 --- a/networking_generic_switch/devices/netmiko_devices/__init__.py +++ b/networking_generic_switch/devices/netmiko_devices/__init__.py @@ -71,6 +71,10 @@ class NetmikoSwitch(devices.GenericSwitchDevice): DELETE_PORT = None + PLUG_BOND_TO_NETWORK = None + + UNPLUG_BOND_FROM_NETWORK = None + ADD_NETWORK_TO_TRUNK = None REMOVE_NETWORK_FROM_TRUNK = None @@ -79,6 +83,10 @@ class NetmikoSwitch(devices.GenericSwitchDevice): DISABLE_PORT = None + ENABLE_BOND = None + + DISABLE_BOND = None + SAVE_CONFIGURATION = None ERROR_MSG_PATTERNS = () @@ -284,6 +292,47 @@ def delete_port(self, port, segmentation_id): cmds += self._format_commands(self.DISABLE_PORT, port=port) return self.send_commands_to_device(cmds) + @check_output('plug bond') + def plug_bond_to_network(self, bond, segmentation_id): + cmds = [] + if self._disable_inactive_ports() and self.ENABLE_BOND: + cmds += self._format_commands(self.ENABLE_BOND, bond=bond) + ngs_port_default_vlan = self._get_port_default_vlan() + if ngs_port_default_vlan: + cmds += self._format_commands( + self.UNPLUG_BOND_FROM_NETWORK, + bond=bond, + segmentation_id=ngs_port_default_vlan) + cmds += self._format_commands( + self.PLUG_BOND_TO_NETWORK, + bond=bond, + segmentation_id=segmentation_id) + return self.send_commands_to_device(cmds) + + @check_output('unplug bond') + def unplug_bond_from_network(self, bond, segmentation_id): + cmds = self._format_commands(self.UNPLUG_BOND_FROM_NETWORK, + bond=bond, + segmentation_id=segmentation_id) + ngs_port_default_vlan = self._get_port_default_vlan() + if ngs_port_default_vlan: + # NOTE(mgoddard): Pass network_id and segmentation_id for drivers + # not yet using network_name. + network_name = self._get_network_name(ngs_port_default_vlan, + ngs_port_default_vlan) + cmds += self._format_commands( + self.ADD_NETWORK, + segmentation_id=ngs_port_default_vlan, + network_id=ngs_port_default_vlan, + network_name=network_name) + cmds += self._format_commands( + self.PLUG_BOND_TO_NETWORK, + bond=bond, + segmentation_id=ngs_port_default_vlan) + if self._disable_inactive_ports() and self.DISABLE_BOND: + cmds += self._format_commands(self.DISABLE_BOND, bond=bond) + return self.send_commands_to_device(cmds) + def send_config_set(self, net_connect, cmd_set): """Send a set of configuration lines to the device. diff --git a/networking_generic_switch/devices/netmiko_devices/cumulus.py b/networking_generic_switch/devices/netmiko_devices/cumulus.py index f6a5b40f..9095348e 100644 --- a/networking_generic_switch/devices/netmiko_devices/cumulus.py +++ b/networking_generic_switch/devices/netmiko_devices/cumulus.py @@ -51,6 +51,14 @@ class Cumulus(netmiko_devices.NetmikoSwitch): 'net del interface {port} bridge access {segmentation_id}', ] + PLUG_BOND_TO_NETWORK = [ + 'net add bond {bond} bridge access {segmentation_id}', + ] + + UNPLUG_BOND_FROM_NETWORK = [ + 'net del bond {bond} bridge access {segmentation_id}', + ] + ENABLE_PORT = [ 'net del interface {port} link down', ] @@ -59,6 +67,14 @@ class Cumulus(netmiko_devices.NetmikoSwitch): 'net add interface {port} link down', ] + ENABLE_BOND = [ + 'net del bond {bond} link down', + ] + + DISABLE_BOND = [ + 'net add bond {bond} link down', + ] + SAVE_CONFIGURATION = [ 'net commit', ] diff --git a/networking_generic_switch/generic_switch_mech.py b/networking_generic_switch/generic_switch_mech.py index a5e153d9..43365aa0 100644 --- a/networking_generic_switch/generic_switch_mech.py +++ b/networking_generic_switch/generic_switch_mech.py @@ -442,28 +442,32 @@ def bind_port(self, context): # of the links should be processed. if not self._is_link_valid(port, network): return - else: - for link in local_link_information: - port_id = link.get('port_id') - switch_info = link.get('switch_info') - switch_id = link.get('switch_id') - switch = device_utils.get_switch_device( - self.switches, switch_info=switch_info, - ngs_mac_address=switch_id) - - segments = context.segments_to_bind - # If segmentation ID is None, set vlan 1 - segmentation_id = segments[0].get('segmentation_id') or 1 - LOG.debug("Putting port %(port_id)s on %(switch_info)s " - "to vlan: %(segmentation_id)s", - {'port_id': port_id, 'switch_info': switch_info, - 'segmentation_id': segmentation_id}) - # Move port to network + + is_802_3ad = self._is_802_3ad(port) + for link in local_link_information: + port_id = link.get('port_id') + switch_info = link.get('switch_info') + switch_id = link.get('switch_id') + switch = device_utils.get_switch_device( + self.switches, switch_info=switch_info, + ngs_mac_address=switch_id) + + segments = context.segments_to_bind + # If segmentation ID is None, set vlan 1 + segmentation_id = segments[0].get('segmentation_id') or 1 + LOG.debug("Putting port %(port_id)s on %(switch_info)s " + "to vlan: %(segmentation_id)s", + {'port_id': port_id, 'switch_info': switch_info, + 'segmentation_id': segmentation_id}) + # Move port to network + if is_802_3ad and hasattr(switch, 'plug_bond_to_network'): + switch.plug_bond_to_network(port_id, segmentation_id) + else: switch.plug_port_to_network(port_id, segmentation_id) - LOG.info("Successfully bound port %(port_id)s in segment " - "%(segment_id)s on device %(device)s", - {'port_id': port['id'], 'device': switch_info, - 'segment_id': segmentation_id}) + LOG.info("Successfully bound port %(port_id)s in segment " + "%(segment_id)s on device %(device)s", + {'port_id': port['id'], 'device': switch_info, + 'segment_id': segmentation_id}) context.set_binding(segments[0][api.ID], portbindings.VIF_TYPE_OTHER, {}) @@ -541,6 +545,21 @@ def _is_port_bound(port): vif_type = port[portbindings.VIF_TYPE] return vif_type == portbindings.VIF_TYPE_OTHER + @staticmethod + def _is_802_3ad(port): + """Return whether a port is using 802.3ad link aggregation. + + :param port: The port to check + :returns: Whether the port is a port group using 802.3ad link + aggregation. + """ + binding_profile = port['binding:profile'] + local_group_information = binding_profile.get( + 'local_group_information') + if not local_group_information: + return False + return local_group_information.get('bond_mode') in ['4', '802.3ad'] + def _unplug_port_from_network(self, port, network): """Unplug a port from a network. @@ -555,6 +574,8 @@ def _unplug_port_from_network(self, port, network): local_link_information = binding_profile.get('local_link_information') if not local_link_information: return + + is_802_3ad = self._is_802_3ad(port) for link in local_link_information: switch_info = link.get('switch_info') switch_id = link.get('switch_id') @@ -571,7 +592,10 @@ def _unplug_port_from_network(self, port, network): {'port': port_id, 'switch_info': switch_info, 'segmentation_id': segmentation_id}) try: - switch.delete_port(port_id, segmentation_id) + if is_802_3ad and hasattr(switch, 'unplug_bond_from_network'): + switch.unplug_bond_from_network(port_id, segmentation_id) + else: + switch.delete_port(port_id, segmentation_id) except Exception as e: LOG.error("Failed to unplug port %(port_id)s " "on device: %(switch)s from network %(net_id)s " diff --git a/networking_generic_switch/tests/unit/netmiko/test_cumulus.py b/networking_generic_switch/tests/unit/netmiko/test_cumulus.py index e783e83f..2ea84e0c 100644 --- a/networking_generic_switch/tests/unit/netmiko/test_cumulus.py +++ b/networking_generic_switch/tests/unit/netmiko/test_cumulus.py @@ -110,6 +110,51 @@ def test_delete_port_simple(self, mock_exec): mock_exec.assert_called_with( ['net del interface 3333 bridge access 33']) + @mock.patch('networking_generic_switch.devices.netmiko_devices.' + 'NetmikoSwitch.send_commands_to_device', + return_value="") + def test_plug_bond_to_network(self, mock_exec): + self.switch.plug_bond_to_network(3333, 33) + mock_exec.assert_called_with( + ['net del bond 3333 link down', + 'net del bond 3333 bridge access 123', + 'net add bond 3333 bridge access 33']) + + @mock.patch('networking_generic_switch.devices.netmiko_devices.' + 'NetmikoSwitch.send_commands_to_device', + return_value="") + def test_plug_bond_simple(self, mock_exec): + switch = self._make_switch_device({ + 'ngs_disable_inactive_ports': 'false', + 'ngs_port_default_vlan': '', + }) + switch.plug_bond_to_network(3333, 33) + mock_exec.assert_called_with( + ['net add bond 3333 bridge access 33']) + + @mock.patch('networking_generic_switch.devices.netmiko_devices.' + 'NetmikoSwitch.send_commands_to_device', + return_value="") + def test_unplug_bond_from_network(self, mock_exec): + self.switch.unplug_bond_from_network(3333, 33) + mock_exec.assert_called_with( + ['net del bond 3333 bridge access 33', + 'net add vlan 123', + 'net add bond 3333 bridge access 123', + 'net add bond 3333 link down']) + + @mock.patch('networking_generic_switch.devices.netmiko_devices.' + 'NetmikoSwitch.send_commands_to_device', + return_value="") + def test_unplug_bond_from_network_simple(self, mock_exec): + switch = self._make_switch_device({ + 'ngs_disable_inactive_ports': 'false', + 'ngs_port_default_vlan': '', + }) + switch.unplug_bond_from_network(3333, 33) + mock_exec.assert_called_with( + ['net del bond 3333 bridge access 33']) + def test_save(self): mock_connect = mock.MagicMock() mock_connect.save_config.side_effect = NotImplementedError diff --git a/networking_generic_switch/tests/unit/test_devices.py b/networking_generic_switch/tests/unit/test_devices.py index 57a22b72..1eb8d8d4 100644 --- a/networking_generic_switch/tests/unit/test_devices.py +++ b/networking_generic_switch/tests/unit/test_devices.py @@ -57,6 +57,18 @@ def test_fault_class(self): self.assertIn(m, ex.exception.args[0]) self.assertIsNone(device) + @mock.patch.object(FakeDevice, 'plug_port_to_network') + def test_plug_bond_to_network_fallback(self, mock_plug): + device = FakeDevice({'spam': 'ham'}) + device.plug_bond_to_network(22, 33) + mock_plug.assert_called_once_with(22, 33) + + @mock.patch.object(FakeDevice, 'delete_port') + def test_unplug_bond_from_network_fallback(self, mock_delete): + device = FakeDevice({'spam': 'ham'}) + device.unplug_bond_from_network(22, 33) + mock_delete.assert_called_once_with(22, 33) + class TestDeviceManager(unittest.TestCase): diff --git a/networking_generic_switch/tests/unit/test_generic_switch_mech.py b/networking_generic_switch/tests/unit/test_generic_switch_mech.py index 549b09f0..6c3356c9 100644 --- a/networking_generic_switch/tests/unit/test_generic_switch_mech.py +++ b/networking_generic_switch/tests/unit/test_generic_switch_mech.py @@ -280,6 +280,40 @@ def test_delete_portgroup_postcommit(self, m_list): [mock.call(2222, 123), mock.call(3333, 123)]) + def test_delete_portgroup_postcommit_802_3ad(self, m_list): + driver = gsm.GenericSwitchDriver() + driver.initialize() + mock_context = mock.create_autospec(driver_context.PortContext) + mock_context.current = {'binding:profile': + {'local_link_information': + [ + { + 'switch_info': 'foo', + 'port_id': 2222 + }, + { + 'switch_info': 'foo', + 'port_id': 3333 + }, + ], + 'local_group_information': + { + 'bond_mode': '4' + } + }, + 'binding:vnic_type': 'baremetal', + 'binding:vif_type': 'other', + 'id': 'aaaa-bbbb-cccc'} + mock_context.network = mock.Mock() + mock_context.network.current = {'provider:segmentation_id': 123, + 'id': 'aaaa-bbbb-cccc'} + mock_context.segments_to_bind = [mock_context.network.current] + + driver.delete_port_postcommit(mock_context) + self.switch_mock.unplug_bond_from_network.assert_has_calls( + [mock.call(2222, 123), + mock.call(3333, 123)]) + def test_delete_port_postcommit_failure(self, m_list): driver = gsm.GenericSwitchDriver() driver.initialize() @@ -513,6 +547,45 @@ def test_update_portgroup_postcommit_complete_provisioning(self, resources.PORT, 'GENERICSWITCH')]) + @mock.patch.object(provisioning_blocks, 'provisioning_complete') + def test_update_portgroup_postcommit_complete_provisioning_802_3ad(self, + m_pc, + m_list): + driver = gsm.GenericSwitchDriver() + driver.initialize() + mock_context = mock.create_autospec(driver_context.PortContext) + mock_context._plugin_context = mock.MagicMock() + mock_context.current = {'binding:profile': + {'local_link_information': + [ + { + 'switch_info': 'foo', + 'port_id': 2222 + }, + { + 'switch_info': 'foo', + 'port_id': 3333 + }, + ], + 'local_group_information': + { + 'bond_mode': '802.3ad' + } + }, + 'binding:vnic_type': 'baremetal', + 'id': '123', + 'binding:vif_type': 'other'} + mock_context.original = {'binding:profile': {}, + 'binding:vnic_type': 'baremetal', + 'id': '123', + 'binding:vif_type': 'unbound'} + driver.update_port_postcommit(mock_context) + self.switch_mock.plug_bond_to_network.assert_not_called() + m_pc.assert_has_calls([mock.call(mock_context._plugin_context, + mock_context.current['id'], + resources.PORT, + 'GENERICSWITCH')]) + @mock.patch.object(provisioning_blocks, 'provisioning_complete') def test_update_port_postcommit_unbind_not_bound(self, m_pc, m_list): driver = gsm.GenericSwitchDriver() @@ -754,6 +827,54 @@ def test_bind_portgroup(self, m_apc, m_list): resources.PORT, 'GENERICSWITCH')]) + @mock.patch.object(provisioning_blocks, 'add_provisioning_component') + def test_bind_portgroup_802_3ad(self, m_apc, m_list): + driver = gsm.GenericSwitchDriver() + driver.initialize() + mock_context = mock.create_autospec(driver_context.PortContext) + mock_context._plugin_context = mock.MagicMock() + mock_context.current = {'binding:profile': + {'local_link_information': + [ + { + 'switch_info': 'foo', + 'port_id': 2222 + }, + { + 'switch_info': 'foo', + 'port_id': 3333 + }, + ], + 'local_group_information': + { + 'bond_mode': '802.3ad' + } + }, + 'binding:vnic_type': 'baremetal', + 'id': '123'} + mock_context.network.current = { + 'provider:physical_network': 'physnet1' + } + mock_context.segments_to_bind = [ + { + 'segmentation_id': None, + 'id': 123 + } + ] + + driver.bind_port(mock_context) + self.switch_mock.plug_bond_to_network.assert_has_calls( + [mock.call(2222, 1), + mock.call(3333, 1)] + ) + mock_context.set_binding.assert_has_calls( + [mock.call(123, 'other', {})] + ) + m_apc.assert_has_calls([mock.call(mock_context._plugin_context, + mock_context.current['id'], + resources.PORT, + 'GENERICSWITCH')]) + @mock.patch.object(provisioning_blocks, 'add_provisioning_component') def test_bind_port_with_physnet(self, m_apc, m_list): driver = gsm.GenericSwitchDriver() @@ -878,3 +999,34 @@ def test_empty_methods(self, m_list): driver.create_port_postcommit(mock_context) driver.update_port_precommit(mock_context) driver.delete_port_precommit(mock_context) + + +class TestGenericSwitchDriverStaticMethods(unittest.TestCase): + + def test__is_802_3ad_no_lgi(self): + driver = gsm.GenericSwitchDriver() + port = {'binding:profile': {}} + self.assertFalse(driver._is_802_3ad(port)) + + def test__is_802_3ad_no_bond_mode(self): + driver = gsm.GenericSwitchDriver() + port = {'binding:profile': {'local_group_information': {}}} + self.assertFalse(driver._is_802_3ad(port)) + + def test__is_802_3ad_wrong_bond_mode(self): + driver = gsm.GenericSwitchDriver() + port = {'binding:profile': {'local_group_information': + {'bond_mode': 42}}} + self.assertFalse(driver._is_802_3ad(port)) + + def test__is_802_3ad_numeric(self): + driver = gsm.GenericSwitchDriver() + port = {'binding:profile': {'local_group_information': + {'bond_mode': '4'}}} + self.assertTrue(driver._is_802_3ad(port)) + + def test__is_802_3ad_string(self): + driver = gsm.GenericSwitchDriver() + port = {'binding:profile': {'local_group_information': + {'bond_mode': '802.3ad'}}} + self.assertTrue(driver._is_802_3ad(port)) diff --git a/releasenotes/notes/cumulus-802.3ad-da9bffe131995f98.yaml b/releasenotes/notes/cumulus-802.3ad-da9bffe131995f98.yaml new file mode 100644 index 00000000..facb726d --- /dev/null +++ b/releasenotes/notes/cumulus-802.3ad-da9bffe131995f98.yaml @@ -0,0 +1,4 @@ +--- +features: + - | + Adds support for 802.3ad port groups on NVIDIA Cumulus devices.