Skip to content

[L1 Switch] Introduction of the L1 Switch API #8431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[L1 Switch] Introduction of the L1 Switch API #8431

wants to merge 2 commits into from

Conversation

developfast
Copy link
Contributor

@developfast developfast commented May 26, 2023 •
edited
Loading

Description of PR

Summary: With the introduction of L1 switches to SONiC's testing suite, there were no existing APIs available to interact with the L1 switch and use its functionality. This PR introduces new APIs that will allow any user to interact with an L1 switch in the lab, especially for automating connections for the integrated tesbed setup, and during nightly runs.

We introduce 3 primary APIs to interact with the L1 switch in a seamless manner. All APIs have been designed to be idempotent:

create_connection(ports):
        """
        Creates a bi-directional connection between the two given ports.

        Args:
            ports (list): List of tuple of ports to connect ex. [(1, 2), (3, 4)]
        Returns:
            None
        """

remove_connection(ports):
        """
        Removes the bi-directional connection between the two given ports.

        Args:
            ports (list): List of tuple of ports to remove ex. [(1, 2), (3, 4)]
        Returns:
            None
        """

get_connected_ports():
        """
        Gets all the connected ports in dict form on the switch.

        Args:
            None
        Returns:
            connected_ports (dict): key value pair of connected ports
        """

Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 201911
  • 202012
  • 202205

Approach

What is the motivation for this PR?

The introduction of L1 switches requires a level of automation for automated setup of testbeds for debugging as well as nightly runs that is not available to us currently. This PR provides easily usable APIs that can be used to automate several L1 switch operations such as creating or removing connections.

How did you do it?

Using the paramiko SSH client, commands are able to be run on the remote server i.e. L1 switch from a host server. Created 3 new APIs.

How did you verify/test it?

Ran it on the SONiC L1 Switch

Ran a test script

l1_switch = L1SwitchHost(hostname, username, password)
print("Create connections...")
l1_switch.create_connection([(1, 2), (3, 4)])
print("Current connections:\n {}".format(l1_switch.connections))
l1_switch.remove_connection([(1, 2), (3, 4)])
print("Current connections:\n {}".format(l1_switch.connections))

Output

devojha@dojhaVM:~/l1-switch-cli$ python3 l1_test_script.py 
Create connections...
Executing command: connection create 1AE1 to 1AW2
Executing command: connection create 1AE2 to 1AW1
Executing command: connection create 1AE3 to 1AW4
Executing command: connection create 1AE4 to 1AW3
Current connections:
 {'1AE1': '1AW2', '1AW1': '1AE2', '1AE2': '1AW1', '1AW2': '1AE1', '1AE3': '1AW4', '1AW3': '1AE4', '1AE4': '1AW3', '1AW4': '1AE3'}
Executing command: connection disconnect 1AE1 from 1AW2
Executing command: connection disconnect 1AE2 from 1AW1
Executing command: connection disconnect 1AE3 from 1AW4
Executing command: connection disconnect 1AE4 from 1AW3
Current connections:
 {}

Logs on the L1 switch

W2W-ROME[OPER]# 04-11-2023 00:01 CONNECTING...
04-11-2023 00:01 CONNECTION OPERATION SUCCEEDED:E1[1AE1]<->W2[1AW2] OP:connect
04-11-2023 00:01 CONNECTING...
04-11-2023 00:01 CONNECTION OPERATION SUCCEEDED:E2[1AE2]<->W1[1AW1] OP:connect
04-11-2023 00:01 CONNECTING...
04-11-2023 00:02 CONNECTION OPERATION SUCCEEDED:E3[1AE3]<->W4[1AW4] OP:connect
04-11-2023 00:02 CONNECTING...
04-11-2023 00:02 CONNECTION OPERATION SUCCEEDED:E4[1AE4]<->W3[1AW3] OP:connect
04-11-2023 00:02 DISCONNECTING...
04-11-2023 00:03 CONNECTION OPERATION SUCCEEDED:E1[1AE1]<->W2[1AW2] OP:disconnect
04-11-2023 00:03 DISCONNECTING...
04-11-2023 00:03 CONNECTION OPERATION SUCCEEDED:E2[1AE2]<->W1[1AW1] OP:disconnect
04-11-2023 00:03 DISCONNECTING...
04-11-2023 00:03 CONNECTION OPERATION SUCCEEDED:E3[1AE3]<->W4[1AW4] OP:disconnect
04-11-2023 00:03 DISCONNECTING...
04-11-2023 00:04 CONNECTION OPERATION SUCCEEDED:E4[1AE4]<->W3[1AW3] OP:disconnect

Any platform specific information?

Applicable for the W2W ROME L1 switches

Supported testbed topology if it's a new test case?

Documentation

@neethajohn neethajohn requested review from wangxin and neethajohn May 26, 2023 16:19
@developfast developfast marked this pull request as ready for review May 31, 2023 17:15
wangxin
wangxin reviewed Jun 1, 2023
View reviewed changes
Copy link
Collaborator

@wangxin wangxin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely, configuring L1 switch is required in both ansible playbook and in pytest script. One possible better solution is to create some customized ansible modules and put them under ansible/library.
Then in ansible playbook, we can call the modules to configure L1 switch or get status from L1 switch. In pytest, similarly, we can define a class like L1SwitchHost as a subclass of AnsibleHost and encapsulate some operations to call the customized modules as methods of this class.

Another possible direction is to use the netmiko package installed in sonic-mgmt docker container. This package already has code for running random commands on remote host through SSH.

tests/common/devices/l1_switch.py
"""
@summary: Class for L1 Switch

For running commands on the L1 switch via ansible. Each port on the L1 switch consists of two uni-directional ports,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my understanding, this class is to run commands through SSH connection established by paramiko, not through ansible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's right

tests/common/devices/l1_switch.py

def __init__(self, hostname, user, passwd, device_type="W2W ROME"):
self.hostname = hostname
self.type = device_type
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use self.device_type because type is also a python built-in keyword.

tests/common/devices/l1_switch.py
self.execute_command(command)
time.sleep(2)

def startup(self, port_name, direction="bi-directional"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I see code duplication in the startup and shutdown method. It hints that the code in these two methods can be extracted into a third private method like self._update_port_status(self, port_name, status, direction).
  • For the direction argument, it would be better to use enum instead of hard coded string.
  • Is it better to get the result of such operations and return it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take all these into account

tests/common/devices/l1_switch.py

return self.connections

def create_connection(self, ports):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to rename ports argument to port_pairs?

tests/common/devices/l1_switch.py
self.connections[port1_E] = port2_W
self.connections[port1_W] = port2_E
self.connections[port2_E] = port1_W
self.connections[port2_W] = port1_E
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to return result of this operation?

tests/common/devices/l1_switch.py
self.connections[port2_E] = port1_W
self.connections[port2_W] = port1_E

def remove_connection(self, ports):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ports -> port_pairs?

tests/common/devices/l1_switch.py
pass

def __str__(self):
return "{ L1 Switch - hostname: '%s', device_type: '%s' }" % (self.hostname, self.type)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The common practice to represent a class is to use < xxx >.

tests/common/devices/l1_switch.py
attempts = 0
connected_session = False
stdin, stdout, stderr = None, None, None
self.client = self.setup_SSH_connection()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time execute_command is called, it needs to setup a SSH connection. Not really efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required as the SSH command requires a timeout after running every command, and during this timeout the SSH session is lost. The L1 switch is a robotic switch so running each command takes some time.

tests/common/devices/l1_switch.py
self.client = None

def __getattr__(self, module_name):
return getattr(self.host, module_name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is for getting an ansible module? However, I don't think this gonna work. When is self.host initialized?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangxin wangxin wangxin left review comments

@neethajohn neethajohn Awaiting requested review from neethajohn

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Request for 202012 branch Request for 202205 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@developfast @wangxin @neethajohn