Skip to content
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.

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

Update y_cable_simulator_client to address SFP issue. #2952

Merged
merged 4 commits into from
Feb 19, 2021

Conversation

bingwang-ms
Copy link
Collaborator

@bingwang-ms bingwang-ms commented Feb 7, 2021

Signed-off-by: bingwang [email protected]

Description of PR

Summary:
Fixes # (issue)
In topo dualtor_56, a 100G interfaces are splited into two 50G interfaces according to port_config.ini.

# name          lanes             alias         index  speed
Ethernet0       1,2               Ethernet1/1   1      50000
Ethernet2       3,4               Ethernet1/3   1      50000
Ethernet4       5,6               Ethernet2/1   2      50000
Ethernet6       7,8               Ethernet2/3   2      50000
Ethernet8       9,10              Ethernet3/1   3      50000
Ethernet10      11,12             Ethernet3/3   3      50000
Ethernet12      13,14             Ethernet4/1   4      50000
……

As a result, the physical port id got from SFP is different with host interface index, which is used as the name of mux bridge.
For example, the physical port id for Ethernet4 is 2 according port_config.ini, but the interface is connect to mux bridge mbr-vms17-8-2. Therefore, we need to calculate host interface index from physical port id.

This PR addressed the issue by calculating host interface index according to port config file. Most of the logic is same with read_porttab_mappings in sfputilhelper.py

Type of change

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

Approach

What is the motivation for this PR?

This PR is to addressed the issue caused by port spliting.

How did you do it?

Calculate the correct mapping from physical port to host interface index.

How did you verify/test it?

  1. Verified on A7050 by printing out the mapping and checked manually.
  2. Print out the mapping from physical port to host interface index, and confirmed the mapping is correct.
>>> y.g_physical_to_host_port_mapping
{1: 0, 2: 2, 3: 4, 4: 6, 5: 8, 6: 10, 7: 12, 8: 13, 9: 14, 10: 15, 11: 16, 12: 18, 13: 20, 14: 22, 15: 24, 16: 26, 17: 28, 18: 30, 19: 32, 20: 34, 21: 36, 22: 38, 23: 40, 24: 41, 25: 42, 26: 43, 27: 44, 28: 46, 29: 48, 30: 50, 31: 52, 32: 54}

Any platform specific information?

No.

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

No.

Documentation

In topo dualtor_56, a 100G interfaces are splited into two 50G
interfaces. The physical port id got from spf is different with host
interface index, which is used as the name of mux bridge. As a result,
y_cable_simulator_client failed to toggle some interface. This PR
addressed the issue by calculating the correct mapping from physical
port to host interface index.

Signed-off-by: bingwang <[email protected]>
@bingwang-ms bingwang-ms requested a review from a team as a code owner February 7, 2021 11:07
else:
g_physical_to_logical_port_mapping[fp_port_index].append(intf_name)

else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel that the else portion could be abstracted out as a helper function. This change can come with another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion. Updated. Thanks


if not g_physical_to_host_port_mapping or not g_physical_to_logical_port_mapping:
_load_port_info()
return g_physical_to_host_port_mapping[physical_port]
Copy link
Collaborator

@yxieca yxieca Feb 16, 2021

Choose a reason for hiding this comment

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

This is unlikely to happen, but if _load_port_info() failed, you probably should return the old (physical_port - 1) as it still works on some platforms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated. Thanks!

@bingwang-ms bingwang-ms changed the title Update y_cable_simulator_client to address spf issue. Update y_cable_simulator_client to address SFP issue. Feb 18, 2021
1. Return physical_port - 1 if loading port_config file failed.
2. Fix typo

Signed-off-by: bingwang <[email protected]>
for intf in ports.keys():
logical_list.append(intf)

logical = natsorted(logical_list, key=lambda y: y.lower())
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using a single line of code for the purpose of line 75-79:

logical = natsorted([intf.lower() for intf in ports.keys()])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using a single line code is a good suggestion. But I'd like to keep current code here because converting all interface name into lower case can cause some issue in following code.

tests/templates/y_cable_simulator_client.j2 Outdated Show resolved Hide resolved
else:
g_physical_to_logical_port_mapping[fp_port_index].append(intf_name)

def _load_port_config_ini(porttabfile):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering if the sonic_py_common package already has code for reading and parsing the port_config.ini file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, there is no such code. Most of the code for parsing port config file is from sfputilhelper.py

tests/templates/y_cable_simulator_client.j2 Show resolved Hide resolved
tests/templates/y_cable_simulator_client.j2 Outdated Show resolved Hide resolved
tests/templates/y_cable_simulator_client.j2 Show resolved Hide resolved
Signed-off-by: bingwang <[email protected]>
@bingwang-ms bingwang-ms merged commit 9b41a7a into sonic-net:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants