Skip to content

fix default route for t0-isolated topo#22565

Merged
StormLiangMS merged 1 commit intosonic-net:masterfrom
sdszhang:bgp_default_route
Mar 13, 2026
Merged

fix default route for t0-isolated topo#22565
StormLiangMS merged 1 commit intosonic-net:masterfrom
sdszhang:bgp_default_route

Conversation

@sdszhang
Copy link
Copy Markdown
Contributor

@sdszhang sdszhang commented Feb 24, 2026

Description of PR

Summary:
In t0-isolated topo, the default route may come from PT0 or T1, and PT0 is preferred due to shorter AS path. Update the test case to suit it.

Fixes default route for t0-isolated topo.

route/test_default_route.py::test_default_route_with_bgp_flap FAILED     [ 75%]
......
        logging.info("peer intf ip from tb {}".format(upstream_neigh_ip))
>       pytest_assert(len(nexthops) == len(upstream_neigh_ip),
                      "Default route nexthops doesn't match the testbed topology")
E       Failed: Default route nexthops doesn't match the testbed topology

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • New Test case
    • Skipped for non-supported platforms
  • Test case improvement

Back port request

  • 202205
  • 202305
  • 202311
  • 202405
  • 202411
  • 202505
  • 202511

Approach

What is the motivation for this PR?

Fix default route announcement from PT0 in t0-isolated topo

How did you do it?

  • advertise default for PT0 neighbor.
  • update test_default_route.py test case to use PT0 only as upstream neighbor.

How did you verify/test it?

local testbed.

Any platform specific information?

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

Documentation

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@sdszhang
Copy link
Copy Markdown
Contributor Author

local test result on 202412:

route/test_default_route.py::test_default_route_set_src PASSED           [ 25%]
route/test_default_route.py::test_default_ipv6_route_next_hop_global_address PASSED [ 50%]
route/test_default_route.py::test_default_route_with_bgp_flap PASSED     [ 75%]
route/test_default_route.py::test_ipv6_default_route_table_enabled_for_mgmt_interface PASSED [100%]
......
=================== 4 passed, 2 warnings in 97.45s (0:01:37) ===================

@r12f
Copy link
Copy Markdown
Collaborator

r12f commented Feb 24, 2026

Will this break other tests?

@r12f
Copy link
Copy Markdown
Collaborator

r12f commented Feb 24, 2026

this is a fairly large change, and I would recommend run a full test run before commit this in.

@sdszhang
Copy link
Copy Markdown
Contributor Author

sdszhang commented Feb 25, 2026

this is a fairly large change, and I would recommend run a full test run before commit this in.

Hi @r12f , there is no new failure with this change: https://elastictest.org/scheduler/testplan/699da1eb32ebdf0ced87b34e

Copy link
Copy Markdown
Collaborator

@StormLiangMS StormLiangMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm the route generation callees (e.g. \generate_routes) accept \ or_default_route\ as a keyword argument — otherwise this will raise a \TypeError\ at runtime.

Copy link
Copy Markdown
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI agent on behalf of Ying: Note that this adds tor_default_route to the generate_routes call. I don’t see the corresponding parameter addition in the helper; please confirm the callee accepts this kwarg or update it, otherwise we’ll hit a TypeError at runtime. (Echoing StormLiangMS’ concern.)

Copy link
Copy Markdown
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI agent on behalf of Ying: Confirmed generate_routes already accepts tor_default_route in announce_routes.py; change looks good. Approving.

@yxieca
Copy link
Copy Markdown
Collaborator

yxieca commented Feb 27, 2026

@sdszhang there is a merge conflict, can you check?

Copy link
Copy Markdown
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. AI agent on behalf of Ying.

Copy link
Copy Markdown
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The PT-only neighbor filter for t0-isolated makes the default-route check match topology preference, and tor_default_route gating aligns announce_routes behavior. AI agent on behalf of Ying.

Copy link
Copy Markdown
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Change is scoped and aligns the test expectation with PT0-preferred default route behavior in t0-isolated. (AI agent on behalf of Ying)

yxieca
yxieca previously approved these changes Mar 1, 2026
Copy link
Copy Markdown
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. The PT-only upstream filter for t0-isolated aligns with the preferred default route behavior and the announce_routes tweak is consistent with existing tor_default_route handling.\n\nAI agent on behalf of Ying.

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Dashuai Zhang <dashuaizhang@microsoft.com>

# Conflicts:
#	tests/route/test_default_route.py
@sdszhang sdszhang force-pushed the bgp_default_route branch from 818e43d to dd6a486 Compare March 1, 2026 22:17
@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@sdszhang sdszhang added the Request for 202511 branch Request to backport a change to 202511 branch label Mar 8, 2026
Copy link
Copy Markdown
Collaborator

@r12f r12f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed with no pick to 202412. LGTM

@dayouliu1
Copy link
Copy Markdown
Contributor

local test result on 202412:

route/test_default_route.py::test_default_route_set_src PASSED           [ 25%]
route/test_default_route.py::test_default_ipv6_route_next_hop_global_address PASSED [ 50%]
route/test_default_route.py::test_default_route_with_bgp_flap PASSED     [ 75%]
route/test_default_route.py::test_ipv6_default_route_table_enabled_for_mgmt_interface PASSED [100%]
......
=================== 4 passed, 2 warnings in 97.45s (0:01:37) ===================

Just wondering if this test is passing for t0-isolated-d32u32s2 on 202412

@sdszhang
Copy link
Copy Markdown
Contributor Author

local test result on 202412:

route/test_default_route.py::test_default_route_set_src PASSED           [ 25%]
route/test_default_route.py::test_default_ipv6_route_next_hop_global_address PASSED [ 50%]
route/test_default_route.py::test_default_route_with_bgp_flap PASSED     [ 75%]
route/test_default_route.py::test_ipv6_default_route_table_enabled_for_mgmt_interface PASSED [100%]
......
=================== 4 passed, 2 warnings in 97.45s (0:01:37) ===================

Just wondering if this test is passing for t0-isolated-d32u32s2 on 202412

The test will pass on 202412 with the cherry-pick in Azure/sonic-mgmt.msft#1037, even though no routes are coming from PT0 in 202412 after add-topo.
This PR is to fix the behavior where the default route is not being advertised by PT0 neighbors.

Copy link
Copy Markdown
Collaborator

@StormLiangMS StormLiangMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@StormLiangMS StormLiangMS merged commit 1032966 into sonic-net:master Mar 13, 2026
27 of 28 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Mar 13, 2026
Description of PR
Summary:
In t0-isolated topo, the default route may come from PT0 or T1, and PT0 is preferred due to shorter AS path. Update the test case to suit it.

Fixes default route for t0-isolated topo.

route/test_default_route.py::test_default_route_with_bgp_flap FAILED     [ 75%]
......
        logging.info("peer intf ip from tb {}".format(upstream_neigh_ip))
>       pytest_assert(len(nexthops) == len(upstream_neigh_ip),
                      "Default route nexthops doesn't match the testbed topology")
E       Failed: Default route nexthops doesn't match the testbed topology

Signed-off-by: Dashuai Zhang <dashuaizhang@microsoft.com>
Signed-off-by: mssonicbld <sonicbld@microsoft.com>
@mssonicbld
Copy link
Copy Markdown
Collaborator

Cherry-pick PR to 202511: #22933

mssonicbld pushed a commit that referenced this pull request Mar 13, 2026
Description of PR
Summary:
In t0-isolated topo, the default route may come from PT0 or T1, and PT0 is preferred due to shorter AS path. Update the test case to suit it.

Fixes default route for t0-isolated topo.

route/test_default_route.py::test_default_route_with_bgp_flap FAILED     [ 75%]
......
        logging.info("peer intf ip from tb {}".format(upstream_neigh_ip))
>       pytest_assert(len(nexthops) == len(upstream_neigh_ip),
                      "Default route nexthops doesn't match the testbed topology")
E       Failed: Default route nexthops doesn't match the testbed topology

Signed-off-by: Dashuai Zhang <dashuaizhang@microsoft.com>
Signed-off-by: mssonicbld <sonicbld@microsoft.com>
selldinesh pushed a commit to selldinesh/sonic-mgmt that referenced this pull request Mar 16, 2026
Description of PR
Summary:
In t0-isolated topo, the default route may come from PT0 or T1, and PT0 is preferred due to shorter AS path. Update the test case to suit it.

Fixes default route for t0-isolated topo.

route/test_default_route.py::test_default_route_with_bgp_flap FAILED     [ 75%]
......
        logging.info("peer intf ip from tb {}".format(upstream_neigh_ip))
>       pytest_assert(len(nexthops) == len(upstream_neigh_ip),
                      "Default route nexthops doesn't match the testbed topology")
E       Failed: Default route nexthops doesn't match the testbed topology

Signed-off-by: Dashuai Zhang <dashuaizhang@microsoft.com>
Signed-off-by: selldinesh <dinesh.sellappan@keysight.com>
abhishek-nexthop pushed a commit to nexthop-ai/sonic-mgmt that referenced this pull request Mar 17, 2026
Description of PR
Summary:
In t0-isolated topo, the default route may come from PT0 or T1, and PT0 is preferred due to shorter AS path. Update the test case to suit it.

Fixes default route for t0-isolated topo.

route/test_default_route.py::test_default_route_with_bgp_flap FAILED     [ 75%]
......
        logging.info("peer intf ip from tb {}".format(upstream_neigh_ip))
>       pytest_assert(len(nexthops) == len(upstream_neigh_ip),
                      "Default route nexthops doesn't match the testbed topology")
E       Failed: Default route nexthops doesn't match the testbed topology

Signed-off-by: Dashuai Zhang <dashuaizhang@microsoft.com>
Signed-off-by: Abhishek <abhishek@nexthop.ai>
vrajeshe pushed a commit to vrajeshe/sonic-mgmt that referenced this pull request Mar 23, 2026
Description of PR
Summary:
In t0-isolated topo, the default route may come from PT0 or T1, and PT0 is preferred due to shorter AS path. Update the test case to suit it.

Fixes default route for t0-isolated topo.

route/test_default_route.py::test_default_route_with_bgp_flap FAILED     [ 75%]
......
        logging.info("peer intf ip from tb {}".format(upstream_neigh_ip))
>       pytest_assert(len(nexthops) == len(upstream_neigh_ip),
                      "Default route nexthops doesn't match the testbed topology")
E       Failed: Default route nexthops doesn't match the testbed topology

Signed-off-by: Dashuai Zhang <dashuaizhang@microsoft.com>
Signed-off-by: Venkata Gouri Rajesh Etla <vrajeshe@cisco.com>
ravaliyel pushed a commit to ravaliyel/sonic-mgmt that referenced this pull request Mar 27, 2026
Description of PR
Summary:
In t0-isolated topo, the default route may come from PT0 or T1, and PT0 is preferred due to shorter AS path. Update the test case to suit it.

Fixes default route for t0-isolated topo.

route/test_default_route.py::test_default_route_with_bgp_flap FAILED     [ 75%]
......
        logging.info("peer intf ip from tb {}".format(upstream_neigh_ip))
>       pytest_assert(len(nexthops) == len(upstream_neigh_ip),
                      "Default route nexthops doesn't match the testbed topology")
E       Failed: Default route nexthops doesn't match the testbed topology


Signed-off-by: Dashuai Zhang <dashuaizhang@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants