Skip to content

tests: Ensure that nexthop group and route are installed before conti…#22614

Open
donaldsharp wants to merge 1 commit into
FRRouting:masterfrom
donaldsharp:all_protocol_nhg_lookup
Open

tests: Ensure that nexthop group and route are installed before conti…#22614
donaldsharp wants to merge 1 commit into
FRRouting:masterfrom
donaldsharp:all_protocol_nhg_lookup

Conversation

@donaldsharp

Copy link
Copy Markdown
Member

…nuing

Currently test_all_protocol_startup.py is initiating route installation for a route that depends on other routes that were also installed by sharpd. Under very heavy load previous sharpd route installs have not finished and the recursive route is not properly being installed due to timing. Modify the code such that when looking for the routes nexthop group, that the route is actually installed. Also provide a way to ignore the need for the route installation for the one case where we are testing a route-map deny of the install.

…nuing

Currently test_all_protocol_startup.py is initiating route installation
for a route that depends on other routes that were also installed by
sharpd.  Under very heavy load previous sharpd route installs have
not finished and the recursive route is not properly being installed
due to timing.  Modify the code such that when looking for the routes
nexthop group, that the route is actually installed.  Also provide
a way to ignore the need for the route installation for the one
case where we are testing a route-map deny of the install.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
@frrbot frrbot Bot added the tests Topotests, make check, etc label Jul 8, 2026
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown

Greptile Summary

This PR makes the all-protocol startup topotest wait for route installation before using a route's nexthop group. The main changes are:

  • Adds an optional require_installed flag to route_get_nhg_id.
  • Requires Status: Installed before returning a nexthop group ID by default.
  • Disables that installed-status check for the route-map deny case.

Confidence Score: 4/5

The changed test flow looks mergeable after tightening the deny-route nexthop-group check.

  • The default helper behavior now waits for installed routes.
  • The deny-route exception can still return a shared nexthop group without proving it belongs to a non-installed route.

tests/topotests/all_protocol_startup/test_all_protocol_startup.py

Important Files Changed

Filename Overview
tests/topotests/all_protocol_startup/test_all_protocol_startup.py Updates the nexthop-group helper to wait for installed routes, with a deny-case exception that may still inspect a shared installed group.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
tests/topotests/all_protocol_startup/test_all_protocol_startup.py:1589
**Denied Route Can Reuse Installed Group**

When the denied route still reports a `Nexthop Group ID`, this opt-out allows `route_get_nhg_id` to return an ID without checking whether the route is installed. Since this route is installed with the same named nexthop group as the permitted route, `show nexthop-group rib <id>` can inspect the shared installed group and make the deny test fail on `Valid` or `Installed` even though the denied route itself was not installed.

Reviews (1): Last reviewed commit: "tests: Ensure that nexthop group and rou..." | Re-trigger Greptile

)

nhg_id = route_get_nhg_id(deny_route_str)
nhg_id = route_get_nhg_id(deny_route_str, require_installed=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Denied Route Can Reuse Installed Group

When the denied route still reports a Nexthop Group ID, this opt-out allows route_get_nhg_id to return an ID without checking whether the route is installed. Since this route is installed with the same named nexthop group as the permitted route, show nexthop-group rib <id> can inspect the shared installed group and make the deny test fail on Valid or Installed even though the denied route itself was not installed.

Rule Used: Do not flag test dependencies in FRRouting test su... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/all_protocol_startup/test_all_protocol_startup.py
Line: 1589

Comment:
**Denied Route Can Reuse Installed Group**

When the denied route still reports a `Nexthop Group ID`, this opt-out allows `route_get_nhg_id` to return an ID without checking whether the route is installed. Since this route is installed with the same named nexthop group as the permitted route, `show nexthop-group rib <id>` can inspect the shared installed group and make the deny test fail on `Valid` or `Installed` even though the denied route itself was not installed.

**Rule Used:** Do not flag test dependencies in FRRouting test su... ([source](https://app.greptile.com/frrouting/github/frrouting/frr/-/custom-context?memory=46f3e596-e95d-4f59-b33a-1c9746142882))

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

master size/S tests Topotests, make check, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant