-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[everflow] Update everflow test plan to cover egress mirroring #428
Conversation
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 check the comments.
doc/acl/Everflow-test-plan.md
Outdated
- [Egress mirroring](#Egress-mirroring) | ||
- [Some existing areas not covered by the existing scripts](#Some-existing-areas-not-covered-by-the-existing-scripts) | ||
- [ACL rule for matching "IN_PORTS"](#ACL-rule-for-matching-IN_PORTS) | ||
- [IPv6 everflow](#IPv6-everflow) |
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 you add ICMP code/type test as well?
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.
@stcheng Does SONiC support matching ICMP type/code? If yes, could you please point me to relevant document/design/code? I need to know how to add ACL rules for matching ICMP type/code. Thanks!
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.
hello, yes https://github.com/Azure/sonic-swss/blob/cadc5bd8542d8691f3ce39f4d2569f786a558ff7/orchagent/aclorch.cpp#L343 although acl-loader
does not support icmp code/type, if you use configuration database, you could still program it.
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.
Thanks for the info! I have updated the test plan to cover ACL rules matching ICMP type and code.
doc/acl/Everflow-test-plan.md
Outdated
|
||
Each test case will analyze Everflow packet header and payload (if mirrored packet is equal to original). In case of egress mirroring, verify that TTL of the mirrored packet in GRE tunnel is decremented comparing with the injected packet. | ||
|
||
### Test case \#1 - Resolved route |
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.
I notice that some of the tests are pretty similar to each other; is it possible to merge some of the tests into single ones so that we could have a simplified list of tests.
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.
Agree. I have merged some of the test cases. Now we totally have 4 test cases.
In addition to covering ACL rules matching ICMP type and rule, I also added two more scenarios to be covered in the last commit:
|
|
||
##### IPv6 everflow | ||
The existing scripts only covered IPv6. IPv6 is also supported by SONiC now. The scripts need to be extended to cover IPv6 everflow too. To cover IPv6: | ||
* Everflow ACL table of type "MIRRORV6" needs to be defined and loaded during testing. |
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.
Are both the MIRROR and MIRRORv6 ACL Tables created together?
Some devices would be able to fit both IPv4 and IPv6 rules in the same ACL table; in this scenario creating two ACL tables for mirroring would result in wasting the h/w resources. Is there a way we can define based on the platform whether to have separate mirror ACL tables for v4 and v6?
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.
@rck-innovium For simplicity, the MIRROR and MIRRORv6 ACL tables can be created together in configuration/setup stage of the testing.
It's for testing everflow, as long as h/w resources are enough, it is not a issue. Why would we need to save h/w resources when we have enough of them? It's not for production.
If on platforms which hardware resources are not enough to do the testing, the script can create ACL table for testing each scenario. Then cleanup the ACL rules and tables after testing the scenario is completed.
This document is an updated version of the existing everflow test plan: https://github.com/Azure/SONiC/wiki/Everflow-test-plan
Purpose of this change is to cover the everflow egress mirroring capability added in #411. Some other enhancement is also included in this change.
Major changes: