From 1c61066a473bf2ce470f92f7d29661c93ad291b9 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 21 May 2025 08:47:43 -0700 Subject: [PATCH 1/9] ci: workaround neutron's move to uwsgi only When neutron changed to using uwsgi only, it automatically excluded configuration parameters needed to supply configuration to launch ML2 plugins in the standard style of devstack, because the pattern only allowed for auto-configuration which only loads two files, or from a files in a single directory pattern, or a list of files supplied by environment variable. Ideally, we'd be able to restart neutron as well. We can't due to when things happen upstream; the post-config phase is not always late enough since neutron does some configuration there, the extra phase is too late as Ironic will be setting up nodes there. For now, we're relying on a race condition that we reliably win. Change-Id: I5cfd1fc247043ede9b71252ee681ce2ca413ede6 Signed-off-by: Jay Faulkner --- devstack/plugin.sh | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/devstack/plugin.sh b/devstack/plugin.sh index 24b25e97..85bd6bbf 100644 --- a/devstack/plugin.sh +++ b/devstack/plugin.sh @@ -164,8 +164,13 @@ function configure_generic_switch { done fi fi + # NOTE(TheJulia): This is not respected presently with uwsgi launched + # neutron as it auto-identifies it's configuration files. neutron_server_config_add $GENERIC_SWITCH_INI_FILE + if [ -f /etc/neutron/neutron-api-uwsgi.ini ]; then + iniset -sudo /etc/neutron/neutron-api-uwsgi.ini uwsgi env OS_NEUTRON_CONFIG_FILES='/etc/neutron/neutron.conf;/etc/neutron/plugins/ml2/ml2_conf.ini;/etc/neutron/plugins/ml2/ml2_conf_genericswitch.ini' + fi } function add_generic_switch_to_ml2_config { @@ -241,6 +246,26 @@ function ngs_configure_tempest { fi } +function ngs_restart_neutron { + echo_summary "NGS doing required neutron restart. Stopping neutron." + # NOTE(JayF) In practice restarting OVN causes problems, I'm not sure why. + # This avoids the restart. + local existing_skip_stop_ovn + SKIP_STOP_OVN=True + # We are changing the base config, and need ot restart the neutron services + stop_neutron + # NOTE(JayF): Neutron services are initialized in a particular order, this appears to + # match that order as currently defined in stack.sh (2025-05-22). + # TODO(JayF): Introduce a function in upstream devstack that documents this order so + # ironic won't break anytime initialization steps are rearranged. + echo_summary "NGS starting neutron service" + start_neutron_service_and_check + echo_summary "NGS started neutron service, now launch neutron agents" + start_neutron + echo_summary "NGS required neutron restart completed." + SKIP_STOP_OVN=False +} + # check for service enabled if is_service_enabled generic_switch; then @@ -250,7 +275,7 @@ if is_service_enabled generic_switch; then install_generic_switch elif [[ "$1" == "stack" && "$2" == "post-config" ]]; then - # Configure after the other layer 1 and 2 services have been configured + # Configure after the other layer 1 and 2 services have been started echo_summary "Configuring Generic_switch ML2" # Source ml2 plugin, set default config @@ -262,7 +287,22 @@ if is_service_enabled generic_switch; then Q_PLUGIN_CLASS="ml2" fi + # TODO(JayF): This currently relies on winning a race, as many of the + # files modified by this method are created during this + # phase. In practice it works, but moving forward we likely + # need a supported-by-devstack/neutron-upstream method to + # ensure this is done at the right moment. configure_generic_switch + + if is_service_enabled neutron; then + # TODO(JayF): Similarly, we'd like to restart neutron to ensure + # our config changes have taken effect; we can't do + # that reliably here because it may not be fully + # configured, and extra phase is too late. + echo_summary "Skipping ngs_restart_neutron" + #ngs_restart_neutron + fi + elif [[ "$1" == "stack" && "$2" == "test-config" ]]; then if is_service_enabled tempest; then echo_summary "Configuring Tempest NGS" From c77674475b89f549210ab19e3b8a5499e2533629 Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Tue, 27 May 2025 12:55:59 -0700 Subject: [PATCH 2/9] Make devstack runs slightly more deterministic This eliminates one of the two implied ordering things we're relying on. We should now be OK if networking_generic_switch is configured before neutron. We are still relying on implied behavior of neutron services restarting sometime after our work in the post-config phase. Change-Id: I5a763b74ea349a74c66feca38ded46cda91d82a4 --- devstack/plugin.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/devstack/plugin.sh b/devstack/plugin.sh index 85bd6bbf..226c48cd 100644 --- a/devstack/plugin.sh +++ b/devstack/plugin.sh @@ -168,9 +168,10 @@ function configure_generic_switch { # neutron as it auto-identifies it's configuration files. neutron_server_config_add $GENERIC_SWITCH_INI_FILE - if [ -f /etc/neutron/neutron-api-uwsgi.ini ]; then - iniset -sudo /etc/neutron/neutron-api-uwsgi.ini uwsgi env OS_NEUTRON_CONFIG_FILES='/etc/neutron/neutron.conf;/etc/neutron/plugins/ml2/ml2_conf.ini;/etc/neutron/plugins/ml2/ml2_conf_genericswitch.ini' - fi + # NOTE(JayF): It's possible in some rare cases this config doesn't exist + # if so, no big deal, iniset is used in devstack and should + # not lose our changes. See `write_uwsgi_config` in lib/apache + iniset -sudo /etc/neutron/neutron-api-uwsgi.ini uwsgi env OS_NEUTRON_CONFIG_FILES='/etc/neutron/neutron.conf;/etc/neutron/plugins/ml2/ml2_conf.ini;/etc/neutron/plugins/ml2/ml2_conf_genericswitch.ini' } function add_generic_switch_to_ml2_config { From ae002a3839619159665557c1d71d992611b50446 Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Tue, 27 May 2025 12:45:31 -0700 Subject: [PATCH 3/9] Remove explicit use of eventlet This removes explicit eventlet usages in NGS, enabling us to be compatible when neutron is done with their migration. Generated-By: claude-code Change-Id: I4ec319a19e8c27689babeef023eddb4f6e1dd918 --- networking_generic_switch/batching.py | 20 ++++++++++---------- requirements.txt | 2 +- tools/ngs-stress/ngs_stress.py | 3 --- tox.ini | 1 - 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/networking_generic_switch/batching.py b/networking_generic_switch/batching.py index 5ec3c8f2..f4d2dc10 100644 --- a/networking_generic_switch/batching.py +++ b/networking_generic_switch/batching.py @@ -14,14 +14,15 @@ import atexit import json +import threading import etcd3gw from etcd3gw import exceptions as etcd3gw_exc from etcd3gw.utils import _decode from etcd3gw.utils import _encode from etcd3gw.utils import _increment_last_byte -import eventlet from oslo_log import log as logging +from oslo_service import threadgroup from oslo_utils import netutils from oslo_utils import uuidutils import tenacity @@ -32,7 +33,7 @@ LOG = logging.getLogger(__name__) -THREAD_POOL = eventlet.greenpool.GreenPool() +THREAD_POOL = threadgroup.ThreadGroup() class ShutdownTimeout(Exception): @@ -48,12 +49,12 @@ def _wait_for_threads(): and performing switch configuration operations which should not be interrupted. """ + active_threads = len(THREAD_POOL.threads) LOG.info("Waiting %d seconds for %d threads to complete", - SHUTDOWN_TIMEOUT, THREAD_POOL.running()) + SHUTDOWN_TIMEOUT, active_threads) try: - with eventlet.Timeout(SHUTDOWN_TIMEOUT, ShutdownTimeout): - THREAD_POOL.waitall() - except ShutdownTimeout: + THREAD_POOL.stop(graceful=True, timeout=SHUTDOWN_TIMEOUT) + except Exception: LOG.error("Timed out waiting for threads to complete") else: LOG.info("Finished waiting for threads to complete") @@ -365,13 +366,12 @@ def do_work(): @staticmethod def _spawn(work_fn): - # TODO(johngarbutt) remove hard eventlet dependency - # in a similar way to etcd3gw # Sleep to let possible other work to batch together - eventlet.sleep(0) + # This works with both eventlet and native threading + threading.Event().wait(0.001) # Run all pending tasks, which might be a no op # if pending tasks already ran - THREAD_POOL.spawn_n(work_fn) + THREAD_POOL.add_thread(work_fn) def _execute_pending_batches(self, device, item): """Execute all batches currently registered. diff --git a/requirements.txt b/requirements.txt index 3d8dc11f..913556ec 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,7 +2,7 @@ # date but we do not test them so no guarantee of having them all correct. If # you find any incorrect lower bounds, let us know or propose a fix. etcd3gw>=2.1.0 # Apache-2.0 -eventlet>=0.18.2 # Apache-2.0 +oslo.service[threading]>=4.2.0 # Apache-2.0 stevedore>=1.20.0 # Apache-2.0 netmiko>=4.1.1 # MIT neutron>=13.0.0.0b1 # Apache-2.0 diff --git a/tools/ngs-stress/ngs_stress.py b/tools/ngs-stress/ngs_stress.py index 95af461e..85d4d933 100644 --- a/tools/ngs-stress/ngs_stress.py +++ b/tools/ngs-stress/ngs_stress.py @@ -18,15 +18,12 @@ import threading import uuid -import eventlet from neutron_lib.utils import net from oslo_config import cfg import oslo_log.log as logging import networking_generic_switch.generic_switch_mech as generic_switch -eventlet.monkey_patch() - CONF = cfg.CONF LOG = logging.getLogger(__name__) diff --git a/tox.ini b/tox.ini index 0b46a5af..24e913b9 100644 --- a/tox.ini +++ b/tox.ini @@ -4,7 +4,6 @@ envlist = py3,pep8 ignore_basepython_conflict=true [testenv] -constrain_package_deps = true usedevelop = True setenv = VIRTUAL_ENV={envdir} PYTHONDONTWRITEBYTECODE = 1 From 1b58a37bc735893023002c4898caf8e8e1cbf9cc Mon Sep 17 00:00:00 2001 From: mumesan Date: Tue, 27 May 2025 14:20:28 +0100 Subject: [PATCH 4/9] Improve NGS documentation. Some minor nitpicks and changes Change-Id: I593a0c44b4ad3638875bc6959a3aea8feebdba0e --- doc/source/dev/dev-quickstart.rst | 121 ++++++++------- networking_generic_switch/devices/__init__.py | 8 +- .../devices/netmiko_devices/cumulus.py | 141 +++++++++--------- .../devices/netmiko_devices/smc.py | 4 +- .../devices/netmiko_devices/sonic.py | 54 +++---- 5 files changed, 173 insertions(+), 155 deletions(-) diff --git a/doc/source/dev/dev-quickstart.rst b/doc/source/dev/dev-quickstart.rst index 368a4f50..bdd4ef47 100644 --- a/doc/source/dev/dev-quickstart.rst +++ b/doc/source/dev/dev-quickstart.rst @@ -34,33 +34,36 @@ Switch to the stack user and clone DevStack:: git clone https://github.com/openstack-dev/devstack.git devstack Create devstack/local.conf with minimal settings required to enable -Networking-generic-switch. Here is and example of local.conf:: - - [[local|localrc]] - # Set credentials - ADMIN_PASSWORD=secrete - DATABASE_PASSWORD=secrete - RABBIT_PASSWORD=secrete - SERVICE_PASSWORD=secrete - SERVICE_TOKEN=secrete - - # Enable minimal required services - ENABLED_SERVICES="dstat,mysql,rabbit,key,q-svc,q-agt,q-dhcp" - - # Enable networking-generic-switch plugin - enable_plugin networking-generic-switch https://review.openstack.org/openstack/networking-generic-switch - - # Configure Neutron - OVS_PHYSICAL_BRIDGE=brbm - PHYSICAL_NETWORK=mynetwork - Q_PLUGIN=ml2 - ENABLE_TENANT_VLANS=True - Q_ML2_TENANT_NETWORK_TYPE=vlan - TENANT_VLAN_RANGE=100:150 - - # Configure logging - LOGFILE=$HOME/devstack.log - LOGDIR=$HOME/logs +Networking-generic-switch. Here is an example of local.conf: + + + .. code-block:: ini + + [[local|localrc]] + # Set credentials + ADMIN_PASSWORD=secrete + DATABASE_PASSWORD=secrete + RABBIT_PASSWORD=secrete + SERVICE_PASSWORD=secrete + SERVICE_TOKEN=secrete + + # Enable minimal required services + ENABLED_SERVICES="dstat,mysql,rabbit,key,q-svc,q-agt,q-dhcp" + + # Enable networking-generic-switch plugin + enable_plugin networking-generic-switch https://review.openstack.org/openstack/networking-generic-switch + + # Configure Neutron + OVS_PHYSICAL_BRIDGE=brbm + PHYSICAL_NETWORK=mynetwork + Q_PLUGIN=ml2 + ENABLE_TENANT_VLANS=True + Q_ML2_TENANT_NETWORK_TYPE=vlan + TENANT_VLAN_RANGE=100:150 + + # Configure logging + LOGFILE=$HOME/devstack.log + LOGDIR=$HOME/logs Run stack.sh:: @@ -86,45 +89,53 @@ Test with real hardware Add information about hardware switch to Networking-generic-switch config ``/etc/neutron/plugins/ml2/ml2_conf_genericswitch.ini`` and -restart Neutron server:: +restart Neutron server: + + .. code-block:: ini - [genericswitch:cisco_switch_1] - device_type = netmiko_cisco_ios - ip = 1.2.3.4 - username = cisco - password = cisco - secret = enable_password + [genericswitch:cisco_switch_1] + device_type = netmiko_cisco_ios + ip = 1.2.3.4 + username = cisco + password = cisco + secret = enable_password Get current configuration of the port on the switch, for example for -Cisco IOS device:: +Cisco IOS device: + + .. code-block:: ini - sh running-config int gig 0/12 - Building configuration... + sh running-config int gig 0/12 + Building configuration... - Current configuration : 283 bytes - ! - interface GigabitEthernet0/12 - switchport mode access - end + Current configuration : 283 bytes + ! + interface GigabitEthernet0/12 + switchport mode access + end Run exercise.py to create/update Neutron port. It will print VLAN id to be -assigned:: +assigned: - $ neutron net-create test - $ python ~/networking-generic-switch/devstack/exercise.py --switch_name cisco_switch_1 --port Gig0/12 --switch_id=06:58:1f:e7:b4:44 --network test - 126 + .. code-block:: ini + + $ neutron net-create test + $ python ~/networking-generic-switch/devstack/exercise.py --switch_name cisco_switch_1 --port Gig0/12 --switch_id=06:58:1f:e7:b4:44 --network test + 126 Verify that VLAN has been changed on the switch port, for example for -Cisco IOS device:: +Cisco IOS device: + + .. code-block:: ini - sh running-config int gig 0/12 - Building configuration... + sh running-config int gig 0/12 + Building configuration... - Current configuration : 311 bytes - ! - interface GigabitEthernet0/12 - switchport access vlan 126 - switchport mode access - end + Current configuration : 311 bytes + ! + interface GigabitEthernet0/12 + switchport access vlan 126 + switchport mode access + end diff --git a/networking_generic_switch/devices/__init__.py b/networking_generic_switch/devices/__init__.py index 0b6f2cfb..68209cdf 100644 --- a/networking_generic_switch/devices/__init__.py +++ b/networking_generic_switch/devices/__init__.py @@ -235,7 +235,7 @@ def plug_port_to_network(self, port_id, segmentation_id, trunk_details=None): """Plug port into network. - :param port_id: Then name of the switch interface + :param port_id: The name of the switch interface :param segmentation_id: VLAN identifier of the network used as access or native VLAN for port. @@ -247,7 +247,7 @@ def plug_port_to_network(self, port_id, segmentation_id, def delete_port(self, port_id, segmentation_id, trunk_details=None): """Delete port from specific network. - :param port_id: Then name of the switch interface + :param port_id: The name of the switch interface :param segmentation_id: VLAN identifier of the network used as access or native VLAN for port. @@ -259,7 +259,7 @@ def plug_bond_to_network(self, bond_id, segmentation_id, trunk_details=None): """Plug bond port into network. - :param port_id: Then name of the switch interface + :param port_id: The name of the switch interface :param segmentation_id: VLAN identifier of the network used as access or native VLAN for port. @@ -275,7 +275,7 @@ def unplug_bond_from_network(self, bond_id, segmentation_id, trunk_details=None): """Unplug bond port from network. - :param port_id: Then name of the switch interface + :param port_id: The name of the switch interface :param segmentation_id: VLAN identifier of the network used as access or native VLAN for port. diff --git a/networking_generic_switch/devices/netmiko_devices/cumulus.py b/networking_generic_switch/devices/netmiko_devices/cumulus.py index ce965d2a..895d02df 100644 --- a/networking_generic_switch/devices/netmiko_devices/cumulus.py +++ b/networking_generic_switch/devices/netmiko_devices/cumulus.py @@ -22,71 +22,73 @@ class Cumulus(netmiko_devices.NetmikoSwitch): Note for this switch you want config like this, where secret is the password needed for sudo su: - [genericswitch:] - device_type = netmiko_cumulus - ip = - username = - password = - secret = - ngs_physical_networks = physnet1 - ngs_max_connections = 1 - ngs_port_default_vlan = 123 - ngs_disable_inactive_ports = False + .. code-block:: ini + + [genericswitch:] + device_type = netmiko_cumulus + ip = + username = + password = + secret = + ngs_physical_networks = physnet1 + ngs_max_connections = 1 + ngs_port_default_vlan = 123 + ngs_disable_inactive_ports = False """ NETMIKO_DEVICE_TYPE = "linux" - ADD_NETWORK = [ + ADD_NETWORK = ( 'net add vlan {segmentation_id}', - ] + ) - DELETE_NETWORK = [ + DELETE_NETWORK = ( 'net del vlan {segmentation_id}', - ] + ) - PLUG_PORT_TO_NETWORK = [ + PLUG_PORT_TO_NETWORK = ( 'net add interface {port} bridge access {segmentation_id}', - ] + ) - DELETE_PORT = [ + DELETE_PORT = ( 'net del interface {port} bridge access {segmentation_id}', - ] + ) - PLUG_BOND_TO_NETWORK = [ + PLUG_BOND_TO_NETWORK = ( 'net add bond {bond} bridge access {segmentation_id}', - ] + ) - UNPLUG_BOND_FROM_NETWORK = [ + UNPLUG_BOND_FROM_NETWORK = ( 'net del bond {bond} bridge access {segmentation_id}', - ] + ) - ENABLE_PORT = [ + ENABLE_PORT = ( 'net del interface {port} link down', - ] + ) - DISABLE_PORT = [ + DISABLE_PORT = ( 'net add interface {port} link down', - ] + ) - ENABLE_BOND = [ + ENABLE_BOND = ( 'net del bond {bond} link down', - ] + ) - DISABLE_BOND = [ + DISABLE_BOND = ( 'net add bond {bond} link down', - ] + ) - SAVE_CONFIGURATION = [ + SAVE_CONFIGURATION = ( 'net commit', - ] + ) - ERROR_MSG_PATTERNS = [ + ERROR_MSG_PATTERNS = ( # Its tempting to add this error message, but as only one # bridge-access is allowed, we ignore that error for now: # re.compile(r'configuration does not have "bridge-access') re.compile(r'ERROR: Command not found.'), re.compile(r'command not found'), re.compile(r'is not a physical interface on this switch'), - ] + ) class CumulusNVUE(netmiko_devices.NetmikoSwitch): @@ -95,44 +97,47 @@ class CumulusNVUE(netmiko_devices.NetmikoSwitch): Note for this switch you want config like this, where secret is the password needed for sudo su: - [genericswitch:] - device_type = netmiko_cumulus_nvue - ip = - username = - password = - secret = - ngs_physical_networks = physnet1 - ngs_max_connections = 1 - ngs_port_default_vlan = 123 - ngs_disable_inactive_ports = False + .. code-block:: ini + + [genericswitch:] + device_type = netmiko_cumulus_nvue + ip = + username = + password = + secret = + ngs_physical_networks = physnet1 + ngs_max_connections = 1 + ngs_port_default_vlan = 123 + ngs_disable_inactive_ports = False + """ NETMIKO_DEVICE_TYPE = "linux" - ADD_NETWORK = [ + ADD_NETWORK = ( 'nv set bridge domain br_default vlan {segmentation_id}', - ] + ) - DELETE_NETWORK = [ + DELETE_NETWORK = ( 'nv unset bridge domain br_default vlan {segmentation_id}', - ] + ) - PLUG_PORT_TO_NETWORK = [ + PLUG_PORT_TO_NETWORK = ( 'nv unset interface {port} bridge domain br_default untagged', 'nv set interface {port} bridge domain br_default access ' '{segmentation_id}', - ] + ) - ADD_NETWORK_TO_TRUNK = [ + ADD_NETWORK_TO_TRUNK = ( 'nv unset interface {port} bridge domain br_default access', 'nv set interface {port} bridge domain br_default vlan ' '{segmentation_id}', - ] + ) - ADD_NETWORK_TO_BOND_TRUNK = [ + ADD_NETWORK_TO_BOND_TRUNK = ( 'nv unset interface {bond} bridge domain br_default access', 'nv set interface {bond} bridge domain br_default vlan ' '{segmentation_id}', - ] + ) REMOVE_NETWORK_FROM_TRUNK = ( 'nv unset interface {port} bridge domain br_default vlan ' @@ -144,21 +149,21 @@ class CumulusNVUE(netmiko_devices.NetmikoSwitch): '{segmentation_id}', ) - SET_NATIVE_VLAN = [ + SET_NATIVE_VLAN = ( 'nv unset interface {port} bridge domain br_default access', 'nv set interface {port} bridge domain br_default untagged ' '{segmentation_id}', 'nv set interface {port} bridge domain br_default vlan ' '{segmentation_id}', - ] + ) - SET_NATIVE_VLAN_BOND = [ + SET_NATIVE_VLAN_BOND = ( 'nv unset interface {bond} bridge domain br_default access', 'nv set interface {bond} bridge domain br_default untagged ' '{segmentation_id}', 'nv set interface {bond} bridge domain br_default vlan ' '{segmentation_id}', - ] + ) DELETE_NATIVE_VLAN = ( 'nv unset interface {port} bridge domain br_default untagged ' @@ -174,25 +179,25 @@ class CumulusNVUE(netmiko_devices.NetmikoSwitch): '{segmentation_id}', ) - DELETE_PORT = [ + DELETE_PORT = ( 'nv unset interface {port} bridge domain br_default access', 'nv unset interface {port} bridge domain br_default untagged', 'nv unset interface {port} bridge domain br_default vlan', - ] + ) - ENABLE_PORT = [ + ENABLE_PORT = ( 'nv set interface {port} link state up', - ] + ) - DISABLE_PORT = [ + DISABLE_PORT = ( 'nv set interface {port} link state down', - ] + ) - SAVE_CONFIGURATION = [ + SAVE_CONFIGURATION = ( 'nv config save', - ] + ) - ERROR_MSG_PATTERNS = [ + ERROR_MSG_PATTERNS = ( # Its tempting to add this error message, but as only one # bridge-access is allowed, we ignore that error for now: # re.compile(r'configuration does not have "bridge-access') @@ -204,7 +209,7 @@ class CumulusNVUE(netmiko_devices.NetmikoSwitch): re.compile(r'Error: Invalid parameter'), re.compile(r'Unable to restart services'), re.compile(r'Failure during apply'), - ] + ) 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/smc.py b/networking_generic_switch/devices/netmiko_devices/smc.py index 12365082..e0174a7d 100644 --- a/networking_generic_switch/devices/netmiko_devices/smc.py +++ b/networking_generic_switch/devices/netmiko_devices/smc.py @@ -16,8 +16,8 @@ class SupermicroSmis(netmiko_devices.NetmikoSwitch): - """A class to represent a Supermicro SMIS switch""" - """ + """A class to represent a Supermicro SMIS switch + Inherits from: -------------- netmiko_devices.NetmikoSwitch diff --git a/networking_generic_switch/devices/netmiko_devices/sonic.py b/networking_generic_switch/devices/netmiko_devices/sonic.py index 7a78c80a..96f02565 100644 --- a/networking_generic_switch/devices/netmiko_devices/sonic.py +++ b/networking_generic_switch/devices/netmiko_devices/sonic.py @@ -22,53 +22,55 @@ class Sonic(netmiko_devices.NetmikoSwitch): Note for this switch you want config like this, where secret is the password needed for sudo su: - [genericswitch:] - device_type = netmiko_sonic - ip = - username = - password = - secret = - ngs_physical_networks = physnet1 - ngs_max_connections = 1 - ngs_port_default_vlan = 123 - ngs_disable_inactive_ports = False + .. code-block:: ini + + [genericswitch:] + device_type = netmiko_sonic + ip = + username = + password = + secret = + ngs_physical_networks = physnet1 + ngs_max_connections = 1 + ngs_port_default_vlan = 123 + ngs_disable_inactive_ports = False """ NETMIKO_DEVICE_TYPE = "linux" - ADD_NETWORK = [ + ADD_NETWORK = ( 'config vlan add {segmentation_id}', - ] + ) - DELETE_NETWORK = [ + DELETE_NETWORK = ( 'config vlan del {segmentation_id}', - ] + ) - PLUG_PORT_TO_NETWORK = [ + PLUG_PORT_TO_NETWORK = ( 'config vlan member add -u {segmentation_id} {port}', - ] + ) - DELETE_PORT = [ + DELETE_PORT = ( 'config vlan member del {segmentation_id} {port}', - ] + ) - ADD_NETWORK_TO_TRUNK = [ + ADD_NETWORK_TO_TRUNK = ( 'config vlan member add {segmentation_id} {port}', - ] + ) - REMOVE_NETWORK_FROM_TRUNK = [ + REMOVE_NETWORK_FROM_TRUNK = ( 'config vlan member del {segmentation_id} {port}', - ] + ) - SAVE_CONFIGURATION = [ + SAVE_CONFIGURATION = ( 'config save -y', - ] + ) - ERROR_MSG_PATTERNS = [ + ERROR_MSG_PATTERNS = ( re.compile(r'VLAN[0-9]+ doesn\'t exist'), re.compile(r'Invalid Vlan Id , Valid Range : 1 to 4094'), re.compile(r'Interface name is invalid!!'), re.compile(r'No such command'), - ] + ) def send_config_set(self, net_connect, cmd_set): """Send a set of configuration lines to the device. From 4cc309013139399ff8dda3ddf558b5660ec85b88 Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Thu, 8 May 2025 11:08:01 +1200 Subject: [PATCH 5/9] Store driver objects in a global dict When security group support is added, device objects need to be shared between the existing mechanism driver and also a service plugin. This change invokes drivers and stores them in a single global DEVICES dict. Change-Id: I5cd5880cded950cd5f3cfff6c77750f44e92618e Related-Bug: #2110760 --- networking_generic_switch/devices/__init__.py | 16 ++++++++++++++++ networking_generic_switch/generic_switch_mech.py | 7 +------ .../tests/unit/test_generic_switch_mech.py | 2 ++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/networking_generic_switch/devices/__init__.py b/networking_generic_switch/devices/__init__.py index 68209cdf..a2273605 100644 --- a/networking_generic_switch/devices/__init__.py +++ b/networking_generic_switch/devices/__init__.py @@ -15,10 +15,12 @@ import abc from neutron_lib.utils.helpers import parse_mappings +from oslo_concurrency import lockutils from oslo_log import log as logging from oslo_utils import strutils import stevedore +from networking_generic_switch import config as gsw_conf from networking_generic_switch import exceptions as gsw_exc GENERIC_SWITCH_NAMESPACE = 'generic_switch.devices' @@ -60,6 +62,20 @@ {'name': 'ngs_allowed_ports'}, ] +EM_SEMAPHORE = 'ngs_device_manager' +DEVICES = {} + + +@lockutils.synchronized(EM_SEMAPHORE) +def get_devices(): + global DEVICES + gsw_devices = gsw_conf.get_devices() + for device_name, device_cfg in gsw_devices.items(): + if device_name in DEVICES: + continue + DEVICES[device_name] = device_manager(device_cfg, device_name) + return DEVICES + def device_manager(device_cfg, device_name=""): device_type = device_cfg.get('device_type', '') diff --git a/networking_generic_switch/generic_switch_mech.py b/networking_generic_switch/generic_switch_mech.py index 13621c21..cc8f05cf 100644 --- a/networking_generic_switch/generic_switch_mech.py +++ b/networking_generic_switch/generic_switch_mech.py @@ -22,7 +22,6 @@ from neutron_lib.plugins.ml2 import api from oslo_log import log as logging -from networking_generic_switch import config as gsw_conf from networking_generic_switch import devices from networking_generic_switch.devices import utils as device_utils from networking_generic_switch import exceptions as ngs_exc @@ -51,11 +50,7 @@ def initialize(self): self.vif_details = {portbindings.VIF_DETAILS_CONNECTIVITY: portbindings.CONNECTIVITY_L2} - gsw_devices = gsw_conf.get_devices() - self.switches = {} - for switch_info, device_cfg in gsw_devices.items(): - switch = devices.device_manager(device_cfg, switch_info) - self.switches[switch_info] = switch + self.switches = devices.get_devices() LOG.info('Devices %s have been loaded', self.switches.keys()) if not self.switches: 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 2f946b0c..cbf3ba43 100644 --- a/networking_generic_switch/tests/unit/test_generic_switch_mech.py +++ b/networking_generic_switch/tests/unit/test_generic_switch_mech.py @@ -21,6 +21,7 @@ from neutron_lib.callbacks import resources from neutron_lib.plugins import directory +from networking_generic_switch import devices from networking_generic_switch.devices import utils as device_utils from networking_generic_switch import exceptions from networking_generic_switch import generic_switch_mech as gsm @@ -32,6 +33,7 @@ class TestGenericSwitchDriver(unittest.TestCase): def setUp(self): super(TestGenericSwitchDriver, self).setUp() + devices.DEVICES.clear() self.switch_mock = mock.Mock() self.switch_mock.config = {'device_type': 'bar', 'spam': 'ham', 'ip': 'ip'} From e5bb406e6b6787f7cab4679e9f1b280506e09a87 Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Thu, 8 May 2025 15:54:37 +1200 Subject: [PATCH 6/9] Move _is_port_supported, _is_port_bound to utils These functions are useful for future neutron plugins which are not the mechanism driver, so they are moved to utils. Change-Id: I3a87a96f7fe220a05b584f6ef261edec61ed4da2 Related-Bug: #2110760 --- .../generic_switch_mech.py | 39 +++---------------- .../tests/unit/test_generic_switch_mech.py | 23 +++++++---- networking_generic_switch/utils.py | 30 ++++++++++++++ 3 files changed, 51 insertions(+), 41 deletions(-) diff --git a/networking_generic_switch/generic_switch_mech.py b/networking_generic_switch/generic_switch_mech.py index cc8f05cf..b7fed190 100644 --- a/networking_generic_switch/generic_switch_mech.py +++ b/networking_generic_switch/generic_switch_mech.py @@ -347,7 +347,7 @@ def update_port_postcommit(self, context): """ port = context.current segment = context.top_bound_segment - if self._is_port_bound(port): + if ngs_utils.is_port_bound(port): binding_profile = port['binding:profile'] local_link_information = binding_profile.get( 'local_link_information') @@ -416,7 +416,7 @@ def update_port_postcommit(self, context): context._plugin.update_port_status( context.plugin_context, subport["port_id"], const.PORT_STATUS_ACTIVE) - elif self._is_port_bound(context.original): + elif ngs_utils.is_port_bound(context.original): # The port has been unbound. This will cause the local link # information to be lost, so remove the port from the segment on # the switch now while we have the required information. @@ -449,7 +449,7 @@ def delete_port_postcommit(self, context): """ port = context.current - if self._is_port_bound(port): + if ngs_utils.is_port_bound(port): self._unplug_port_from_segment(port, context.top_bound_segment) def bind_port(self, context): @@ -502,7 +502,7 @@ def bind_port(self, context): binding_profile = port['binding:profile'] local_link_information = binding_profile.get('local_link_information') - if self._is_port_supported(port) and local_link_information: + if ngs_utils.is_port_supported(port) and local_link_information: # Filter segments where port is already assigned to subnet(s) subnets = [] for fixed_ip in port.get('fixed_ips', []): @@ -589,33 +589,6 @@ def _is_link_valid(self, port, segment): return False return True - @staticmethod - def _is_port_supported(port): - """Return whether a port is supported by this driver. - - Ports supported by this driver have a VNIC type of 'baremetal'. - - :param port: The port to check - :returns: Whether the port is supported by the NGS driver - """ - vnic_type = port[portbindings.VNIC_TYPE] - return vnic_type == portbindings.VNIC_BAREMETAL - - @staticmethod - def _is_port_bound(port): - """Return whether a port is bound by this driver. - - Ports bound by this driver have their VIF type set to 'other'. - - :param port: The port to check - :returns: Whether the port is bound by the NGS driver - """ - if not GenericSwitchDriver._is_port_supported(port): - return False - - vif_type = port[portbindings.VIF_TYPE] - return vif_type == portbindings.VIF_TYPE_OTHER - def _unplug_port_from_segment(self, port, segment): """Unplug a port from a segment. @@ -695,7 +668,7 @@ def subports_added(self, context, port, subports): 'that has been deleted') return - if not self._is_port_supported(port): + if not ngs_utils.is_port_supported(port): return binding_profile = port['binding:profile'] @@ -738,7 +711,7 @@ def subports_deleted(self, context, port, subports): 'that has been deleted') return - if not self._is_port_supported(port): + if not ngs_utils.is_port_supported(port): return binding_profile = port['binding:profile'] 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 cbf3ba43..40ba86f1 100644 --- a/networking_generic_switch/tests/unit/test_generic_switch_mech.py +++ b/networking_generic_switch/tests/unit/test_generic_switch_mech.py @@ -25,6 +25,7 @@ from networking_generic_switch.devices import utils as device_utils from networking_generic_switch import exceptions from networking_generic_switch import generic_switch_mech as gsm +from networking_generic_switch import utils as ngs_utils @mock.patch('networking_generic_switch.config.get_devices', @@ -1099,10 +1100,11 @@ def test_bind_port_with_physnet(self, m_apc, m_list): @mock.patch.object(provisioning_blocks, 'add_provisioning_component', autospec=True) - def test_bind_portgroup_port_not_supported(self, m_apc, m_list): + @mock.patch.object(ngs_utils, 'is_port_supported', autospec=True) + def test_bind_portgroup_port_not_supported(self, m_ips, m_apc, m_list): driver = gsm.GenericSwitchDriver() driver.initialize() - driver._is_port_supported = mock.MagicMock(return_value=False) + m_ips.return_value = False mock_context = mock.create_autospec(driver_context.PortContext) mock_context._plugin_context = mock.MagicMock() mock_context.current = {'binding:profile': @@ -1134,10 +1136,12 @@ def test_bind_portgroup_port_not_supported(self, m_apc, m_list): @mock.patch.object(provisioning_blocks, 'add_provisioning_component', autospec=True) - def test_bind_port_with_physnet_port_not_supported(self, m_apc, m_list): + @mock.patch.object(ngs_utils, 'is_port_supported', autospec=True) + def test_bind_port_with_physnet_port_not_supported(self, m_ips, m_apc, + m_list): driver = gsm.GenericSwitchDriver() driver.initialize() - driver._is_port_supported = mock.MagicMock(return_value=False) + m_ips.return_value = False mock_context = mock.create_autospec(driver_context.PortContext) mock_context._plugin_context = mock.MagicMock() mock_context.current = {'binding:profile': @@ -1166,10 +1170,11 @@ def test_bind_port_with_physnet_port_not_supported(self, m_apc, m_list): @mock.patch.object(provisioning_blocks, 'add_provisioning_component', autospec=True) - def test_bind_port_port_not_supported(self, m_apc, m_list): + @mock.patch.object(ngs_utils, 'is_port_supported', autospec=True) + def test_bind_port_port_not_supported(self, m_ips, m_apc, m_list): driver = gsm.GenericSwitchDriver() driver.initialize() - driver._is_port_supported = mock.MagicMock(return_value=False) + m_ips.return_value = False mock_context = mock.create_autospec(driver_context.PortContext) mock_context._plugin_context = mock.MagicMock() mock_context.current = {'binding:profile': @@ -1199,10 +1204,12 @@ def test_bind_port_port_not_supported(self, m_apc, m_list): @mock.patch.object(provisioning_blocks, 'add_provisioning_component', autospec=True) - def test_bind_portgroup_802_3ad_port_not_supported(self, m_apc, m_list): + @mock.patch.object(ngs_utils, 'is_port_supported', autospec=True) + def test_bind_portgroup_802_3ad_port_not_supported(self, m_ips, m_apc, + m_list): driver = gsm.GenericSwitchDriver() driver.initialize() - driver._is_port_supported = mock.MagicMock(return_value=False) + m_ips.return_value = False mock_context = mock.create_autospec(driver_context.PortContext) mock_context._plugin_context = mock.MagicMock() mock_context.current = {'binding:profile': diff --git a/networking_generic_switch/utils.py b/networking_generic_switch/utils.py index 82464643..ff2e3b6e 100644 --- a/networking_generic_switch/utils.py +++ b/networking_generic_switch/utils.py @@ -12,6 +12,9 @@ # License for the specific language governing permissions and limitations # under the License. +from neutron_lib.api.definitions import portbindings + + def is_802_3ad(binding_profile): """Return whether a port binding profile is using 802.3ad link aggregation. @@ -25,3 +28,30 @@ def is_802_3ad(binding_profile): if not local_group_information: return False return local_group_information.get('bond_mode') in ['4', '802.3ad'] + + +def is_port_supported(port): + """Return whether a port is supported by this driver. + + Ports supported by this driver have a VNIC type of 'baremetal'. + + :param port: The port to check + :returns: Whether the port is supported by the NGS driver + """ + vnic_type = port[portbindings.VNIC_TYPE] + return vnic_type == portbindings.VNIC_BAREMETAL + + +def is_port_bound(port): + """Return whether a port is bound by this driver. + + Ports bound by this driver have their VIF type set to 'other'. + + :param port: The port to check + :returns: Whether the port is bound by the NGS driver + """ + if not is_port_supported(port): + return False + + vif_type = port[portbindings.VIF_TYPE] + return vif_type == portbindings.VIF_TYPE_OTHER From 5140ba50f89c052ff99ede5192269a209020015d Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Wed, 14 May 2025 12:34:38 +1200 Subject: [PATCH 7/9] Improve test coverage of NetmikoSwitch commands By populating the command variables with template strings, this change validates that the expected commands and substitutions are occuring. Change-Id: I16e0f6b5ff0fd9a25334c1f4deae26656e167b87 --- .../tests/unit/netmiko/test_netmiko_base.py | 113 ++++++++++++++++-- 1 file changed, 100 insertions(+), 13 deletions(-) diff --git a/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py b/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py index 0f78b17f..a6da5892 100644 --- a/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py +++ b/networking_generic_switch/tests/unit/netmiko/test_netmiko_base.py @@ -43,7 +43,62 @@ def _make_switch_device(self, extra_cfg={}): device_cfg = {'device_type': 'netmiko_base', 'ip': 'host'} device_cfg.update(extra_cfg) - return netmiko_devices.NetmikoSwitch(device_cfg) + switch = netmiko_devices.NetmikoSwitch(device_cfg) + switch.ADD_NETWORK = ( + 'add network {segmentation_id} {network_id} {network_name}', + ) + switch.DELETE_NETWORK = ( + 'delete network {segmentation_id} {network_id} {network_name}', + ) + switch.PLUG_PORT_TO_NETWORK = ( + 'plug port {port} to network {segmentation_id}', + ) + switch.DELETE_PORT = ( + 'delete port {port} from network {segmentation_id}', + ) + switch.PLUG_BOND_TO_NETWORK = ( + 'plug bond {bond} to network {segmentation_id}', + ) + switch.UNPLUG_BOND_FROM_NETWORK = ( + 'unplug bond {bond} from network {segmentation_id}', + ) + switch.ADD_NETWORK_TO_TRUNK = ( + 'add network {segmentation_id} to trunk {port}', + ) + switch.REMOVE_NETWORK_FROM_TRUNK = ( + 'remove network {segmentation_id} from trunk {port}', + ) + switch.ENABLE_PORT = ( + 'enable port {port}', + ) + switch.DISABLE_PORT = ( + 'disable port {port}', + ) + switch.ENABLE_BOND = ( + 'enable bond {bond}', + ) + switch.DISABLE_BOND = ( + 'disable bond {bond}', + ) + switch.SET_NATIVE_VLAN = ( + 'set native vlan to {segmentation_id} on port {port}', + ) + switch.DELETE_NATIVE_VLAN = ( + 'delete native vlan {segmentation_id} on port {port}', + ) + switch.SET_NATIVE_VLAN_BOND = ( + 'set native vlan to {segmentation_id} on bond {bond}', + ) + switch.DELETE_NATIVE_VLAN_BOND = ( + 'delete native vlan {segmentation_id} on bond {bond}', + ) + switch.ADD_NETWORK_TO_BOND_TRUNK = ( + 'add network {segmentation_id} to bond trunk {port}', + ) + switch.DELETE_NETWORK_ON_BOND_TRUNK = ( + 'delete network {segmentation_id} on bond trunk {port}', + ) + return switch class TestNetmikoSwitch(NetmikoSwitchTestBase): @@ -64,7 +119,9 @@ def test_batch_missing_backend_url(self): 'NetmikoSwitch.check_output', autospec=True) def test_add_network(self, m_check, m_sctd): self.switch.add_network(22, '0ae071f5-5be9-43e4-80ea-e41fefe85b21') - m_sctd.assert_called_with(self.switch, []) + m_sctd.assert_called_with(self.switch, [ + 'add network 22 0ae071f55be943e480eae41fefe85b21 ' + '0ae071f55be943e480eae41fefe85b21']) m_check.assert_called_once_with(self.switch, 'fake output', 'add network') @@ -76,7 +133,12 @@ def test_add_network(self, m_check, m_sctd): def test_add_network_with_trunk_ports(self, m_check, m_sctd): switch = self._make_switch_device({'ngs_trunk_ports': 'port1, port2'}) switch.add_network(22, '0ae071f5-5be9-43e4-80ea-e41fefe85b21') - m_sctd.assert_called_with(switch, []) + m_sctd.assert_called_with(switch, [ + 'add network 22 0ae071f55be943e480eae41fefe85b21 ' + '0ae071f55be943e480eae41fefe85b21', + 'add network 22 to trunk port1', + 'add network 22 to trunk port2' + ]) m_check.assert_called_once_with(switch, 'fake output', 'add network') @mock.patch('networking_generic_switch.devices.netmiko_devices.' @@ -97,7 +159,9 @@ def test_add_network_with_no_manage_vlans(self, m_check, m_sctd): 'NetmikoSwitch.check_output', autospec=True) def test_del_network(self, m_check, m_sctd): self.switch.del_network(22, '0ae071f5-5be9-43e4-80ea-e41fefe85b21') - m_sctd.assert_called_with(self.switch, []) + m_sctd.assert_called_with(self.switch, [ + 'delete network 22 0ae071f55be943e480eae41fefe85b21 ' + '0ae071f55be943e480eae41fefe85b21']) m_check.assert_called_once_with(self.switch, 'fake output', 'delete network') @@ -109,7 +173,11 @@ def test_del_network(self, m_check, m_sctd): def test_del_network_with_trunk_ports(self, m_check, m_sctd): switch = self._make_switch_device({'ngs_trunk_ports': 'port1, port2'}) switch.del_network(22, '0ae071f5-5be9-43e4-80ea-e41fefe85b21') - m_sctd.assert_called_with(switch, []) + m_sctd.assert_called_with(switch, [ + 'remove network 22 from trunk port1', + 'remove network 22 from trunk port2', + 'delete network 22 0ae071f55be943e480eae41fefe85b21 ' + '0ae071f55be943e480eae41fefe85b21']) m_check.assert_called_once_with(switch, 'fake output', 'delete network') @@ -131,7 +199,8 @@ def test_del_network_with_no_manage_vlans(self, m_check, m_sctd): 'NetmikoSwitch.check_output', autospec=True) def test_plug_port_to_network(self, m_check, m_sctd): self.switch.plug_port_to_network(2222, 22) - m_sctd.assert_called_with(self.switch, []) + m_sctd.assert_called_with(self.switch, [ + 'plug port 2222 to network 22']) m_check.assert_called_once_with(self.switch, 'fake output', 'plug port') @@ -143,7 +212,10 @@ def test_plug_port_to_network(self, m_check, m_sctd): def test_plug_port_has_default_vlan(self, m_check, m_sctd): switch = self._make_switch_device({'ngs_port_default_vlan': '20'}) switch.plug_port_to_network(2222, 22) - m_sctd.assert_called_with(switch, []) + m_sctd.assert_called_with(switch, [ + 'delete port 2222 from network 20', + 'plug port 2222 to network 22' + ]) m_check.assert_called_once_with(switch, 'fake output', 'plug port') @mock.patch('networking_generic_switch.devices.netmiko_devices.' @@ -155,7 +227,10 @@ def test_plug_port_to_network_disable_inactive(self, m_check, m_sctd): switch = self._make_switch_device( {'ngs_disable_inactive_ports': 'true'}) switch.plug_port_to_network(2222, 22) - m_sctd.assert_called_with(switch, []) + m_sctd.assert_called_with(switch, [ + 'enable port 2222', + 'plug port 2222 to network 22' + ]) m_check.assert_called_once_with(switch, 'fake output', 'plug port') @mock.patch('networking_generic_switch.devices.netmiko_devices.' @@ -165,7 +240,8 @@ def test_plug_port_to_network_disable_inactive(self, m_check, m_sctd): 'NetmikoSwitch.check_output', autospec=True) def test_delete_port(self, m_check, m_sctd): self.switch.delete_port(2222, 22, trunk_details={}) - m_sctd.assert_called_with(self.switch, []) + m_sctd.assert_called_with(self.switch, [ + 'delete port 2222 from network 22']) m_check.assert_called_once_with(self.switch, 'fake output', 'unplug port') @@ -177,7 +253,10 @@ def test_delete_port(self, m_check, m_sctd): def test_delete_port_has_default_vlan(self, m_check, m_sctd): switch = self._make_switch_device({'ngs_port_default_vlan': '20'}) switch.delete_port(2222, 22, trunk_details={}) - m_sctd.assert_called_with(switch, []) + m_sctd.assert_called_with(switch, [ + 'delete port 2222 from network 22', + 'add network 20 20 20', + 'plug port 2222 to network 20']) m_check.assert_called_once_with(switch, 'fake output', 'unplug port') @mock.patch('networking_generic_switch.devices.netmiko_devices.' @@ -189,13 +268,16 @@ def test_delete_port_disable_inactive(self, m_check, m_sctd): switch = self._make_switch_device( {'ngs_disable_inactive_ports': 'true'}) switch.delete_port(2222, 22) - m_sctd.assert_called_with(switch, []) + m_sctd.assert_called_with(switch, [ + 'delete port 2222 from network 22', + 'disable port 2222']) m_check.assert_called_once_with(switch, 'fake output', 'unplug port') @mock.patch('networking_generic_switch.devices.netmiko_devices.' 'NetmikoSwitch.plug_port_to_network', return_value='fake output', autospec=True) def test_plug_bond_to_network_fallback(self, m_plug): + self.switch.PLUG_BOND_TO_NETWORK = None self.switch.plug_bond_to_network(2222, 22) m_plug.assert_called_with(self.switch, 2222, 22, trunk_details=None) @@ -203,13 +285,18 @@ def test_plug_bond_to_network_fallback(self, m_plug): 'NetmikoSwitch.delete_port', return_value='fake output', autospec=True) def test_unplug_bond_from_network_fallback(self, m_delete): + self.switch.UNPLUG_BOND_FROM_NETWORK = None self.switch.unplug_bond_from_network(2222, 22) m_delete.assert_called_with(self.switch, 2222, 22, trunk_details=None) def test__format_commands(self): - self.switch._format_commands( - netmiko_devices.NetmikoSwitch.ADD_NETWORK, + self.switch.ADD_NETWORK = ( + 'add network {segmentation_id} {network_id}', + ) + cmds = self.switch._format_commands( + self.switch.ADD_NETWORK, segmentation_id=22, network_id=22) + self.assertEqual(['add network 22 22'], cmds) @mock.patch.object(netmiko_devices.tenacity, 'wait_fixed', return_value=tenacity.wait_fixed(0.01), autospec=True) From ac24e71a8767156b2ff6cdd5c993eb2d63ee2082 Mon Sep 17 00:00:00 2001 From: mumesan Date: Tue, 3 Jun 2025 15:24:06 +0100 Subject: [PATCH 8/9] Improve Netmiko Device Commands Documentation Adds a Sphinx directive to parse each file in the netmiko devices folder and return documentation containing the switch, command modules capable of being executed by the switch, and the CLI commands sent to the switch when the command module is selected. Change-Id: Ie49b158be44d517c82b8f6647096fd8ec6cc903c Signed-off-by: mumesan --- .gitignore | 2 +- doc/source/_exts/netmiko_device_commands.py | 150 ++++++++++++++++++ doc/source/conf.py | 5 +- doc/source/index.rst | 1 + doc/source/netmiko-device-commands.rst | 9 ++ .../devices/netmiko_devices/smc.py | 28 +--- 6 files changed, 166 insertions(+), 29 deletions(-) create mode 100644 doc/source/_exts/netmiko_device_commands.py create mode 100644 doc/source/netmiko-device-commands.rst diff --git a/.gitignore b/.gitignore index cca74a74..8bac9558 100644 --- a/.gitignore +++ b/.gitignore @@ -3,7 +3,7 @@ # Sphinx _build -doc/source/api/ +doc/source/contributor/api/ # release notes build releasenotes/build diff --git a/doc/source/_exts/netmiko_device_commands.py b/doc/source/_exts/netmiko_device_commands.py new file mode 100644 index 00000000..56c708fb --- /dev/null +++ b/doc/source/_exts/netmiko_device_commands.py @@ -0,0 +1,150 @@ +import ast +import inspect +import stevedore + +from docutils import nodes +from docutils.parsers import rst +from docutils.statemachine import ViewList +from sphinx.util.nodes import nested_parse_with_titles + +command_descriptions = { + 'ADD_NETWORK': 'A tuple of command strings used to add a VLAN', + 'DELETE_NETWORK': 'A tuple of command strings used to delete a VLAN', + 'PLUG_PORT_TO_NETWORK': 'A tuple of command strings used to configure a \ + port to connect to a specific VLAN', + 'DELETE_PORT': 'A tuple of command strings used to remove a port from the\ + VLAN', + 'NETMIKO_DEVICE_TYPE': 'Netmiko Supported device type', + 'ADD_NETWORK_TO_TRUNK': 'Adds a network to a trunk port.', + 'REMOVE_NETWORK_FROM_TRUNK': 'Removes a network from a trunk port.', + 'ENABLE_PORT': 'Enables the port', + 'DISABLE_PORT': 'Shuts down the port', + 'ERROR_MSG_PATTERNS': 'A tuple of regular expressions. These patterns are\ + used to match and handle error messages returned by the switch.', + 'SET_NATIVE_VLAN': 'Sets a specified native VLAN', + 'DELETE_NATIVE_VLAN': 'Removes the native VLAN', + 'SAVE_CONFIGURATION': 'Saves the configuration', + 'SET_NATIVE_VLAN_BOND': 'Sets the native VLAN for the bond interface', + 'DELETE_NATIVE_VLAN_BOND': 'Unsets the native VLAN for the bond \ + interface', + 'ADD_NETWORK_TO_BOND_TRUNK': 'Adds a VLAN to the bond interface for \ + trunking', + 'DELETE_NETWORK_ON_BOND_TRUNK': 'Removes a VLAN from the bond interface \ + for trunking', + 'PLUG_PORT_TO_NETWORK_GENERAL': 'Allows the VLAN and lets it carry \ + untagged frames', + 'DELETE_PORT_GENERAL': 'Removes VLAN from allowed list and stops allowing\ + it to carry untagged frames', + 'QUERY_PORT': 'Shows details about the switch for that port', + 'PLUG_BOND_TO_NETWORK': 'Adds bond to the bridge as a port for the VLAN', + 'UNPLUG_BOND_FROM_NETWORK': 'Removes bond\'s access VLAN assignment', + 'ENABLE_BOND': 'Enables bond interface by removing link down state', + 'DISABLE_BOND': 'Disables bond interface by setting its link state to \ + down', +} + + +class DeviceCommandsDirective(rst.Directive): + + def parse_tuples(value): + """Parses the value in the tuples and returns a list of its contents""" + tuple_values = [] + for elt in value.elts: + # Parsing if the item in the tuple is a function call + if isinstance(elt, ast.Call): + func_name = '' + if isinstance(elt.func, ast.Attribute): + func_name = f"{ast.unparse(elt.func.value)}.{elt.func.attr}" + elif isinstance(elt.func, ast.Name): + func_name = elt.func.id + args = [ast.literal_eval(arg) for arg in elt.args] + call_command = str(func_name) + '(\'' + str(args[0]) + '\')' + tuple_values.append(call_command) + + else: + tuple_values.append(ast.literal_eval(elt)) + return tuple_values + + def parse_file(file_content, filename): + """Uses ast to split document body into nodes and parse them""" + tree = ast.parse(file_content, filename=filename) + classes = {} + for node in tree.body: + if isinstance(node, ast.ClassDef): + device_name = node.name + cli_commands = {} + docstring = ast.get_docstring(node) + + if docstring: + cli_commands['__doc__'] = docstring + # Iterates through nodes, checks for type of node and extracts the value + for subnode in node.body: + if isinstance(subnode, ast.Assign): + for target in subnode.targets: + command_name = target.id + if isinstance(target, ast.Name): + ast_type = subnode.value + if isinstance(ast_type, ast.Tuple): + cli_commands[command_name] = DeviceCommandsDirective.parse_tuples(ast_type) + else: + cli_commands[command_name] = ast.literal_eval(ast_type) + if cli_commands: + classes[device_name] = cli_commands + return classes + + def format_output(switch_details): + """Formats output that is to be displayed""" + formatted_output = ViewList() + if '__doc__' in switch_details: + for line in switch_details['__doc__'].splitlines(): + formatted_output.append(f" {line}", "") + formatted_output.append("", "") + del switch_details['__doc__'] + for command_name, cli_commands in switch_details.items(): + desc = command_descriptions.get(command_name, 'No description provided') + formatted_output.append(f" - {command_name}: {desc}", "") + formatted_output.append(f" - CLI commands:", "") + if isinstance(cli_commands, list): + if cli_commands: + for command in cli_commands: + formatted_output.append(f" - {command}", "") + else: + formatted_output.append(f" - No cli commands for this switch command", "") + else: + formatted_output.append(f" - {cli_commands}", "") + return formatted_output + + def run(self): + """Loads the files, parses them and formats the output""" + manager = stevedore.ExtensionManager( + namespace='generic_switch.devices', + invoke_on_load=False, + ) + output_lines = ViewList() + output_lines.append("Switches", "") + output_lines.append("========", "") + + for file_loader in manager.extensions: + switch = file_loader.plugin + module = inspect.getmodule(switch) + file_content = inspect.getsource(module) + filename = module.__file__ + parsed_device_file = DeviceCommandsDirective.parse_file(file_content, filename) + switch_name = switch.__name__ + output_lines.append(f"{switch_name}:", "") + subheading_characters = "^" + subheading = subheading_characters * (len(switch_name) + 1) + output_lines.append(subheading, "") + + if switch_name in parsed_device_file: + switch_details = parsed_device_file[switch_name] + output_lines.extend(DeviceCommandsDirective.format_output(switch_details)) + output_lines.append("", "") + + node = nodes.section() + node.document = self.state.document + nested_parse_with_titles(self.state, output_lines, node) + return node.children + +def setup(app): + app.add_directive('netmiko-device-commands', DeviceCommandsDirective) diff --git a/doc/source/conf.py b/doc/source/conf.py index a311f66d..74d0c958 100644 --- a/doc/source/conf.py +++ b/doc/source/conf.py @@ -16,13 +16,15 @@ import sys sys.path.insert(0, os.path.abspath('../..')) +sys.path.insert(0, os.path.join(os.path.abspath('.'), '_exts')) # -- General configuration ---------------------------------------------------- # Add any Sphinx extension module names here, as strings. They can be # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom ones. extensions = [ 'sphinxcontrib.apidoc', - 'openstackdocstheme' + 'openstackdocstheme', + 'netmiko_device_commands' ] # openstackdocstheme options @@ -85,4 +87,5 @@ apidoc_output_dir = 'contributor/api' apidoc_excluded_paths = [ 'tests', + 'devices/netmiko_devices', ] diff --git a/doc/source/index.rst b/doc/source/index.rst index 0de4200d..50097295 100644 --- a/doc/source/index.rst +++ b/doc/source/index.rst @@ -9,6 +9,7 @@ Welcome to networking-generic-switch's documentation! configuration dev/dev-quickstart contributing + netmiko-device-commands contributor/api/modules Indices and tables diff --git a/doc/source/netmiko-device-commands.rst b/doc/source/netmiko-device-commands.rst new file mode 100644 index 00000000..9d597827 --- /dev/null +++ b/doc/source/netmiko-device-commands.rst @@ -0,0 +1,9 @@ +======================= +Netmiko Device Commands +======================= + +This section contains details of the commands capable of being executed +by the switches and the CLI commands sent to the switch for each command +module that is selected. + +.. netmiko-device-commands:: diff --git a/networking_generic_switch/devices/netmiko_devices/smc.py b/networking_generic_switch/devices/netmiko_devices/smc.py index e0174a7d..8c0b43d4 100644 --- a/networking_generic_switch/devices/netmiko_devices/smc.py +++ b/networking_generic_switch/devices/netmiko_devices/smc.py @@ -16,34 +16,8 @@ class SupermicroSmis(netmiko_devices.NetmikoSwitch): - """A class to represent a Supermicro SMIS switch + """A class to represent a Supermicro SMIS switch.""" - Inherits from: - -------------- - netmiko_devices.NetmikoSwitch - - Class Attributes: - ----------------- - ADD_NETWORK : tuple - A tuple of command strings used to add a VLAN - with a specific segmentation ID - and name the VLAN. - - DELETE_NETWORK : tuple - A tuple of command strings used to delete a VLAN - by its segmentation ID. - - PLUG_PORT_TO_NETWORK : tuple - A tuple of command strings used to configure a port - to connect to a specific VLAN. - This sets the port to access mode and assigns it - to the specified VLAN. - - DELETE_PORT : tuple - A tuple of command strings used to remove a port - from the VLAN. This removes - any trunking configuration and clears VLAN assignments. - """ ADD_NETWORK = ( 'vlan {segmentation_id}', 'name {network_name}', From d26adb822d2cd006e9fabe89a31359e416ee7bce Mon Sep 17 00:00:00 2001 From: Afonne-CID Date: Tue, 3 Jun 2025 23:14:10 +0100 Subject: [PATCH 9/9] Cast numeric Netmiko kwargs to native types. Convert port/timeout options from str to int/float, fixing type errors raised by ConnectHandler. Closes-Bug: #2110111 Change-Id: I9a4a1f857d2ace7c87cfbd6ed56356c1ed227caf Signed-off-by: Afonne-CID --- .../devices/netmiko_devices/__init__.py | 25 +++++++++++++++++++ .../tests/unit/test_devices.py | 21 ++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/networking_generic_switch/devices/netmiko_devices/__init__.py b/networking_generic_switch/devices/netmiko_devices/__init__.py index 5c372ef8..16a601d1 100644 --- a/networking_generic_switch/devices/netmiko_devices/__init__.py +++ b/networking_generic_switch/devices/netmiko_devices/__init__.py @@ -144,6 +144,31 @@ def __init__(self, device_cfg, *args, **kwargs): self.config['session_log_record_writes'] = True self.config['session_log_file_mode'] = 'append' + _NUMERIC_CAST = { + "port": int, + "global_delay_factor": float, + "conn_timeout": float, + "auth_timeout": float, + "banner_timeout": float, + "blocking_timeout": float, + "timeout": float, + "session_timeout": float, + "read_timeout_override": float, + "keepalive": int, + } + + for key, expected_type in _NUMERIC_CAST.items(): + value = self.config.get(key) + if isinstance(value, str): + try: + self.config[key] = expected_type(value) + except ValueError: + LOG.error( + "Invalid value %s for %s; expected %s", + value, key, expected_type.__name__, + ) + raise exc.GenericSwitchNetmikoConfigError() + self.lock_kwargs = { 'locks_pool_size': int(self.ngs_config['ngs_max_connections']), 'locks_prefix': self.config.get( diff --git a/networking_generic_switch/tests/unit/test_devices.py b/networking_generic_switch/tests/unit/test_devices.py index 4486c364..dc971007 100644 --- a/networking_generic_switch/tests/unit/test_devices.py +++ b/networking_generic_switch/tests/unit/test_devices.py @@ -267,3 +267,24 @@ def test__get_ssh_disabled_algorithms(self): "ciphers": ["blowfish-cbc", "3des-cbc"], } self.assertEqual(expected, algos) + + def test_float_params_cast(self): + config = { + "device_type": 'netmiko_ovs_linux', + "ip": "10.1.2.3", + "username": "u", + "password": "p", + "conn_timeout": "20.0", + "global_delay_factor": "2.5", + "port": "2222", + } + device = devices.device_manager(config) + + self.assertIsInstance(device.config["conn_timeout"], float) + self.assertEqual(device.config["conn_timeout"], 20.0) + + self.assertIsInstance(device.config["global_delay_factor"], float) + self.assertEqual(device.config["global_delay_factor"], 2.5) + + self.assertIsInstance(device.config["port"], int) + self.assertEqual(device.config["port"], 2222)