-
Notifications
You must be signed in to change notification settings - Fork 727
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
[VLAN test] Add VLAN test #375
Conversation
- debug: var=vlan_intf_list | ||
|
||
- name: Flush all IP addresses on the LAGs | ||
shell: cfgmgr intf del {{ (item.addr ~ "/" ~ item.mask)|ipaddr()|upper() }} dev {{ item.attachto }} |
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.
cfgmgr is not checked in. any plan to do this?
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.
We do not know how to run this test since the cfgmgr is not checked-in. Is there any plan to integrate cfgmgr into the sonic-utilities first?
I have replace cfgmgr to vlan command introduced by sonic-net/sonic-utilities#151. Please take a look at. Thank you. |
- "{{ minigraph_portchannel_interfaces }}" | ||
become: true | ||
|
||
- name: Delete all IP addresses on the LAGs in config DB |
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.
This is a workaround since there is no command line to do both configure interface IP and write to configDB now. Is there any plan to support this?
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.
We plan to support IP and Lag configuration though configDb in next release March.
In reply to: 155185549 [](ancestors = 155185549)
dst_mac, self.dataplane.get_mac(0, target["port_index"]), | ||
src_ip, target["remote_ip"], 63) | ||
|
||
# Test case #5 |
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.
The Test case 5 tests ping to CPU and verifies reply from CPU.
One general question may not relate to the code but need to clarify. |
Hi @keboliu, |
#----------------------------------------- | ||
- name: Clean up Vlan test configuration on the testbed | ||
include: vlan_cleanup.yml | ||
tags: vlan_cleanup |
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.
suggest adding using log analyzer to analyze the errors in the log as part of the test as ACL test and Everflow test did, by this could make these test cases more complete.
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.
Thank you kebo, LogAnalyzer will make the test case more complete. But I think it's better for me to add LogAnalyzer later after the test check in due to the time factor.
shell: config save -y | ||
become: true | ||
|
||
- name: Reboot is required for config_db.json change |
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.
could this reboot be avoided by using "config reload config_db.json" to flush and reload the new configuration?
shell: mv /etc/sonic/config_db_bak.json /etc/sonic/config_db.json | ||
become: true | ||
|
||
- name: Reboot is required for config_db.json change |
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.
Same question as previous one, is this reboot avoidable by using "config reload config_db_back.json"?
@tieguoevan, can you provide command line how to run this test in README.test.md? |
|
||
#-------------------------------------------------------------------------- | ||
def build_icmp_packet(self, vlan_id, | ||
src_mac="00:22:00:00:00:02", dst_mac="00:22:00:00:00:01", |
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.
where does this dst_mac get populated the in the DUT switch. How does the switch know how to forward this packet?
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.
If dst_mac is not specified, an unknown unicast ICMP package is constructed by default. "00:22:00:00:00:01" is unknown for switch unless a coincidence, switch need to flood this packet in the VLAN.
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.
since your purpose is to broadcast the packet, why not use broadcast mac in this case? the behavior of unknown unicast mac can be changed.
@tieguoevan, did you modify the arp_responder to support new format?
|
I think arp_responder is support this format. What error message have you encountered? |
self.verify_icmp_packets_from_specified_port(target["port_index"], | ||
target["vlan_id"] if target["vlan_id"] != target["pvid"] else 0, | ||
dst_mac, self.dataplane.get_mac(0, target["port_index"]), | ||
src_ip, target["peer_ip"], 63) |
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.
port 16 is part of lag, the packet can go to either port 16 or port 17. Here you only check port 16 which is not enough.
01:36:21.910 root : INFO : Send tagged(200) packet from 4...
01:36:21.910 root : INFO : 7c:fe:90:5e:6b:04 -> 90:b1:1c:f4:a8:53, 192.168.200.4 -> 192.168.100.16
01:36:21.912 root : INFO : Verify packet from port 16
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.
The test only supports t0 testbed type now. The t0 testbed type should only suppprt one member in LAG? If t1-lag is supported in the future, here need to be modified indeed.
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.
another issue.
05:14:27.515 root : INFO : Test case #4 starting ...
05:14:27.524 root : INFO : Send tagged(200) packet from 4...
05:14:27.525 root : INFO : 7c:fe:90:5e:6b:04 -> 90:b1:1c:f4:a8:53, 192.168.200.4 -> 192.168.100.4
05:14:27.528 root : INFO : Verify packet from port 4
05:14:27.543 root : INFO : Send tagged(200) packet from 4...
05:14:27.543 root : INFO : 7c:fe:90:5e:6b:04 -> 90:b1:1c:f4:a8:53, 192.168.200.4 -> 192.168.100.16
05:14:27.545 root : INFO : Verify packet from port 16
05:14:27.558 root : INFO : Send tagged(200) packet from 4...
05:14:27.559 root : INFO : 7c:fe:90:5e:6b:04 -> 90:b1:1c:f4:a8:53, 192.168.200.4 -> 192.168.100.0
05:14:27.562 root : INFO : Verify packet from port 0
The destination IP 192.168.100.0 is strange
pass | ||
#-------------------------------------------------------------------------- | ||
|
||
def setUpArpResponder(self): |
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.
in order for the test to make, every port in the ptf conainer needs to have a different mac. That is not the setup by default, you need to run this script. Can you add in the test?
#-------------------------------------------------------------------------- | ||
def send_icmp_packet(self, vlan_port, vlan_id=0, | ||
src_mac="00:22:00:00:00:02", dst_mac="00:22:00:00:00:01", | ||
src_ip="192.168.0.1", dst_ip="192.168.0.2", ttl=64): |
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.
you already have default paramaters in build_icmp_packet, can we avoid duplication here?
#-------------------------------------------------------------------------- | ||
def verify_icmp_packets(self, vlan_port, vlan_id, | ||
src_mac="00:22:00:00:00:02", dst_mac="00:22:00:00:00:01", | ||
src_ip="192.168.0.1", dst_ip="192.168.0.2", ttl=64): |
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.
you already have default paramaters in build_icmp_packet, can we avoid duplication here?
masked_exp_pkt = Mask(exp_pkt) | ||
masked_exp_pkt.set_do_not_care_scapy(scapy.IP, "id") | ||
|
||
verify_packets(self, masked_exp_pkt, list(str(src_port))) |
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.
can you print similiar log here?
self.log("Verify packet from ports ???")
I do not find the verify log in test 5.
- name: Configure IP addresses on VLAN interfaces in Config DB | ||
shell: docker exec -i database redis-cli -n 4 hset "VLAN_INTERFACE|Vlan{{ item.vlan_id }}|{{ item.ip }}" NULL NULL | ||
with_items: "{{ vlan_intf_list }}" | ||
become: true |
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.
all these steps seems complex, why not provide a configdb.json file we need here and just reload to the new configdb?
become: true | ||
|
||
- name: Configure IP addresses on VLAN interfaces in Config DB | ||
shell: docker exec -i database redis-cli -n 4 hset "VLAN_INTERFACE|Vlan{{ item.vlan_id }}|{{ item.ip }}" NULL NULL |
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.
why do we need to config ip addr manually in previous task? isn't enough by just configuring the VLAN_INTERFACE in the configdb?
include_vars: '/tmp/vlan_info.yml' | ||
|
||
- debug: var=vlan_ports_list | ||
- debug: var=vlan_intf_list |
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.
can we move line 7-20 to vlantb.yml? I need those variables when I only run the vlan_test.yml
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.
as comments
@tieguoevan besides enabling the QinQ configuration on the fanout switch ports which connected to the DUT, how about the fanout trunk port which connected to the root fanout? any extra configuration needed? |
Modified. Please have a look again. Thanks. @lguohan |
@keboliu no extra configuration needed for us. |
All cases passed on my local testbed. |
* msft_github/master: [PFCWD]: Add Add support for t0 toplogy and arista fanout testbed (sonic-net#424) add orchagent process check before each test and sanity after each test (sonic-net#431) Move mem_check.sh into helpers/ directory to conform with location of other helper scripts (sonic-net#435) fix tests for t0-116 topology (sonic-net#430) Updated labinfo file with Arista fanout sku (sonic-net#429) add missing snmp testcases to top level snmp.yml; fixed wrong test file for neighbour test; change arp test comment more readable (sonic-net#428) Fix sku-sensors-data for 7060 (sonic-net#427) Fix port alias mapping for Arista-7050QX-32S (sonic-net#426) [Lag 2] Allow lacp timing tests to retry limited times until succeeded (sonic-net#403) [hwsku]: Add Accton-AS7716-32X (sonic-net#405) add generate minigrah using testbed_name option (sonic-net#425) Update README.testbed.md Create README.testbed.Example.md Update README.testbed.Config.md add missing test case name (sonic-net#417) fix typo when assign dscp_mode for decap test (sonic-net#418) [repeat harness] rework repeat harness and introduce test case continuous reboot (sonic-net#416) allow upgrade sonic via onie or sonic-to-sonic upgrade method (sonic-net#413) [topology]: Fix t1-64-lag topology device link base indices (sonic-net#414) Run sudo command with log_analyzer (sonic-net#415) [sonic tests] use host time instead of ansible time (sonic-net#410) [upgrade_sonic]: Fix the hostname in the wait_for condition (sonic-net#411) [Test cases] clean up some test cases changes and enabling more tests (sonic-net#409) add restart_swss (sonic-net#412) [sonic test] introduce a repeat harness (sonic-net#402) Update README.test.md [bgp_speaker]: always clean up test environment after it finishes (sonic-net#406) [vlan]: add vlan test to test by testname (sonic-net#408) [bug]: convert the interface from unicode to string (sonic-net#407) [VLAN test] Add VLAN test (sonic-net#375) [vm]: change the default Front plane port to 4 instead of 8 (sonic-net#404) Update README.testbed.Setup.md [acl]: Adopt tests with acl-loader to t0 and t1-lag topo. (sonic-net#370) add connect_topo command for testbed-cli.sh (sonic-net#398) [FIB, BGP speaker] fix t0-116 topology source port list (sonic-net#396) [bgp_speaker]: Specify VLAN IP route in case LPM to other nexthop (sonic-net#394) [sensor]: Change sensor labels for Arista 7050 and 7260 (sonic-net#395) Delete Arista DPMs sensors data (sonic-net#393) [bug]: change maximum packet to 9114 to send in the mtu test (sonic-net#391)
Hi all Please see our test results belows.
|
Signed-off-by: Judong evan.tyf@alibaba-inc.com