Skip to content

Fix lag member ptf to dut test#7776

Closed
twtseng-tim wants to merge 2 commits intosonic-net:202205from
twtseng-tim:fix_202205_lag_member
Closed

Fix lag member ptf to dut test#7776
twtseng-tim wants to merge 2 commits intosonic-net:202205from
twtseng-tim:fix_202205_lag_member

Conversation

@twtseng-tim
Copy link
Copy Markdown
Contributor

@twtseng-tim twtseng-tim commented Mar 17, 2023 •
edited
Loading

Description of PR

Summary:
Fix dut may learn mac on wrong port in ptf_to_dut_traffic_test

Fixes # (issue)
issues#6663

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 lag member test configuration on ptf looks like this:

ptf:                              dut:           
submask: 192.168.9.0/24           Vlan109: ip: 192.168.9.1 
eth23: ip: 192.168.9.3------------|-Ethernet92   
                                  |              
                                  |              
bond1: ip: 192.168.9.2------------|-PortChannel1:
| eth1                              | Ethernet4  
| eth2                              | Ethernet8  
| eth6                              | Ethernet24 
| eth7                              | Ethernet28 
| eth20                             | Ethernet80 
| eth21                             | Ethernet84 
| eth22                             | Ethernet88 
| eth24                             | Ethernet96

Both bond1 and eth23 have same subnet mask.
When dut send arp to ptf for ask nexthop, both ports reply 192.168.9.2 is-at here.
So arp might learn on the wrong port, failing the icmp packet forwarding test.

How did you do it?

Send arp request form ptf first, let dut learn nexthop on the correct port.

How did you verify/test it?

Run and passed the lag_member_test test case.

============================ slowest test durations ============================
304.87s teardown pc/test_lag_member.py::test_lag_member_traffic
113.19s setup    pc/test_lag_member.py::test_lag_member_status
12.62s setup    pc/test_lag_member.py::test_lag_member_traffic
9.50s call     pc/test_lag_member.py::test_lag_member_traffic
8.83s teardown pc/test_lag_member.py::test_lag_member_status
0.98s call     pc/test_lag_member.py::test_lag_member_status
=========================== short test summary info ============================
PASSED pc/test_lag_member.py::test_lag_member_status
PASSED pc/test_lag_member.py::test_lag_member_traffic
==================== 2 passed, 2 warnings in 451.20 seconds ====================

Any platform specific information?

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

Documentation

@twtseng-tim
Fix lag member ptf to dut test
3e3be00 
Fix dut may learn mac on wrong port in ptf_to_dut_traffic_test
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Mar 17, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

yanjundeng
yanjundeng reviewed Apr 25, 2023
View reviewed changes
ansible/roles/test/files/acstests/lag_test.py

arp_pkt = Ether(src= src_mac, dst=dst_mac)/ARP(op=ARP.who_has, psrc=src_ip, pdst=dst_ip, hwsrc=src_mac)
send(self, port_behind_lag, arp_pkt)
time.sleep(1)
Copy link
Copy Markdown
Contributor

@yanjundeng yanjundeng Apr 25, 2023

Choose a reason for hiding this comment

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

Commonly, in sonic test scripts, the "arp responder" is used to give correct ARP response to DUT.
Here, seems trying to use a different way by sending an ARP reqeust or Gratuitous ARP. Maybe that's not a good solution since it will reduce the step of the ARP request from DUT and it may be not a normal behavior in networking. If the "arp request" from dut is also one necessary step for this whole testcase, we should not bypass it.

@yanjundeng
Copy link
Copy Markdown
Contributor

yanjundeng commented Apr 25, 2023

-->Both bond1 and eth23 have same subnet mask.
-->When dut send arp to ptf for ask nexthop, both ports reply 192.168.9.2 is-at here.

Here, why do the ARP in PTF use submask here? The submask should only be used by ARP proxy.

@wangxin
Copy link
Copy Markdown
Collaborator

wangxin commented Jul 10, 2023

@twtseng-tim Is this a backport of master branch PR to 202205 branch? Can you mention the master branch PR ID here in PR description?

@twtseng-tim
Copy link
Copy Markdown
Contributor Author

twtseng-tim commented Jul 10, 2023

Sorry, this is not a backport PR, fixed

@wangxin
Copy link
Copy Markdown
Collaborator

wangxin commented Jul 10, 2023

@twtseng-tim If this is not backport, probably the issue is also on master branch. Do we need to fix it in master branch again?

@twtseng-tim
Copy link
Copy Markdown
Contributor Author

twtseng-tim commented Jul 10, 2023

@twtseng-tim If this is not backport, probably the issue is also on master branch. Do we need to fix it in master branch again?

Yes

@wangxin
Copy link
Copy Markdown
Collaborator

wangxin commented Jul 13, 2023

@twtseng-tim In this case, the correct way is to create PR to master branch instead of creating PR to 202205 branch. Then we can cherry-pick it to 202205 branch. I am closing this PR. Please submit PR to master branch instead. Thanks!

@wangxin wangxin closed this Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yanjundeng yanjundeng yanjundeng left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@twtseng-tim @yanjundeng @wangxin