[dualtor]: Fix loopback route removal #6555

theasianpianist · 2022-10-17T21:11:49Z

Signed-off-by: Lawrence Lee lawlee@microsoft.com

Description of PR

Summary:
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?

Removing the peer switch loopback route with ip route commands does not persist since the route is learned over BGP

How did you do it?

Shutdown BGP sessions on the peer ToR to make sure the route is no longer advertised

How did you verify/test it?

Run the test and make sure that the route checks are passing.
Note that the test case is still expected to fail due to ongoing QOS issues

Any platform specific information?

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

Documentation

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

azure-pipelines · 2022-10-17T21:12:43Z

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/dualtor/test_orchagent_standby_tor_downstream.py fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/dualtor/test_orchagent_standby_tor_downstream.py:7:1: F811 redefinition of unused 'time' from line 3 tests/dualtor/test_orchagent_standby_tor_downstream.py:10:1: F403 'from tests.common.dualtor.dual_tor_mock import *' used; unable to detect undefined names tests/dualtor/test_orchagent_standby_tor_downstream.py:18:1: F401 'tests.common.fixtures.ptfhost_utils.copy_ptftests_directory' imported but unused tests/dualtor/test_orchagent_standby_tor_downstream.py:19:1: F401 'tests.common.fixtures.ptfhost_utils.change_mac_addresses' imported but unused tests/dualtor/test_orchagent_standby_tor_downstream.py:20:1: F401 'tests.common.fixtures.ptfhost_utils.run_garp_service' imported but unused ... [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>

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

azure-pipelines · 2022-10-17T23:28:21Z

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.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/dualtor/test_orchagent_standby_tor_downstream.py:9:1: F403 'from tests.common.dualtor.dual_tor_mock import ' used; unable to detect undefined names tests/dualtor/test_orchagent_standby_tor_downstream.py:9:1: F401 'tests.common.dualtor.dual_tor_mock.' imported but unused tests/dualtor/test_orchagent_standby_tor_downstream.py:301:121: E501 line too long (141 > 120 characters) tests/dualtor/test_orchagent_standby_tor_downstream.py:357:121: E501 line too long (122 > 120 characters)

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>

bingwang-ms · 2022-10-22T09:30:48Z

tests/dualtor/test_orchagent_standby_tor_downstream.py

@@ -191,28 +194,67 @@ def test_standby_tor_downstream_bgp_recovered(
    check_tunnel_balance(**params)


-def test_standby_tor_downstream_loopback_route_readded(rand_selected_dut, get_testbed_params, tbinfo):
+def route_matches_expected_state(duthost, route_ip, expect_route):
+    get_route_cmd = "ip route | grep {}".format(route_ip)


Please grep with -w to do a full word match.

bingwang-ms

Approved with a minor suggestion.

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

…anist/sonic-mgmt into standby-tor-route-fix

azure-pipelines · 2022-10-27T04:03:54Z

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.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 tests/dualtor/test_orchagent_standby_tor_downstream.py:9:1: F403 'from tests.common.dualtor.dual_tor_mock import ' used; unable to detect undefined names tests/dualtor/test_orchagent_standby_tor_downstream.py:9:1: F401 'tests.common.dualtor.dual_tor_mock.' imported but unused tests/dualtor/test_orchagent_standby_tor_downstream.py:301:121: E501 line too long (141 > 120 characters) tests/dualtor/test_orchagent_standby_tor_downstream.py:357:121: E501 line too long (122 > 120 characters)

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>

- Shutdown BGP sessions on the peer ToR to make sure the route is no longer advertised Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

Blueve · 2022-11-01T10:01:53Z

There is an import missing issue introduced to 202012 after this PR merged. Need revert 202012 backport.
@theasianpianist could you manually cherry-pick to 202012 and test it?

This reverts commit c79569b.

theasianpianist · 2022-11-01T10:09:33Z

There is an import missing issue introduced to 202012 after this PR merged. Need revert 202012 backport.
@theasianpianist could you manually cherry-pick to 202012 and test it?

Sure, can you share what the missing import is or what test case is affected?

Blueve · 2022-11-01T10:15:18Z

Thank you @theasianpianist
I got following errors during test collection stage (202012 branch):

==================================== ERRORS ====================================
___ ERROR collecting tests/dualtor/test_orchagent_standby_tor_downstream.py ____
ImportError while importing test module '/azp/agent/_work/16/s/sonic-mgmt-int/tests/dualtor/test_orchagent_standby_tor_downstream.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
dualtor/test_orchagent_standby_tor_downstream.py:27: in <module>
    from tests.common.dualtor.tor_failure_utils import shutdown_bgp_sessions  # noqa: F401
E   ImportError: cannot import name shutdown_bgp_sessions

What is the motivation for this PR? This reverts commit c79569b. This commit was cherry-picked to 202012 branch. On 202012 branch, this branch has importing issue in this line: tests/dualtor/test_orchagent_standby_tor_downstream.py: ``` from tests.common.dualtor.tor_failure_utils import shutdown_bgp_sessions # noqa: F401 ``` How did you do it? Revert commit c79569b

- Shutdown BGP sessions on the peer ToR to make sure the route is no longer advertised Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

…Azure/sonic-mgmt into internal Fix merge conflicts. Commit history: - Modified args for some of the calls (sonic-net#6605) - Run telemetry tests on all DUT types in a multi-dut setup (sonic-net#6098) - [dualtor]: Fix loopback route removal (sonic-net#6555) - Added support for one portChannel and one routed interface if two por… (sonic-net#5331) - Use config_reload when recovering from internal interfaces that are in down state (sonic-net#6076) - Added python fixture for resetting config changes to fix nightly backend snmp test failures (sonic-net#6607) - Fix Python3 RuntimeError - dictionary changed size during iteration (sonic-net#6646) - [mx] Add docs for mx topo (sonic-net#6634) - [topo_mx] Add new topo for BMC Mgmt ToR Router (sonic-net#6636) - [mx] Enhance announce routes to support MX topo (sonic-net#6635) - Merge branch 'azure-master' into dev/yaqiangzhu/internal_merge

[dualtor]: Fix loopback route removal

bb88a9a

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

theasianpianist added Request for 202012 branch Request for 202205 branch labels Oct 17, 2022

theasianpianist requested review from lolyu and bingwang-ms October 17, 2022 21:11

fix some pre-commit issues

3d7ebeb

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

bingwang-ms reviewed Oct 22, 2022

View reviewed changes

bingwang-ms previously approved these changes Oct 22, 2022

View reviewed changes

theasianpianist added 2 commits October 26, 2022 21:02

grep for whole word match

49b81bd

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

Merge branch 'standby-tor-route-fix' of https://github.com/theasianpi…

7a71c6d

…anist/sonic-mgmt into standby-tor-route-fix

theasianpianist dismissed bingwang-ms’s stale review via 7a71c6d October 27, 2022 04:03

theasianpianist requested a review from bingwang-ms October 27, 2022 04:05

bingwang-ms approved these changes Oct 27, 2022

View reviewed changes

theasianpianist merged commit 4d755d2 into sonic-net:master Oct 27, 2022

wangxin pushed a commit that referenced this pull request Oct 31, 2022

[dualtor]: Fix loopback route removal (#6555)

fdfa5c2

- Shutdown BGP sessions on the peer ToR to make sure the route is no longer advertised Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

wangxin added the Included in 202205 branch label Oct 31, 2022

wangxin pushed a commit that referenced this pull request Oct 31, 2022

[dualtor]: Fix loopback route removal (#6555)

c79569b

- Shutdown BGP sessions on the peer ToR to make sure the route is no longer advertised Signed-off-by: Lawrence Lee <lawlee@microsoft.com>

wangxin added the Included in 202012 branch label Oct 31, 2022

wangxin added a commit to wangxin/sonic-mgmt that referenced this pull request Nov 1, 2022

Revert "[dualtor]: Fix loopback route removal (sonic-net#6555)"

19e7a89

This reverts commit c79569b.

wangxin mentioned this pull request Nov 1, 2022

Revert "[dualtor]: Fix loopback route removal (#6555)" #6697

Merged

6 tasks

theasianpianist mentioned this pull request Nov 3, 2022

[202012][dualtor]: Fix loopback route removal #6700

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dualtor]: Fix loopback route removal #6555

[dualtor]: Fix loopback route removal #6555

theasianpianist commented Oct 17, 2022

azure-pipelines bot commented Oct 17, 2022

azure-pipelines bot commented Oct 17, 2022

bingwang-ms Oct 22, 2022

bingwang-ms left a comment

azure-pipelines bot commented Oct 27, 2022

Blueve commented Nov 1, 2022

theasianpianist commented Nov 1, 2022

Blueve commented Nov 1, 2022

[dualtor]: Fix loopback route removal #6555

[dualtor]: Fix loopback route removal #6555

Conversation

theasianpianist commented Oct 17, 2022

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

azure-pipelines bot commented Oct 17, 2022

azure-pipelines bot commented Oct 17, 2022

bingwang-ms Oct 22, 2022

Choose a reason for hiding this comment

bingwang-ms left a comment

Choose a reason for hiding this comment

azure-pipelines bot commented Oct 27, 2022

Blueve commented Nov 1, 2022

theasianpianist commented Nov 1, 2022

Blueve commented Nov 1, 2022