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

[crm] Fix issues in static_route test case #3740

Closed
wants to merge 1 commit into from

Conversation

kevinskwang
Copy link
Collaborator

@kevinskwang kevinskwang commented Jul 7, 2021

What is the motivation for this PR?
There are two issues in the current static route test case:

  1. As all the ECMP entries are in the same subnet on the PTF, the PTF's route table will have three same route entries for this subnet. While sending a packet, the output interface is inexplicit within this subnet. Especially for ARP request, the interface will reply with their own MAC even the request is for other interfaces.
  2. While the config_reload_test option is True, the FDB will be flushed after reload. The packet will flood to all the interfaces in the Vlan1000. This may cause failures as other ports could receive the expected packet.

How did you do it?

  1. Add static neigh on the DUT for each PTF's interfaces, to avoid ARP request from DUT.
  2. Send ICMP packet from each PTF's interfaces to update FDB table on the DUT, to avoid flooding.

How did you verify/test it?
Run test_static_route.py, all the test cases should pass.

Description of PR

Summary:
Fixes # (issue)

Type of change

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

Back port request

  • 201911

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

@kevinskwang kevinskwang requested a review from a team as a code owner July 7, 2021 12:15
@kevinskwang kevinskwang requested a review from lolyu July 7, 2021 12:16
What is the motivation for this PR?
There are two issues in the current static route test case:
1. As all the ECMP entries are in the same subnet on the PTF, the PTF's route table will have three same route entries for this subnet.
   While sending a packet, the output interface is inexplicit within this subnet. Especially for ARP request, the interface will reply with their own MAC even the request is for other interfaces.
2. While the config_reload_test option is True, the FDB will be flushed after reload. The packet will flood to all the interfaces in the Vlan1000. This may cause failures as other ports could receive the expected packet.

How did you do it?
1. Add static neigh on the DUT for each PTF's interfaces, to avoid ARP request from DUT.
2. Send ICMP packet from each PTF's interfaces to update FDB table on the DUT, to avoid flooding.

How did you verify/test it?
Run test_static_route.py, all the test cases should pass.
@prsunny prsunny requested a review from shi-su July 7, 2021 15:50
Copy link
Contributor

@shi-su shi-su left a comment

Choose a reason for hiding this comment

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

I feel that some changes in this PR may not align with the aim of the test cases. I will conduct an analysis on the static feature and also the test, and provide a patch once done. Blocking this PR for now. Sorry for any confusion.

tests/route/test_static_route.py Show resolved Hide resolved
ip_ttl=64
)
# Send pkt to update DUT's FDB table to avoid flooding
testutils.send(ptfadapter, nexthop_devs[idx], pkt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this part does the neighbor resolution? If so, we probably need to avoid it since the automatic neighbor resolution is part of the static route feature, we need to check that. I think the current failures of the test come from two aspects: 1. the arp_update script has a period of 300 seconds, whereas the config reload waiting time is shorter than that; 2. according to sonic-net/sonic-buildimage#4930 the arp_update some times fails.

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, it is to fill the FDB entry for all interfaces on the DUT to avoid flooding unknown unicast packets. I think we may not need this changes anymore if we can wait until arp_update script is finished.

tests/route/test_static_route.py Show resolved Hide resolved
@shi-su
Copy link
Contributor

shi-su commented Jul 7, 2021

I created another PR #3749 to make sure that the arp_update script does the neighbor resolution. That should fix the failure for test_static_route_ecmp, yet test_static_route_ecmp_ipv6 may still fail due to sonic-net/sonic-buildimage#4930.

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