Skip to content
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

[muxorch] Fixing updateRoute logic #2952

Merged
merged 3 commits into from
Nov 9, 2023
Merged

Conversation

Ndancejic
Copy link
Contributor

@Ndancejic Ndancejic commented Nov 2, 2023

What I did:

  • Removed logic that prevented routes from updating if an active nexthop was already found in updateRoute cache.
  • added test to check for scenario where ecmp route update triggers after updateRoute programs route to point to single nh
  • Added logs to some key paths in updateRoute logic to add visibility

Microsoft ADO - 25662690

Why I did it:
Bug in logic was causing routes getting updated multiple times to skip updateRoute logic

How I did it:
removing logic that checked cache for active nexthops

What I did:
- Removed logic that prevented routes from updating if an active nexthop
was already found in updateRoute cache.
- added test to check for scenario where ecmp route update triggers after
updateRoute programs route to point to single nh
- Added logs to some key paths in updateRoute logic to add visibility

Why I did it:
Bug in logic was causing routes getting updated multiple times to skip
updateRoute logic

How I did it:
removing logic that checked cache for active nexthop

Signed-off-by: Nikola Dancejic <ndancejic@microsoft.com>
@Ndancejic Ndancejic requested a review from prsunny as a code owner November 2, 2023 17:25
@@ -584,14 +584,14 @@ def multi_nexthop_test(self, dvs, dvs_route, asicdb, appdb, route, neighbors, ma
print("setting %s to %s" % (mux_ports[toggle_index], ACTIVE))
self.set_mux_state(appdb, mux_ports[toggle_index], ACTIVE)
if start[keep_index] == ACTIVE:
self.check_route_nexthop(dvs_route, asicdb, route, neighbors[keep_index])
self.check_route_nexthop(dvs_route, asicdb, route, neighbors[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is existing test changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic changed, so this test was failing. Each time we update the route, we'll be programming the route to point to the first active neighbor. Before this, we were keeping whatever nexthop was previously programmed.

@prsunny
Copy link
Collaborator

prsunny commented Nov 6, 2023

@Ndancejic , can you rebase with the latest 202205 and update the test?

Ndancejic and others added 2 commits November 6, 2023 22:15
Signed-off-by: Nikola Dancejic <ndancejic@microsoft.com>
@StormLiangMS
Copy link
Contributor

Hi @Ndancejic could you update ADO and test result for 202305?

@prsunny
Copy link
Collaborator

prsunny commented Nov 10, 2023

Hi @Ndancejic could you update ADO and test result for 202305?

@StormLiangMS , updated ADO. It is tested and validated on 202205.

@StormLiangMS
Copy link
Contributor

Hi @Ndancejic could you update ADO and test result for 202305?

@StormLiangMS , updated ADO. It is tested and validated on 202205.

hi @prsunny typo? 202305 or 202205?

@StormLiangMS
Copy link
Contributor

@Ndancejic @prsunny suppose typo, just merged.

StormLiangMS pushed a commit that referenced this pull request Nov 19, 2023
* [muxorch] Fixing updateRoute logic

What I did:
- Removed logic that prevented routes from updating if an active nexthop
was already found in updateRoute cache.
- added test to check for scenario where ecmp route update triggers after
updateRoute programs route to point to single nh
- Added logs to some key paths in updateRoute logic to add visibility

Why I did it:
Bug in logic was causing routes getting updated multiple times to skip
updateRoute logic
@StormLiangMS
Copy link
Contributor

hi @yxieca for 202311 cherry pick.

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.

5 participants