Compare PodCIDRs in NodeRouteController's addNodeRoute fast path - #8255
Open
pujitha24 wants to merge 1 commit into
Open
Compare PodCIDRs in NodeRouteController's addNodeRoute fast path#8255pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
Motivation: Node.Spec.PodCIDR is immutable once set, but NodeRouteController processes Node events through a workqueue keyed by Node name. This means addNodeRoute can observe the same Node name with a different PodCIDR in the following narrow, timing-dependent sequence: 1) Node "foo" with PodCIDR A is created and its routes/flows are installed. 2) Node "foo" is deleted. 3) A new Node "foo" (same name, same Node IPs) with PodCIDR B is created shortly after. If the Delete and Create events are coalesced in the workqueue, syncNodeRoute runs once, fetches the new Node "foo" from the lister, and calls addNodeRoute. The "already installed" fast path only compared Node MAC, Node IPs, and the WireGuard public key against the previously installed nodeRouteInfo, so it did not notice that the PodCIDR had changed and skipped reinstalling routes/flows, leaving the datapath with stale routes for PodCIDR A instead of B. Because this additionally requires the new Node to reuse the same Node IPs as the old one, the trigger conditions are narrow, but the missing comparison is a real correctness gap in the fast path. Approach: Add a samePodCIDRs comparison (using slices.EqualFunc against the podCIDRs recorded in the installed nodeRouteInfo) to the "already installed" fast-path condition in addNodeRoute, alongside the existing Node MAC, Node IP, and WireGuard public key checks. The podCIDRStrs computation (and its associated empty-PodCIDR early return) is moved earlier in the function so it is available for this comparison. Validation: Ran `go build ./pkg/agent/controller/noderoute/...` and `go vet ./pkg/agent/controller/noderoute/...`: both clean. Added TestNodeRecreateNewPodCIDR, which creates a Node with PodCIDR A, processes it, then deletes it and creates a same-named Node with PodCIDR B before the next queue item is processed (simulating the coalesced Delete+Create). Verified this test fails without the fix (missing calls to InstallNodeFlows/AddRoutes for PodCIDR B) and passes with it, by temporarily reverting only the non-test source file and rerunning: go test ./pkg/agent/controller/noderoute/... -run TestNodeRecreateNewPodCIDR -v Ran the full package suite with the fix applied and confirmed no regressions: go test ./pkg/agent/controller/noderoute/... ok antrea.io/antrea/v2/pkg/agent/controller/noderoute 3.260s Also ran `gofmt -l` on both changed files (no output) and `golangci-lint run ./pkg/agent/controller/noderoute/...` (0 issues). This is a controller unit-logic fix; no live-cluster or e2e reproduction was performed, and CONTRIBUTING.md does not require one for this class of change (`make test-unit` is the documented bar). Report: antrea-io#6965 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness gap in the Antrea Agent NodeRouteController fast path: when Node Delete+Create events for the same Node name are coalesced in the workqueue, addNodeRoute could previously treat the Node as “already installed” even if its PodCIDR(s) changed, leaving datapath programming stale.
Changes:
- Add PodCIDR(s) equality checking to
addNodeRoute’s “already installed” fast path (usingslices.EqualFunc). - Move PodCIDR extraction earlier in
addNodeRouteso it can be used by the fast-path condition. - Add a unit test covering same-name Node recreation with a different PodCIDR (coalesced Delete+Create scenario).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/agent/controller/noderoute/node_route_controller.go | Extends the fast-path “already installed” condition to include PodCIDR(s), preventing skipped reprogramming when PodCIDRs change under a reused Node name. |
| pkg/agent/controller/noderoute/node_route_controller_test.go | Adds a regression test for coalesced Delete+Create with a new PodCIDR under the same Node name. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+245
to
+246
| // The workqueue may still be empty by the time this is called, but processNextWorkItem will block. | ||
| c.processNextWorkItem() |
Comment on lines
+576
to
584
| // Route is already added for this Node and Node MAC, transport IP, podCIDRs and WireGuard | ||
| // public key are not changed. | ||
| if installed && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String() && | ||
| peerNodeIPs.Equal(*nrInfo.(*nodeRouteInfo).nodeIPs) && | ||
| samePodCIDRs(nrInfo.(*nodeRouteInfo).podCIDRs) && | ||
| nrInfo.(*nodeRouteInfo).wireGuardPublicKey == peerWireGuardPublicKey { | ||
| return nil | ||
| } | ||
|
|
Author
|
This has been open about a week with no conflicts against main — happy to make any changes that would help move review along, just let me know. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
Node.Spec.PodCIDR is immutable once set, but NodeRouteController
processes Node events through a workqueue keyed by Node name. This
means addNodeRoute can observe the same Node name with a different
PodCIDR in the following narrow, timing-dependent sequence:
installed.
created shortly after.
If the Delete and Create events are coalesced in the workqueue,
syncNodeRoute runs once, fetches the new Node "foo" from the lister,
and calls addNodeRoute. The "already installed" fast path only
compared Node MAC, Node IPs, and the WireGuard public key against the
previously installed nodeRouteInfo, so it did not notice that the
PodCIDR had changed and skipped reinstalling routes/flows, leaving
the datapath with stale routes for PodCIDR A instead of B.
Because this additionally requires the new Node to reuse the same
Node IPs as the old one, the trigger conditions are narrow, but the
missing comparison is a real correctness gap in the fast path.
Approach:
Add a samePodCIDRs comparison (using slices.EqualFunc against the
podCIDRs recorded in the installed nodeRouteInfo) to the "already
installed" fast-path condition in addNodeRoute, alongside the
existing Node MAC, Node IP, and WireGuard public key checks. The
podCIDRStrs computation (and its associated empty-PodCIDR early
return) is moved earlier in the function so it is available for this
comparison.
Validation:
Ran
go build ./pkg/agent/controller/noderoute/...andgo vet ./pkg/agent/controller/noderoute/...: both clean.Added TestNodeRecreateNewPodCIDR, which creates a Node with PodCIDR
A, processes it, then deletes it and creates a same-named Node with
PodCIDR B before the next queue item is processed (simulating the
coalesced Delete+Create). Verified this test fails without the fix
(missing calls to InstallNodeFlows/AddRoutes for PodCIDR B) and
passes with it, by temporarily reverting only the non-test source
file and rerunning:
go test ./pkg/agent/controller/noderoute/... -run TestNodeRecreateNewPodCIDR -v
Ran the full package suite with the fix applied and confirmed no
regressions:
go test ./pkg/agent/controller/noderoute/...
ok antrea.io/antrea/v2/pkg/agent/controller/noderoute 3.260s
Also ran
gofmt -lon both changed files (no output) andgolangci-lint run ./pkg/agent/controller/noderoute/...(0 issues).This is a controller unit-logic fix; no live-cluster or e2e
reproduction was performed, and CONTRIBUTING.md does not require one
for this class of change (
make test-unitis the documented bar).Report: #6965
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Fixes #6965