fix: prevent CNS duplicate-IP corruption by serializing managed IPAM and endpoint state - #4698
Open
Evan Baker (rbtr) wants to merge 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Evan Baker (rbtr)
marked this pull request as ready for review
August 13, 2026 21:55
Evan Baker (rbtr)
requested review from
Ashvin Deodhar (ashvindeodhar)
and
a lite review from Copilot
August 13, 2026 21:55
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Evan Baker (rbtr)
force-pushed
the
fix/cns-ipam-endpoint-transactions
branch
from
August 13, 2026 21:58
a96c1cf to
253f8cd
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens CNS IPAM correctness in managed endpoint-state mode by serializing endpoint state persistence with IP allocation/release under a single service lock, preventing ADD/DEL interleaving and improving rollback behavior when endpoint state persistence fails.
Changes:
- Introduces locked “transactional” paths for IP allocation + endpoint-state update, with rollback only for newly allocated IPs on endpoint-state write failure.
- Serializes DEL (endpoint-state removal + IP release) under the same lock to prevent ordering races.
- Adds deterministic unit tests covering endpoint-store write failures, canceled requests, and ADD/DEL ordering.
Show a summary per file
| File | Description |
|---|---|
| cns/restserver/ipam.go | Adds transactional, lock-held helpers for managed endpoint state and adjusts error/rollback behavior. |
| cns/restserver/ipam_test.go | Updates existing endpoint-state tests to go through handlers and adds new tests for rollback, cancellation, and concurrency ordering. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+119
to
+125
|
|
||
| service.Lock() | ||
| defer service.Unlock() | ||
|
|
||
| if err := ctx.Err(); err != nil { | ||
| return nil, fmt.Errorf("ip config request canceled: %w", err) | ||
| } |
Comment on lines
+424
to
+426
| <-blockingStore.entered | ||
| require.False(t, svc.TryLock(), "ADD must hold the service lock while endpoint state is persisted") | ||
|
|
Evan Baker (rbtr)
force-pushed
the
fix/cns-ipam-endpoint-transactions
branch
from
August 13, 2026 22:10
253f8cd to
ef29a39
Compare
Hold the CNS service lock across each managed endpoint-state ADD and DEL so IPAM ownership and endpoint persistence cannot interleave. Roll back only IPs newly allocated by the failing ADD, preserving existing idempotent assignments. Stop canceled queued ADDs before mutation. Factor lock-free internal helpers for the combined transaction and remove the obsolete exported/private wrappers that no longer have production callers. Add deterministic coverage for write failures and concurrent ADD/DEL ordering.
Evan Baker (rbtr)
force-pushed
the
fix/cns-ipam-endpoint-transactions
branch
from
August 14, 2026 17:57
ef29a39 to
2386f7a
Compare
Evan Baker (rbtr)
changed the title
Paul Johnston (pjohnst5)
approved these changes
Aug 19, 2026
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.
This is the fix for the duplicate-IP crashloop.
On master, a managed ADD spans two separate critical sections (assign the IP,
then write endpoint state) and a DEL spans two more (remove endpoint state, then
release the IP). CNI DEL can arrive before or during its ADD — containerd/Cilium
run DEL cleanup when the ADD times out — which allows this interleaving:
removeEndpointStatefinds nothing (ADD has not written yet) — no-op.container -> IP.Availablein memory.IPAM now says the IP is free while the endpoint statefile says it is owned. A
later pod legitimately takes the IP and writes a second endpoint record for it.
On restart
endpointStateToPodInfoByIPsees two owners and CNS refuses to startwith
duplicate IP detected in CNS initialization.Fix
Hold the service lock across the whole of each managed ADD and DEL, so IPAM
ownership and endpoint persistence can never interleave. With that, both
orderings leave consistent state and the split brain is impossible.
call; an idempotently returned existing assignment is never released.
*Untransactedinternals are factored out for the combinedtransaction. The lock-taking wrappers that still have callers
(
releaseIPConfigsfor the non-managed path,requestIPConfigsHelperforstartup reconcile) are retained.
GetIPFamilyCount/getIPFamiliesMapnow run inside the lock. They take nolock themselves, so there is no reentrancy, and this incidentally fixes an
unsynchronized read of
service.statethat existed on master.Tests
Validation
go test -race -count=1 ./cns/restserver/...