Conversation
Summary of ChangesHello @oilbeater, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a persistent warning in OVN northd ("No path for static route") that occurred during controller startup and node additions. The issue stemmed from static gateway routes being added before the corresponding OVN Logical Router Port for the join subnet was fully established, leading to unreachable next-hop errors. The solution refactors the route addition logic by centralizing default route reconciliation within the VPC worker, introducing a readiness check for the logical switch, and removing redundant route additions from the initialization and node handling phases. This ensures routes are only added when the OVN infrastructure is prepared, eliminating the warnings and streamlining the network configuration process. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the OVN northd warning on startup by refactoring the default route creation logic to avoid a race condition. The changes are clean and well-targeted. However, I've identified a potential regression in behavior. The removed addNodeGatewayStaticRoute function contained logic to avoid adding a default route if the user was managing static routes for the VPC. The new implementation in handleAddOrUpdateVpc appears to unconditionally add a default route, which could lead to conflicts with user-defined configurations. I've added a high-severity comment detailing this issue.
I am having trouble creating individual review comments. Click here to see my feedback.
pkg/controller/node.go (921-955)
By removing addNodeGatewayStaticRoute, the logic to avoid adding a default gateway route when a user provides their own static routes in the VPC spec is also removed. The previous implementation checked vpc.Spec.StaticRoutes and skipped adding a default route if it was user-managed. The new implementation in handleAddOrUpdateVpc unconditionally adds a default route for the default VPC. This could lead to conflicts with user-defined routes and represents a regression in behavior.
To fix this, the logic in handleAddOrUpdateVpc should be updated to check if a default route (0.0.0.0/0 or ::/0) already exists in vpc.Spec.StaticRoutes before adding its own.
Pull Request Test Coverage Report for Build 22316478569Details
💛 - Coveralls |
During controller startup, default gateway static routes (0.0.0.0/0 and ::/0) were added to the OVN logical router before the join subnet's Logical Router Port was created, causing OVN northd to warn about unreachable next hops. This fix ensures routes are only added after the join subnet's OVN Logical Switch and LRP exist: 1. Remove addNodeGatewayStaticRoute() call from syncNodeRoutes() which runs during init phase before any OVN logical switches are created. 2. Add LogicalSwitchExists check in handleAddOrUpdateVpc() to requeue the VPC if the join subnet's logical switch is not yet ready, avoiding the race between VPC worker and Subnet worker. 3. Remove redundant addNodeGatewayStaticRoute() call from handleAddNode() since handleAddOrUpdateVpc() already manages default route reconciliation as part of VPC route diffing. Also remove the now-unused function. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
b3b380e to
c0f6d01
Compare
Summary
addNodeGatewayStaticRoute()fromsyncNodeRoutes()(init phase) andhandleAddNode()where it ran before the join subnet's OVN Logical Router Port existed, causing OVN northd to warn:No path for static route 0.0.0.0/0 on router ovn-cluster; next hop 100.64.0.1LogicalSwitchExistscheck inhandleAddOrUpdateVpc()to requeue the default VPC if the join subnet's logical switch is not yet ready, preventing the race between VPC worker and Subnet workeraddNodeGatewayStaticRoute()function sincehandleAddOrUpdateVpc()already manages default route reconciliation via VPC route diffingRoot Cause
During controller startup, there were three places adding default gateway routes:
syncNodeRoutes()→addNodeGatewayStaticRoute()handleAddOrUpdateVpc()handleAddNode()→addNodeGatewayStaticRoute()The init phase call always triggered the warning. The VPC worker call could race with the Subnet worker.
Fix
On controller restart, the informer's initial list triggers
enqueueAddVpcfor the default VPC, and since OVN NB data is persistent, the join logical switch already exists — routes are added immediately without warnings.Test plan
make lintpasses (0 issues)make utpasses (all packages ok)No path for static routewarning in OVN northd logs after controller restart0.0.0.0/0,::/0) are present after fresh install and controller restart🤖 Generated with Claude Code