-
Notifications
You must be signed in to change notification settings - Fork 15
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
[SDFAB-221] Make PTF tests more readable #268
Conversation
- Including new function get_test_args - Reads in high level parameters from actual test and generates test test arguments - Makes tests more readable by simplifying repeated code
Codecov Report
@@ Coverage Diff @@
## main #268 +/- ##
=========================================
Coverage 69.69% 69.69%
Complexity 467 467
=========================================
Files 37 37
Lines 3260 3260
Branches 333 333
=========================================
Hits 2272 2272
Misses 813 813
Partials 175 175 Continue to review full report at Codecov.
|
redesign - Created one giant for-loop for return dictionaries - Changed appending and returning a list into yielding dictionaries - Populate lists in order of for-loop complexity from top to bottom
- Changed FabricSpgwDownlinkTest in test.py to match high-level param model - Fixed list naming model to match consistent style
working for FabricSpgwDownlinkTest - Changed self.get_test_args call to get_test_args (causing error in Jenkins)
specification - One high level variable called traffic_dir that takes in string input of model "source-device-destination" - Implementation then parses input into variables source, device, dest - vlan_conf_list and other such lists are now specified based off of this parameter rather than multiple variables like device_type and traffic_dir being separate
ptf/tests/ptf/fabric_test.py
Outdated
""" SEND_REPORT_TO_SPINE """ | ||
# Check if INT is specified and a spine is specified in traffic_dir | ||
# TODO: what about if source is a spine? | ||
if int_test_type in INT_OPTIONS and is_next_hop_spine_list[0] != None: | ||
send_report_to_spine_list = [False, True] | ||
else: | ||
send_report_to_spine_list = [None] |
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 believed that this will always be [False, True]
if it is an INT test.
Consider a topology like a figure below, host1 sends packets to host2 (red arrow), here we have three traffic direction in this case: host1-leaf1-spine1
, leaf1-spine1-leaf2
, and spine1-leaf2-host2
Every device in this red path will generate an INT report and send it to the collector on the leaf1 (orange arrows)
- For
host1-leaf1-spine1
, leaf1 sends the INT report to the collector directly - For
leaf1-spine1-leaf2
, spine1 sends the INT report to leaf1 and leaf1 will help forward it to the collector - For
spine1-leaf2-host2
, leaf2 sends the INT report to spine1, and spine1 will help to forward it to the collector via leaf1
Also, when sending packets from host2 to host1, the leaf2 will also send an INT report to the spine. (`host2-leaf2-spine1)
This tells us that send_report_to_spine_list
will be [False, True]
if traffic_dir
is host-leaf-spine
or spine-leaf-host
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 case is to have a remote cluster/POD with an INT collector.
For every type of traffic in the local culster (host-leaf-spine
, leaf-spine-leaf
, spine-leaf-host
, and host-leaf-host
), the device(leaf or spine) will send the INT report to another spine.
I didn't draw
host-leaf-host
traffic, but it is similar tohost-leaf-spine
case.
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.
Firstly, thank you for this explanation, I appreciate the effort you went through to make your reasoning clear.
I understand the reasoning behind the first example. However, I'm confused by the second example. In the first one, you state that send_report_to_spine = [True, False]
if traffic_dir
is host-leaf-spine
or spine-leaf-host
. However, in this separate example, send_report_to_spine = [True, False]
regardless of traffic_dir
, as long as it is an INT test.
Which is it? Is send_report_to_spine = [True, False]
just as long as it is an INT test, or is there a specific topology in which this is the case, and other topologies in which it is not?
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.
Right, send_report_to_spine
will always be [True, False]
if it is an INT test since we need to consider all possible cases.
The traffic_dir
won't really affect the value of send_report_to_spine
Co-authored-by: Yi Tseng <a86487817@gmail.com>
- Didn't call .items() when looping through vlan_confs dict
- Cleanup spacing and remove obvious comments - Remove loops for consistent singular test arguments - Check correct traffic direction devices with const option lists - Yield more test arguments
* Added allow list to get_test_args for-loop generation * Added new fields to print statement in for loop * Question about lines 150-156, packet attribute specifications
* Modify get_test_args to parse packet addresses and pass in loop * Modify SPGW test cases to include new address specifications
* Remove consideration of traffic direction from conditional
* Modify get_test_args to accomodate for INT test tweaks * Clean unused paramaters from INT+SPGW test doRunTest function * Make pkt_addrs parameter in get_test_args not required since SPGW+INT tests don't use the convention for their packets
* Only access pkt_addrs dictionary for non-INT tests * Stop printing actual pkt while test is running by adding pkt after print * Change params in INT doRunTest to access tagged1 + tagged2 instead of just tagged
* Implement INT SPGW non-drop tests * Implement INT SPGW drop tests
* Move OPTIONS constants from get_test_args to top of file with other constants * Add documentation for get_test_args function * Rename vague allow param to ue_recirculation * Fixed: include **kwargs in doRunTest for INT SPGW 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.
some comments, but lgtm overall
* Implement conditional psc flag for SPGW tests * Add some more traffic directions for tests that weren't testing enough device types (i.e., include spine type for destination device for some tests)
…-tna into readable-ptf
* Some INT tests follow unreadable for-loop structure, convert these to readable format * Change default packet list to include VXLAN packet types
Here are some summaries after looking at all parameters carefully.
Here is the diff, compare with only common parameters: The SCTP issue:
The
|
@ccascone Can you take a look at the test coverage in my comment? Just want to make sure it is ok to skip SCP packet type in all SPGW tests. Thanks |
I won't be able to do a proper review until Sunday, but if understand your question we don't have a requirement to support GTPU encap/decap of SCTP packets. SCTP is used for control traffic between eNB and MME, UEs are not supposed to generate/receive SCTP. |
@Yi-Tseng please feel free to merge this, don't wait for me. If necessary, I can do a post-merge review. |
[SDFAB-183]
Pull request building more readable PTF tests by creating a function generating test arguments in fabric_test.py which takes in high level parameters and returns dictionaries of various test configurations