OCPBUGS-74261: Fix NNCP MaxUnavailableLimitReached deadlock with unavailable-slot audit - #1571
Open
mkowalski wants to merge 14 commits into
Open
OCPBUGS-74261: Fix NNCP MaxUnavailableLimitReached deadlock with unavailable-slot audit#1571mkowalski wants to merge 14 commits into
mkowalski wants to merge 14 commits into
Conversation
kubevirt-prow
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Member
Author
|
@emy this is for you, vibecoded OCPBUGS-74261. Feel free to review or feel free to drop it completely. |
There was a problem hiding this comment.
Pull request overview
Fixes NNCP deadlocks caused by stale maxUnavailable slots after interrupted handler operations.
Changes:
- Audits unavailable-slot counters against live enactments.
- Reclaims interrupted slots during handler startup.
- Adds unit, reconciliation, and end-to-end recovery tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/handler/main.go |
Reclaims interrupted enactments during startup. |
controllers/handler/nodenetworkconfigurationpolicy_controller.go |
Adds slot auditing, release ordering, and bounded requeues. |
controllers/handler/nodenetworkconfigurationpolicy_controller_test.go |
Tests slot claims, releases, and failures. |
pkg/enactmentstatus/conditions/conditions.go |
Exposes Progressing errors and marks interrupted enactments Pending. |
pkg/enactmentstatus/conditions/conditions_test.go |
Tests interrupted-enactment transitions. |
pkg/node/audit.go |
Implements live-holder auditing and counter repair. |
pkg/node/audit_test.go |
Tests audit semantics and stale thresholds. |
test/e2e/handler/nncp_slot_recovery_test.go |
Tests recovery after terminating a handler mid-apply. |
Suppressed comments (1)
controllers/handler/nodenetworkconfigurationpolicy_controller.go:333
- The success update error is discarded after the slot has already been released. If the NNCE status write exhausts its retries, the enactment remains Progressing and there is no explicit requeue; this controller does not watch NNCE updates or status-only NNCP updates, so the policy can remain Progressing indefinitely. A later reconcile also treats this stale Progressing condition as a held slot and applies without claiming. Return and handle the error while preserving a post-apply state that retries only finalization.
enactmentConditions.NotifySuccess(ctx)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // heartbeat must be before the audit considers its holder dead. It must | ||
| // exceed the worst-case apply cycle: | ||
| // DesiredStateConfigurationTimeout (8 min) + post-apply probes. | ||
| DefaultStaleEnactmentThreshold = 15 * time.Minute |
Comment on lines
+283
to
+285
| if didClaim { | ||
| if releaseErr := r.decrementUnavailableNodeCount(ctx, instance, generationKey); releaseErr != nil { | ||
| log.Error(releaseErr, "failed releasing just-claimed slot after Progressing write failure") |
Comment on lines
328
to
331
| if err := r.decrementUnavailableNodeCount(ctx, instance, generationKey); err != nil { | ||
| r.Log.Info("Failed to update NNCP status, will retry", "error", err, "requeueAfter", "10s") | ||
| r.Log.Info("Failed to release unavailable-node slot, will retry without re-applying", | ||
| "error", err, "requeueAfter", "10s") | ||
| return ctrl.Result{RequeueAfter: 10 * time.Second}, nil |
| Duration: 500 * time.Millisecond, | ||
| Factor: 2.0, | ||
| Jitter: 0.1, | ||
| Steps: 6, // 0.5+1+2+4+8+16 = ~31.5s cumulative |
| func MarkInterrupted(ctx context.Context, cli client.Client, enactmentKey types.NamespacedName, generationKey string) error { | ||
| return enactmentstatus.Update(ctx, cli, enactmentKey, | ||
| func(status *nmstate.NodeNetworkConfigurationEnactmentStatus) { | ||
| SetPending(&status.Conditions, interruptedByRestartMessage) |
Comment on lines
361
to
364
| if err := enactmentConditions.NotifyFinalizing(ctx); err != nil { | ||
| r.Log.Info("Failed to record finalizing phase, will retry", | ||
| "error", err, "requeueAfter", "10s") | ||
| return ctrl.Result{RequeueAfter: 10 * time.Second}, nil |
Comment on lines
+392
to
+401
| if err := enactmentConditions.NotifySuccess(ctx); err != nil { | ||
| // The slot is released, but success was not persisted. Do not swallow | ||
| // this: the enactment would stay Progressing and, because this | ||
| // controller watches neither NNCE updates nor status-only NNCP | ||
| // updates, nothing would re-trigger reconciliation and the policy | ||
| // would stay Progressing forever. Requeue so a later reconcile | ||
| // finalizes (records success) without re-applying. | ||
| r.Log.Info("Failed to record enactment success, will retry", | ||
| "error", err, "requeueAfter", "10s") | ||
| return ctrl.Result{RequeueAfter: 10 * time.Second} |
Comment on lines
+76
to
+80
| parsed, err := time.ParseDuration(raw) | ||
| if err != nil || parsed <= 0 { | ||
| return DefaultStaleEnactmentThreshold | ||
| } | ||
| return parsed |
Comment on lines
+377
to
+378
| if !enactmentstatus.IsProgressing(&enactment.Status.Conditions) { | ||
| continue |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
pkg/node/audit.go:85
- The documented safety rule says an override must exceed the worst-case apply cycle, but this comparison accepts equality. Since holders are considered stale at
age >= staleThreshold, an equal override can expire while the bounded apply is still completing (and leaves no allowance for the pre-apply gap or clock skew), allowing the audit to over-free a live slot. Reject equality as well.
if parsed < worstCaseApplyCycle {
return DefaultStaleEnactmentThreshold
test/e2e/handler/nncp_slot_recovery_test.go:48
- This deletion uses the pod's normal termination grace period and returns as soon as the API accepts the delete; it does not establish that the handler was killed before the in-flight apply completed. Consequently this regression spec can exercise only the normal success path and still pass, without testing startup reclaim/audit recovery. Force immediate termination (for example, zero grace) and wait for the old pod UID to disappear/replacement to start, or otherwise synchronize on an apply that remains in flight before asserting convergence.
ExpectWithOffset(1, testenv.Client.Delete(context.TODO(), pod)).To(Succeed())
| // probe.Run (sequential post-apply probes) <= ProbesTotalTimeout | ||
| // | ||
| // Derived from the source-of-truth timeouts so it tracks any change to them. | ||
| const worstCaseApplyCycle = nmstateclient.DesiredStateConfigurationTimeout + 2*probe.ProbesTotalTimeout |
AuditUnavailableSlots recomputes UnavailableNodeCountMap from live Progressing enactments instead of trusting blind +/-1 accounting, repairing ghost slots left by interrupted applies. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Resurrects the existing-but-unwritten status field so the slot audit can distinguish fresh counter activity from ghost slots. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
If the slot release fails, the enactment now truthfully stays Progressing instead of entering the Available+held-slot state that deadlocks the policy (OCPBUGS-74261 gap). The authoritative release retry budget grows to ~30s since it runs right after the node's own networking was reconfigured. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
The ordering spec and the pre-existing both-clients-fail decrement spec deliberately exhaust the authoritative release retry, sleeping through the full ~31.5s slotReleaseBackoff and growing the suite from ~2s to ~67s. Save/override/restore the backoff per spec (same pattern as the applyDesiredStateFn seam), bringing the suite back to ~2s. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
When the maxUnavailable cap refuses a claim, recompute the counter from live Progressing enactments and retry once, healing ghost slots at the moment they manifest. An enactment already Progressing for the current generation skips the claim (it holds the slot from an interrupted reconcile). Blocked reconciles requeue within 90-120s so recovery is bounded on quiet clusters (OCPBUGS-74261). Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Replaces blind stale-count decrements (and the #1542 !IsAvailable heuristic, which could over-decrement and violate maxUnavailable) with: mark this node's provably-dead Progressing enactments as interrupted, then recompute the policy counter from live enactments. The initial List is retried for ~2 minutes since the apiserver is often not ready in the post-reboot window this code targets. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Regression coverage for OCPBUGS-74261. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
NotifyProgressing swallowed its persistence error, so a node could hold a maxUnavailable slot with no live-holder marker; after the audit grace another node would repair the count and claim, violating maxUnavailable. NotifyProgressing now returns the error, and the reconcile releases the slot claimed in the same reconcile (best-effort) and requeues after 10s instead of applying. Also fix slotReleaseBackoff to Steps=6 to match the documented ~31.5s cumulative budget, and update the stale setupHandlerEnvironment doc comment to describe reclaimInterruptedSlots. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
…wFn stubs Add a spec asserting that a failed Progressing write after a successful slot claim releases the slot, skips apply, and requeues after 10s. Restore nmstatectlShowFn after each spec that stubs it so the stub does not leak across specs under --randomize-all. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
The slot-recovery spec waited for any Failing-or-Progressing enactment but always killed the handler on nodes[0], so it could pass vacuously. Now it finds a node whose enactment is Progressing=True and kills that node's handler pod. Also restructure AfterEach so policy deletion and node reset always run even when the absent-wait fails. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Addresses the automated review comments on this PR: - node: derive DefaultStaleEnactmentThreshold from the worst-case apply cycle instead of a flat 15m. The Progressing heartbeat is stamped once at apply start and never refreshed, and a successful apply can run ~20m (pre-apply probes + nmstatectl.Set + post-apply probes), so 15m could let the audit classify a still-applying node as stale, free its slot, and break maxUnavailable. It is now DesiredStateConfigurationTimeout + 2*ProbesTotalTimeout + 5m (~33m), derived from the source-of-truth timeouts. - handler: restore slotReleaseBackoff to Steps=7. wait.Backoff.Steps counts attempts and ExponentialBackoff does not sleep after the last attempt, so 6 steps yield only ~15.5s, not the documented ~31.5s; 7 steps give six sleeps (0.5+1+2+4+8+16). - enactmentstatus: mark restart-interrupted enactments Pending with a dedicated ConfigurationInterrupted reason instead of reusing MaxUnavailableLimitReached, which misled consumers about why the apply was waiting. - handler: release the slot claimed in a reconcile whose Progressing write failed using a bounded budget (~7.5s) that stays inside the 30s audit grace window. A longer retry could land after the grace expired and another node had already audited the markerless claim away and taken the slot, double-freeing it. If the bounded release fails, the set-to-truth audit reclaims the slot safely. - handler/enactmentstatus: persist a post-apply Finalizing phase (Progressing=True with a ConfigurationFinalizing reason). NotifySuccess now returns its error; if the slot release or the success write fails after a successful apply, the reconcile requeues and the retry short-circuits into finalizeApply, which releases the slot and records success WITHOUT re-applying the already committed configuration. This closes the swallowed-NotifySuccess deadlock (the enactment would stay Progressing forever, since the controller watches neither NNCE nor status-only NNCP updates) and the wasteful re-apply on the finalization retry. Unit tests added/updated (finalization retry does not re-apply; restart reason; derived threshold). go build, go vet, golangci-lint and the unit suites pass. Assisted-By: claude-opus-4-8 Signed-off-by: Mat Kowalski <mko@redhat.com>
Follow-up to the automated re-review; all four findings were on the new finalization/audit code: - node: reject a NMSTATE_ENACTMENT_STALE_THRESHOLD override below the worst-case apply cycle. Honoring an arbitrarily small value (e.g. 5m) reintroduced the very bug the derived default prevents: another node could classify a still-applying holder as stale and free its slot. The override is now clamped to the safe default when too small. - handler: make the finalization retry release idempotent. A success write that fails after the slot was already decremented left the enactment Finalizing; the retry decremented again and, when another node still held a slot, released that node's slot and broke maxUnavailable. The happy path keeps the fast blind decrement (race-free while still Progressing), but the retry path (finalizeInterruptedApply) now records success first and releases via the set-to-truth audit, which repairs the counter to the live-holder count and never double-frees. The Available short-circuit likewise reconciles the slot via the audit. - handler(startup): recover Finalizing enactments distinctly. A committed post-apply enactment is Progressing=True, so startup previously marked it Pending and reset its retry count, losing the no-reapply marker and forcing a needless re-apply. reclaimInterruptedSlots now finalizes such enactments (record success + idempotent audit release) instead, and only marks genuinely mid-apply (ConfigurationProgressing) enactments interrupted. - handler: give the NotifyFinalizing marker write the authoritative retry budget so a brief post-reconfigure API blip does not drop the no-reapply marker; only a sustained outage falls back to a safe, idempotent re-apply. Tests: add a double-free regression with an aggregate count > 1 (a second live holder) that the previous single-slot test could not catch; assert the override floor; and cover finalization convergence via the audit. go build, go vet, golangci-lint and the unit suites pass. Assisted-By: claude-opus-4-8 Signed-off-by: Mat Kowalski <mko@redhat.com>
mkowalski
force-pushed
the
nncp-maxunavailable-audit-pr
branch
from
August 13, 2026 17:54
9974200 to
c479fb7
Compare
kubevirt-prow
Bot
added
dco-signoff: yes
Member
Author
|
/retest Comment posted with AI assistance (via OpenCode). Please verify before acting on it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/node/audit.go:48
worstCaseApplyCycledoes not cover the full time that a live holder can remain insideApplyDesiredState. When a post-apply probe fails,ApplyDesiredStatecallsrollback, which runsprobe.Runa second time (pkg/client/client.go:137-145,170-172), adding up to anotherProbesTotalTimeout(and the rollback command itself has no explicit bound). The audit can therefore declare this node stale while it is still rolling back and admit another node, violatingmaxUnavailable. Please include the complete failure/rollback budget or refresh the holder heartbeat during these long phases.
const worstCaseApplyCycle = nmstateclient.DesiredStateConfigurationTimeout + 2*probe.ProbesTotalTimeout
pkg/node/audit.go:85
- The documented safety requirement is that an override must exceed the worst-case apply cycle, but this accepts equality. Since
countLiveHolderstreatsage >= staleThresholdas stale, an override exactly equal toworstCaseApplyCyclecan free a still-running holder at the boundary and allow more thanmaxUnavailablenodes to proceed.
if parsed < worstCaseApplyCycle {
return DefaultStaleEnactmentThreshold
…s delay
The audit-on-block commit made a refused maxUnavailable claim return a
fixed ~90-120s RequeueAfter. On a multi-wave rollout (e.g. 4 nodes,
maxUnavailable=50% -> 2 waves) every wave-2 node blocks for that whole
delay: the peer's slot is freed by a status-only NNCP update, which does
not trip this controller's generation-scoped watch, and there is no NNCE
watch, so the blocked node only re-checks on its own timer. That inflated
each e2e-handler spec from ~50s to ~300-450s, so the suite ran only
~20-24 of 104 specs before its 2h timeout, and pushed e2e-upgrade past
its 3m per-policy Available waits.
Restore the pre-audit behavior: return ctrl.Result{Requeue: true} so the
controller's per-item exponential backoff (NNCP_INITIAL_BACKOFF_SECONDS
-> NNCP_MAX_BACKOFF_SECONDS) retries within seconds. Ghost-slot recovery
stays bounded and is in fact tighter: every rate-limited retry re-runs
the set-to-truth audit-on-block (<= NNCP_MAX_BACKOFF, default 30s) rather
than waiting 90-120s. Remove the now-unused blockedRequeue* helpers and
the math/rand import; update the reconcile specs to assert Requeue: true.
Confirmed by log analysis: passing runs on other PRs average ~52s/spec
with a handful of >200s outliers, whereas every PR-1571 run (including the
pre-change head) showed ~60 inter-step gaps clustered at 90s+jitter(0-30s)
-- the exact blockedRequeue distribution -- totaling ~100m of pure sleep.
Assisted-By: claude-opus-4-8
Signed-off-by: Mat Kowalski <mko@redhat.com>
Member
Author
|
/test pull-kubernetes-nmstate-e2e-upgrade-k8s Comment posted with AI assistance (via OpenCode). Please verify before acting on it. |
|
@mkowalski: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
Is this a BUG FIX or a FEATURE?:
/kind bug
What this PR does / why we need it:
Fixes the NNCP
MaxUnavailableLimitReacheddeadlock family (OCPBUGS-74261, OCPBUGS-90538).UnavailableNodeCountMapis maintained by blind+1/-1around each apply; any interruption between the two (ungraceful reboot, handler crash, a failed status write right after the node reconfigured its own networking) leaves a ghost slot. Once the stale count reachesmaxUnavailable, every enactment is refused forever (Pending/"Waiting for progressing nodes to finish") and the only runtime path that clears the map — full policy success — is unreachable. Recreating the NNCP was the only recovery. #1427/#1542 heal some cases at handler startup, but nothing heals a ghost slot created while the handler keeps running, the startupListwas unretried (it fails silently exactly in the post-reboot window it targets), and the broadened!IsAvailablestartup decrement could over-free slots held by other nodes.This PR makes the counter self-auditing — enactments are the source of truth, the counter is only the lock:
cbd8607cf): the reconcile now decrements the slot before marking the enactmentAvailable, with a ~30s authoritative retry budget. A failed release leaves the enactment truthfullyProgressing; the poisonousAvailable+held-slot state is structurally unreachable. An enactment alreadyProgressingfor the current generation skips re-claiming (it holds the slot from an interrupted reconcile).8db393bb0,e1356f75f): when a claim is refused, the counter is recomputed from live holders (current-generation enactments withProgressing=Trueand a heartbeat younger thanNMSTATE_ENACTMENT_STALE_THRESHOLD, default 15m > worst-case apply cycle) and repaired set-to-truth — never blind decrements — guarded by a 30s grace window on the (previously dead)LastUnavailableNodeCountUpdatefield. Blocked reconciles requeue within 90–120s, so recovery is bounded even on a quiet cluster. Policies already deadlocked before an upgrade heal on their first blocked reconcile (nil timestamp = no grace).79f3b907b,0dbee3c0c): on handler start, this node's own still-Progressingenactments (provably dead) are markedPending("interrupted by handler restart"), their retry counts reset, and their policies audited. The initialListis retried ~2min. All blind-decrement cleanup (cleanStaleUnavailableCountsand friends) is deleted, eliminating the!IsAvailableover-free.d7677db8a): if theProgressingcondition cannot be persisted after a successful claim, the slot is released and the reconcile requeues — closing the one path where an invisible holder could be repaired away by another node's audit.614c63000,57034697f).No API schema changes;
maxUnavailablesemantics for healthy clusters are unchanged (the audit runs only when a claim is refused).Special notes for your reviewer:
applyDesiredStateFnis a test seam mirroring the existingnmstatectlShowFnpattern.Assisted-Bytrailer. Design/code were human-directed and the full unit suite plus vet were verified on the final tree. Please verify before acting on any claim herein.Release note: