-
Notifications
You must be signed in to change notification settings - Fork 715
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
[dualtor] Implement dualtor T1 -> Standby Tor orchagent test cases. #3110
Conversation
This pull request introduces 2 alerts and fixes 1 when merging 26bef205c7317b6d4db61146bb62c94c11b95f74 into b4b6d21 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request fixes 1 alert when merging 80494d91a65a824eaaa6c1e9b81f3365c325e0df into b4b6d21 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 641537b23fd25fb4e1be7f11bfe16037ad5bffd4 into eb75a74 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 95098cfcc439c984e0228f6e660ef92063b58ef0 into ae8b02a - view on LGTM.com fixed alerts:
|
Signed-off-by: bingwang <[email protected]>
Signed-off-by: bingwang <[email protected]>
Signed-off-by: bingwang <[email protected]>
Signed-off-by: bingwang <[email protected]>
Signed-off-by: bingwang <[email protected]>
Overall looks good. Please include some test output in the PR description, and make the test filename more specific. Thanks! |
check_tunnel_balance(**params) | ||
|
||
|
||
def test_standby_tor_downstream_loopback_route_removed(ptfhost, rand_selected_dut, rand_unselected_dut, tbinfo): |
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 combine the loopback test cases into one case (similar to the other cases above)
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.
Updated. Thanks
Signed-off-by: bingwang <[email protected]>
1f3f20e
to
fe621f4
Compare
This pull request fixes 1 alert when merging d93dffa into 18dce2d - view on LGTM.com fixed alerts:
|
Signed-off-by: bingwang <[email protected]>
…mgmt into orchagent_crm
This pull request fixes 1 alert when merging 0058c43 into 18dce2d - view on LGTM.com fixed alerts:
|
@bingwang-ms Does your testcase cover TTL/DSCP (pipe/uniform mode) validation for both encap and decap cases? |
|
||
|
||
@pytest.fixture(scope='module') | ||
def apply_dualtor_subtype_to_th2(duthosts): |
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 field already gets added in apply_peer_switch_table_to_dut
, is it possible to just add the SWSS restart functionality to that fixture in case of TH2 ASIC?
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. I didn't realise the subtype
has been added in apply_peer_switch_table_to_dut
.
@gechiang Could you help to confirm if it's no hurt to add subtype: DualToR
for all platform, and does dualToR
equal to DualToR
? 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.
@bingwang-ms the entry in apply_peer_switch_table_to_dut
is supposed to be DualToR
, looks like I made a typo when I first wrote it 😁 would appreciate if you could fix that for me lol
I believe the subtype
field is needed by orchagent for all platforms, @prsunny can you confirm?
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.
@bingwang-ms
The subtype field if you are matching with case sensitive then it needs to match "DualToR".
I did a quick check on what may be impacted if subtype: "DualToR" is added blindly:
The following check for DUALTOR and perform Stopping MUX contaniner:
./src/sonic-utilities/scripts/reboot:SUBTYPE=$(sonic-cfggen -d -v DEVICE_METADATA.localhost.subtype)
The following during config Reload ARP/FDB entries are cached if DualToR:
./src/sonic-utilities/config/main.py: cache_arp_table = not disable_arp_cache and 'subtype' in localhost_metadata and localhost_metadata['subtype'].lower() == 'dualtor'
The following check for DualToR and Mux container is optionally enabled:
../files/build_templates/init_cfg.json.j2:{%- if include_mux == "y" %}{% do features.append(("mux", "{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %}enabled{% else %}always_disabled{% endif %}", false, "enabled")) %}{% endif %}
When DHCP docker started, if DualToR is configured, it enable some special options:
./dockers/docker-dhcp-relay/docker-dhcp-relay.supervisord.conf.j2:{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %} -U Loopback0 -dt{% endif -%}
So if your testcase is not involve in any of the above and you always restore it back (remove the subtype setting if it was set by your testcase only) then it is ok to set it for all platforms.
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.
@bingwang-ms the entry in
apply_peer_switch_table_to_dut
is supposed to beDualToR
, looks like I made a typo when I first wrote it 😁 would appreciate if you could fix that for me lolI believe the
subtype
field is needed by orchagent for all platforms, @prsunny can you confirm?
Sure. I will corretc 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.
Updated. Thanks
Yes. the TTL/DSCP validation is covered by another case implemented in https://github.com/Azure/sonic-mgmt/blob/master/tests/dualtor/test_ipinip.py |
Signed-off-by: bingwang <[email protected]>
This pull request fixes 1 alert when merging d714fac into d63b340 - view on LGTM.com fixed alerts:
|
""" | ||
Verify traffic is equally distributed via loopback route | ||
""" | ||
pt_require('dualtor' in tbinfo['topo']['name'], "Only run on dualtor testbed") |
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 want to be able to run this test on single ToR testbeds as well, what was the reasoning to mark this part as dual ToR only?
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 good catch. It should be test_standby_tor_downstream_loopback_route_readded
. I deleted the function defination in recent merge by mistake😁. I think test_standby_tor_downstream_loopback_route_readded
should run only on dualtor testbed.
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.
@prsunny can you confirm this case is dualtor only? I was under the impression all the orchagent cases were able to run on single ToR.
Signed-off-by: bingwang <[email protected]>
This pull request fixes 1 alert when merging 57116a2 into 99b0bae - view on LGTM.com fixed alerts:
|
Also could you change the filename to |
Signed-off-by: bingwang <[email protected]>
da539b8
to
413bfe0
Compare
This pull request fixes 1 alert when merging 413bfe0 into 84f004f - view on LGTM.com fixed alerts:
|
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!
…onic-net#3110) * Implement dualtor T1 -> Standby Tor test cases. Signed-off-by: bingwang <[email protected]>
…onic-net#3110) * Implement dualtor T1 -> Standby Tor test cases. Signed-off-by: bingwang <[email protected]>
Signed-off-by: bingwang [email protected]
Description of PR
Summary:
Fixes #3256
This PR implements dualtor t1->standby_tor orchagent test cases.
Test plan https://github.com/Azure/sonic-mgmt/blob/master/docs/testplan/dual_tor/dual_tor_orch_test_plan.md#test-cases
T1 -> Standby ToR
Send traffic of varying tuple destined to server under standby mux. The following are the various steps and those which have to be executed in a sequence is grouped together.
Type of change
Approach
What is the motivation for this PR?
This PR is to implement dualtor orchagent test cases.
How did you do it?
Please refer to code.
How did you verify/test it?
Verified on a dualtor testbed.
Verifying on a t0 testbed.
Any platform specific information?
No.
Supported testbed topology if it's a new test case?
No.
Documentation