-
Notifications
You must be signed in to change notification settings - Fork 732
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
support for not all DUT ports connected to a fanout switch #2517
Conversation
This pull request introduces 1 alert when merging 0f8b9b63e5d7623089902177c3c239bbb15c5a55 into 1317643 - view on LGTM.com new alerts:
|
143479c
to
212752e
Compare
@lolyu can you please review these changes. |
432e68e
to
7c71c5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the conflict.
7c71c5c
to
0b6cca0
Compare
@lolyu - I have re-based and fixed the merge conflict. Also, the module_utils PR has been merged into master. So, moved the common code of port_alias_to_name_map between conn_graph_facts and minigraph_facts to module_utils/port_utils.py. Can you please give it a final review before we merge this PR into master. |
This pull request introduces 2 alerts when merging 0b6cca041aa089844b773558e362a8e01995d606 into f5537b1 - view on LGTM.com new alerts:
|
Retest this please |
589c96e
to
38dfa98
Compare
conn_graph_facts: conn_graph_facts.py returns device_vlan_map_list. This used to be a dictionary with key being hostname and value being a list of vlanids for the fanout ports. We have modified this where the value instead of being a list of vlanids, it is a dictionary with key being the port_index and the value being the vlan id. This port_index is what gets put in the topology file. We get the port by looking at the host_port_vlans defined in the conn_graph for that device. This host_port_vlans has key being the Ethernet port (like Ethernet10) and value being a dictionary with key 'vlanlist' being a the list of fanout vlans. We check against all the ports 'vlanlist' to get the port on the DUT that connects to this fanout vlan, and then split on 'Ethernet' to get the port index. For example - lets say on dut with hostname "dut1", we have port Ethernet10 connected to fanout w/ vlan 120, and Ethernet11 connected to fanout w/ vlan 121, then we would have: "host_port_vlans": { "Ethernet10": { "mode": "Access", "vlanids": "120", "vlanlist": [ 120 ] }, "Ethernet11": { "mode": "Access", "vlanids": "121", "vlanlist": [ 121 ] } } "VlanList": [ 120, 121 ] For vlan 120 in VlanList, we would iterate through host_port_vlans to find the port that has vlan 120 - in our case "Ethernet10". The port_index would then be "10". Similarly, for vlan 121, the port_index would be "Ethernet11". So, returned device_vlan_map_list would be: "dut1" : { "10" : 120, "11" : 121 } - vlan_port/kvm_port/mellanox_simx_port: - Updated to return (dut_fp_ports) a dictionary with key being the port index (same as in the topo file) and vlan being the port - instead of the just the list of ports. bind/unbind vm_topology: - vlan_index is now a string in the dictionary of dut_fp_ports - updated regexp for checking valid vlan for multi-dut to be of the format '<dutIndex>.<portIndex>@<ptfIndex>' remove_dut_port.yml: - set cmd to "remove" instead of "create" in vlan_port module call. }
- The dut_fp_ports is a dictionary with key being a string and not an integer
- We were using the port name like 'Ethernet10' and splitting in out 'Ethernet' to get the port_index in the device_vlan_map_list. This port_index is to correspond to what would be in the topology file. This would not work when ports are not consecutively names - like 'Ethernet0', 'Ethernet4', 'Ethernet8' ..... In this scenario, the topology file would have host_interfaces/vlans still as 0,1,2,..... Fix is to get the port names based on 'hwksu' (like it is done for minigraph) and then use that list to get the port_index to be put into device_vlan_map_list
…and conn_graph_facts to module_utils/port_utils The port_alias_to_name_map is generated based on hwsku and is common code between minigraph_facts and conn_graph_facts ansible module. Now that module_utils are supported, added module_utils/port_utils.py to have this common code. Modified the ansible modules to use this new common code.
Since dut_fp_ports is a dictionary with key being the dut name, and the value being a dictionary with key being the interface index as a string, need to typecast the host_interfaces interface index as a string.
7d33ef6
to
be22df7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks, @sanmalho-git. |
[minigraph_facts] Fix conflicts Move the port alias to name mapping parsing to ansible/module_utils/port_utils.py that is introduced by the following patch: sonic-net#2517 Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
Description of PR Fanout graph parsing was broken due to #2517 Following errors were seen while running pfcwd tests E RunAnsibleModuleFail: run module conn_graph_facts failed, Ansible Results => E { E "changed": false, E "failed": true, E "invocation": { E "module_args": { E "anchor": null, E "filename": "/var/nejo/Networking-acs-sonic-mgmt/tests/common/fixtures/../../../ansible/files/starlab_connection_graph.xml", E "filepath": null, E "host": "str-7260cx3-acs-fan-05", E "hosts": null E } E }, E "msg": "Did not find port for Ethernet23/1 in the ports based on hwsku 'Arista-7260CX3' for host str-7260cx3-acs-fan-05" E } How did you do it? Parsing logic added in #2517 was for SONIC duts. Retained the old logic when dev type is FanoutLeaf How did you verify/test it? Ran one of the pfcwd tests and it passed
[minigraph_facts] Fix conflicts Move the port alias to name mapping parsing to ansible/module_utils/port_utils.py that is introduced by the following patch: sonic-net#2517 Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
Description of PR
Summary:
Fixes # (issue)
Type of change
Approach
What is the motivation for this PR?
Currently, it is required that all ports on DUT are in use and are connected to a fanout. However, there is a need to be able to run tests where all ports are not in use. Specifically, when dealing with
So, need to add support where not all DUT ports are connected to fanout and are thus not part of the testing.
How did you do it?
conn_graph_facts:
conn_graph_facts.py returns device_vlan_map_list. This used to be a dictionary with key being hostname and value being a list of vlanids for the fanout ports. Have modified this where the value instead of being a list of vlanids, it is a dictionary with key being the port_index and the value being the vlan id. This port_index is what gets put in the topology file. We get the port by looking at the host_port_vlans defined in the conn_graph for that device. This host_port_vlans has key being the Ethernet port (like Ethernet10) and value being a dictionary with key 'vlanlist' being a the list of fanout vlans. We check against all the ports 'vlanlist' to get the port on the DUT that connects to this fanout vlan, and then split on 'Ethernet' to get the port index.
For example - lets say on dut with hostname "dut1", we have port Ethernet10 connected to fanout w/ vlan 120, and Ethernet11 connected to fanout w/ vlan 121, then we would have:
For vlan 120 in VlanList, we would iterate through host_port_vlans to find the port that has vlan 120 - in our case "Ethernet10". The port_index would then be "10". Similarly, for vlan 121, the port_index would be "Ethernet11".
So, returned device_vlan_map_list would be:
vlan_port/kvm_port/mellanox_simx_port:
bind/unbind vm_topology:
remove_dut_port.yml (bug fix):
How did you verify/test it?
Tested against pizza box DUT with all DUT ports connected to a fanout, and also against another DUT where we have only 4 of the 52 ports connected to a different fanout.
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation