[advanced-reboot] IO path verification fixes for dualtor by vaibhavhd · Pull Request #6968 · sonic-net/sonic-mgmt

vaibhavhd · 2022-12-06T07:14:19Z

Description of PR

Summary: Update checks for vlan and dut MAC in the bidirectional IO.
Fixes # (issue)

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?

Three improvements:

Present test_advanced_reboot crashes in examine_flow. This is due to packets are incorrectly matched in the code.
Additionally, added icmp_responder in the test, so that mux state is healthy during the duration of test.
Change mux config on both TORs to manual during the duration of test.

Fixes:

Identifying received packet in dualtor case -- t1->server rcvd pkt will have src MAC as vlan_mac, and server->t1 rcvd pkt will have src MAC as dut_mac
Identifying sent packet -- t1->server sent pkt will have dst MAC as dut_mac, and server->t1 sent pkt will have dst MAC as vlan_mac

How did you do it?

Added checks for vlan_mac
execute config mux mode manuall all during test, and revert to auto after the test.

How did you verify/test it?

Tested on physical testbed. The test does not crash anymore.
However, there are still some packet drops which points towards an image issue.

2022-12-01 23:32:22 : **************** Packet received summary: ********************
2022-12-01 23:32:22 : *********** Sent packets captured - 45000
2022-12-01 23:32:22 : *********** received packets captured - t1-to-vlan - 35919
2022-12-01 23:32:22 : *********** received packets captured - vlan-to-t1 - 8284
2022-12-01 23:32:22 : *********** Missed received packets - t1-to-vlan - 81
2022-12-01 23:32:22 : *********** Missed received packets - vlan-to-t1 - 716
2022-12-01 23:32:22 : **************************************************************
2022-12-01 23:32:22 : Disruptions happen between 0:00:08.393471 and 0:03:51.360718 after the reboot.
2022-12-01 23:32:22 : Total incoming packets captured 44203

2022-12-01 23:33:21 : The longest disruption lasted 0.045 seconds. 8 packet(s) lost.
2022-12-01 23:33:21 : Total disruptions count is 756. All disruptions lasted 4.434 seconds. Total 797 packet(s) lost

2022-12-01 23:33:37 : --------------------------------------------------
2022-12-01 23:33:37 : Fails:
2022-12-01 23:33:37 : --------------------------------------------------
2022-12-01 23:33:37 : FAILED:dut:Total downtime period must be less then 0:00:00 seconds. It was 4.43433070183

Any platform specific information?

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

Documentation

azure-pipelines · 2022-12-06T07:15:20Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/common/fixtures/advanced_reboot.py fix end of files.........................................................Failed - hook id: end-of-file-fixer - exit code: 1 - files were modified by this hook Fixing tests/platform_tests/test_advanced_reboot.py check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Failed - hook id: check-ast - exit code: 1 ... [truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

vaibhavhd · 2022-12-06T20:34:37Z

/Azp run

azure-pipelines · 2022-12-06T20:34:46Z

Azure Pipelines successfully started running 1 pipeline(s).

yxieca · 2022-12-06T23:43:27Z

@vaibhavhd can you update the PR comments, it should be dst mac instead of src mac in your 'fixes' section.

vaibhavhd · 2022-12-13T17:27:38Z

/Azp run

azure-pipelines · 2022-12-13T17:27:48Z

Azure Pipelines successfully started running 1 pipeline(s).

StormLiangMS · 2022-12-14T02:00:15Z

/azp run

azure-pipelines · 2022-12-14T02:00:24Z

Azure Pipelines successfully started running 1 pipeline(s).

wangxin · 2022-12-19T03:59:03Z

@vaibhavhd Cherry-picking this PR to 202205 branch hit similar issue.

Three improvements: Present test_advanced_reboot crashes in examine_flow. This is due to packets are incorrectly matched in the code. Additionally, added icmp_responder in the test, so that mux state is healthy during the duration of test. Change mux config on both TORs to manual during the duration of test. Fixes: Identifying received packet in dualtor case -- t1->server rcvd pkt will have src MAC as vlan_mac, and server->t1 rcvd pkt will have src MAC as dut_mac Identifying sent packet -- t1->server sent pkt will have dst MAC as dut_mac, and server->t1 sent pkt will have dst MAC as vlan_mac How did you do it? Added checks for vlan_mac execute config mux mode manuall all during test, and revert to auto after the test. How did you verify/test it? Tested on physical testbed. The test does not crash anymore. However, there are still some packet drops which points towards an image issue.

…-responder on single TOR (#7188) This PR fixes two recently introduced issues: Fix advanced reboot tests fail on non-dualtor testbeds #7146 caused by [advanced-reboot] IO path verification fixes for dualtor #6968 After merge of PR Fix pre-commit check errors in platform_tests/conftest.py #7049 , all testcases using get_advanced_reboot fixture are failing due to fixture 'get_advanced_reboot' not found How did you do it? To fix 7146, do not run GARP service and ICMP responder for non-dualtor advanced-reboot cases. I have not added a more simple skip for non-dualtor as these services are still needed for MOCK dualtor cases which run on single TOR testbeds. To fix 2, added a new import in the test scripts directly.

Signed-off-by: Neetha John <nejo@microsoft.com> What is the motivation for this PR? The following commit done as part of cherry-picking to 202012 appears bad (ab3d07d) This was trying to bring in changes from #6968 and #6994 How did you do it? Cleaned up the inadvertent changes How did you verify/test it? Ran the crm testcase "test_crm_nexthop" with the changes and it passes

Fixed "TypeError: __init__() takes exactly 8 arguments (7 given)" in test platform_tests/test_cont_warm_reboot.py which happen after merge community code. Issue introduced in PR: sonic-net#6968 - [advanced-reboot] IO path verification fixes for dualtor Signed-off-by: Petro Pikh <petrop@nvidia.com>

) Fixed "TypeError: __init__() takes exactly 8 arguments (7 given)" in test platform_tests/test_cont_warm_reboot.py which happen after merge community code. Issue introduced in PR: #6968 - [advanced-reboot] IO path verification fixes for dualtor - What is the motivation for this PR? Fix issue "TypeError: init() takes exactly 8 arguments (7 given)" - How did you do it? Added argument in method call - How did you verify/test it? Executed test which has been modified Signed-off-by: Petro Pikh <petrop@nvidia.com>

…nic-net#7322) Fixed "TypeError: __init__() takes exactly 8 arguments (7 given)" in test platform_tests/test_cont_warm_reboot.py which happen after merge community code. Issue introduced in PR: sonic-net#6968 - [advanced-reboot] IO path verification fixes for dualtor - What is the motivation for this PR? Fix issue "TypeError: init() takes exactly 8 arguments (7 given)" - How did you do it? Added argument in method call - How did you verify/test it? Executed test which has been modified Signed-off-by: Petro Pikh <petrop@nvidia.com>

…-responder on single TOR (#7188) This PR fixes two recently introduced issues: Fix advanced reboot tests fail on non-dualtor testbeds #7146 caused by [advanced-reboot] IO path verification fixes for dualtor #6968 After merge of PR Fix pre-commit check errors in platform_tests/conftest.py #7049 , all testcases using get_advanced_reboot fixture are failing due to fixture 'get_advanced_reboot' not found How did you do it? To fix 7146, do not run GARP service and ICMP responder for non-dualtor advanced-reboot cases. I have not added a more simple skip for non-dualtor as these services are still needed for MOCK dualtor cases which run on single TOR testbeds. To fix 2, added a new import in the test scripts directly.

…unt for sent and rcvd packets (#9599) Summary: Currently dual tor warmboot fails w/ control plane not ready for warmboot. This is because the test expects 2 responses for every ping packet sent to Loopback. This expectation is not correct after PR #9089 is merged. Update the testcase to now only expect 1 icmp response for every icmp packet sent to Loopback0. Updated the test to remove the earlier expectation of removing the packets that were sent by icmp_responder. This essentially reverts part of older PR: #6968 How did you verify/test it? Tested on a physical testbed and this test issue is not seen. The test is now able to see the responses of ping packets

…unt for sent and rcvd packets (sonic-net#9599) Summary: Currently dual tor warmboot fails w/ control plane not ready for warmboot. This is because the test expects 2 responses for every ping packet sent to Loopback. This expectation is not correct after PR sonic-net#9089 is merged. Update the testcase to now only expect 1 icmp response for every icmp packet sent to Loopback0. Updated the test to remove the earlier expectation of removing the packets that were sent by icmp_responder. This essentially reverts part of older PR: sonic-net#6968 How did you verify/test it? Tested on a physical testbed and this test issue is not seen. The test is now able to see the responses of ping packets

…unt for sent and rcvd packets (#9599) Summary: Currently dual tor warmboot fails w/ control plane not ready for warmboot. This is because the test expects 2 responses for every ping packet sent to Loopback. This expectation is not correct after PR #9089 is merged. Update the testcase to now only expect 1 icmp response for every icmp packet sent to Loopback0. Updated the test to remove the earlier expectation of removing the packets that were sent by icmp_responder. This essentially reverts part of older PR: #6968 How did you verify/test it? Tested on a physical testbed and this test issue is not seen. The test is now able to see the responses of ping packets

…unt for sent and rcvd packets (sonic-net#9599) Summary: Currently dual tor warmboot fails w/ control plane not ready for warmboot. This is because the test expects 2 responses for every ping packet sent to Loopback. This expectation is not correct after PR sonic-net#9089 is merged. Update the testcase to now only expect 1 icmp response for every icmp packet sent to Loopback0. Updated the test to remove the earlier expectation of removing the packets that were sent by icmp_responder. This essentially reverts part of older PR: sonic-net#6968 How did you verify/test it? Tested on a physical testbed and this test issue is not seen. The test is now able to see the responses of ping packets

…unt for sent and rcvd packets (#9599) (#10068) Raising PR for cherry-pick conflict: 225e257 [advanced-reboot][dualtor] Update Loopback0 ping test: expect same count for sent and rcvd packets (#9599) Summary: Currently dual tor warmboot fails w/ control plane not ready for warmboot. This is because the test expects 2 responses for every ping packet sent to Loopback. This expectation is not correct after PR #9089 is merged. Update the testcase to now only expect 1 icmp response for every icmp packet sent to Loopback0. Updated the test to remove the earlier expectation of removing the packets that were sent by icmp_responder. This essentially reverts part of older PR: #6968 How did you verify/test it? Tested on a physical testbed and this test issue is not seen. The test is now able to see the responses of ping packets Co-authored-by: Vaibhav Hemant Dixit <vaibhav.dixit@microsoft.com>

…unt for sent and rcvd packets (sonic-net#9599) Summary: Currently dual tor warmboot fails w/ control plane not ready for warmboot. This is because the test expects 2 responses for every ping packet sent to Loopback. This expectation is not correct after PR sonic-net#9089 is merged. Update the testcase to now only expect 1 icmp response for every icmp packet sent to Loopback0. Updated the test to remove the earlier expectation of removing the packets that were sent by icmp_responder. This essentially reverts part of older PR: sonic-net#6968 How did you verify/test it? Tested on a physical testbed and this test issue is not seen. The test is now able to see the responses of ping packets

vaibhavhd added 2 commits December 6, 2022 02:02

[advanced-reboot] IO path verification fixes for dualtor

441a436

Update dut and vlan MAC handling, and add comments

ebcd60e

vaibhavhd added Request for 202012 branch Request for 202205 branch labels Dec 6, 2022

vaibhavhd requested review from kellyyeh and yxieca December 6, 2022 07:14

vaibhavhd requested a review from sujinmkang as a code owner December 6, 2022 07:14

kellyyeh previously approved these changes Dec 6, 2022

View reviewed changes

yxieca previously approved these changes Dec 6, 2022

View reviewed changes

Update comments: correct the match reasons for received packet

a838c70

vaibhavhd dismissed stale reviews from yxieca and kellyyeh via a838c70 December 8, 2022 00:40

yxieca previously approved these changes Dec 8, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into dualtor-wb-fixes

40c7df3

kellyyeh previously approved these changes Dec 14, 2022

View reviewed changes

Fix indentation in line break causing PR test failures

99ac7e1

vaibhavhd dismissed stale reviews from kellyyeh and yxieca via 99ac7e1 December 16, 2022 01:35

vaibhavhd requested a review from kellyyeh December 16, 2022 19:19

kellyyeh approved these changes Dec 16, 2022

View reviewed changes

vaibhavhd merged commit 7bd0b8b into sonic-net:master Dec 17, 2022

vaibhavhd deleted the dualtor-wb-fixes branch December 17, 2022 04:58

wangxin added the Cherry Pick Conflict_202205 label Dec 19, 2022

antonptashnik mentioned this pull request Jan 3, 2023

advanced reboot tests fail on non-dualtor testbeds #7146

Closed

This was referenced Jan 4, 2023

[202012][advanced-reboot] IO path verification fixes for dualtor #7170

Merged

Fix test_advanced_reboot: re-add missing import; do not run garp/icmp-responder on single TOR #7188

Merged

[202205][advanced-reboot] IO path verification fixes for dualtor #7189

Merged

wangxin added Included in 202205 branch Included in 202012 branch and removed Cherry Pick Conflict_202012 Cherry Pick Conflict_202205 labels Jan 6, 2023

vaibhavhd mentioned this pull request Jan 6, 2023

Investigate: KVM PR test not catching failures in test_advanced_reboot #7201

Open

neethajohn mentioned this pull request Jan 20, 2023

[202012] Fix crm test failures due to bad cherry-pick #7305

Merged

6 tasks

ppikh mentioned this pull request Jan 24, 2023

Fixed "TypeError: __init__() takes exactly 8 arguments (7 given)" #7322

Merged

6 tasks

vaibhavhd mentioned this pull request Aug 22, 2023

[advanced-reboot][dualtor] Update Loopback0 ping test: expect same count for sent and rcvd packets #9599

Merged

6 tasks

mssonicbld mentioned this pull request Aug 29, 2023

[action] [PR:9599] [advanced-reboot][dualtor] Update Loopback0 ping test: expect same count for sent and rcvd packets #9727

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[advanced-reboot] IO path verification fixes for dualtor#6968

[advanced-reboot] IO path verification fixes for dualtor#6968
vaibhavhd merged 5 commits intosonic-net:masterfrom
vaibhavhd:dualtor-wb-fixes

vaibhavhd commented Dec 6, 2022 •

edited

Loading

Uh oh!

azure-pipelines bot commented Dec 6, 2022

Uh oh!

vaibhavhd commented Dec 6, 2022

Uh oh!

azure-pipelines bot commented Dec 6, 2022

Uh oh!

yxieca commented Dec 6, 2022

Uh oh!

vaibhavhd commented Dec 13, 2022

Uh oh!

azure-pipelines bot commented Dec 13, 2022

Uh oh!

StormLiangMS commented Dec 14, 2022

Uh oh!

azure-pipelines bot commented Dec 14, 2022

Uh oh!

wangxin commented Dec 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

vaibhavhd commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Back port request

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

Uh oh!

azure-pipelines bot commented Dec 6, 2022

Uh oh!

vaibhavhd commented Dec 6, 2022

Uh oh!

azure-pipelines bot commented Dec 6, 2022

Uh oh!

yxieca commented Dec 6, 2022

Uh oh!

vaibhavhd commented Dec 13, 2022

Uh oh!

azure-pipelines bot commented Dec 13, 2022

Uh oh!

StormLiangMS commented Dec 14, 2022

Uh oh!

azure-pipelines bot commented Dec 14, 2022

Uh oh!

wangxin commented Dec 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vaibhavhd commented Dec 6, 2022 •

edited

Loading