fix: preserve PendingDisruption state across informer reconciles

[pujitha24]

Motivation:
In a static NodePool, when the disruption controller starts disrupting
a candidate NodeClaim it patches its DisruptionReason status condition
and calls NodePoolState.MarkNodeClaimPendingDisruption so the candidate
is excluded from the "Active" count while its replacement launches.
Shortly after, the NodeClaim informer controller reconciles that same
status patch and calls Cluster.UpdateNodeClaim, which unconditionally
called MarkNodeClaimActive whenever markedForDeletion was false -
clobbering PendingDisruption back to Active. This inflates the active
count past the NodePool's desired replica count, so the static
deprovisioner deletes the just-launched replacement NodeClaim and the
disruption/replace cycle stalls with repeated "replacement was
deleted, NodeClaim not found" errors.

Approach:
NodePoolState.UpdateNodeClaim now checks the NodeClaim's own
DisruptionReason status condition before re-marking it Active. While
the condition is true, the NodeClaim stays out of Active regardless of
how many times the informer reconciles it. Keying off the condition
(rather than internal PendingDisruption set membership) means this
self-heals correctly: if the disruption controller later abandons the
command and clears the condition via ClearNodeClaimsCondition, the
next informer reconcile of that patch calls MarkNodeClaimActive again,
so the NodeClaim doesn't get stuck PendingDisruption forever.

Validation:
Added a regression test in pkg/controllers/state/suite_test.go that
marks a NodeClaim PendingDisruption via the same public API the
disruption controller uses, then runs it through the real
NodeClaimController reconciler twice: once with the DisruptionReason
condition set (asserts it stays PendingDisruption) and once after the
condition is cleared (asserts it recovers to Active). Confirmed by
temporarily reverting statenodepool.go that this test fails
(pendingdisruption count reverts from 1 to 0) without the fix and
passes with it restored.

Ran:
KUBEBUILDER_ASSETS=<envtest 1.36.2 assets> go test
./pkg/controllers/state/... ./pkg/controllers/disruption/...
./pkg/controllers/static/... -race -timeout 20m
all packages pass. Also ran go build ./..., go vet
./pkg/controllers/state/..., gofmt -l on changed files (clean), and
golangci-lint-kube-api-linter run ./pkg/controllers/state/... (0
issues). Did not run the full make verify codegen/docgen pipeline
since this change touches no generated files, CRDs, or API types.

Report: #3250
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Assisted-by: claude-sonnet-5 (via Claude Code)

Fixes #3250

fix: preserve PendingDisruption state across informer reconciles

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pujitha24
Once this PR has been reviewed and has the lgtm label, please assign derekfrank for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ketanjani21]

Root cause is well-diagnosed. The self-heal design (keying off the condition rather than the internal set so ClearNodeClaimsCondition recovers to Active) is a good call. Three points before LGTM:

1. Invariant between the condition and the internal set

The disruption controller today writes two things when it starts disrupting a candidate: it patches the DisruptionReason condition, and it calls MarkNodeClaimPendingDisruption. This fix only checks the condition, so it works because those two writes are paired at the caller. That coupling is not enforced anywhere in the API, so a future caller could set one without the other and reintroduce the bug (or introduce a symmetric one where an internal PendingDisruption exists without the condition, so UpdateNodeClaim would still call MarkNodeClaimActive on it).

Two options to lock this down:

1. State the invariant here as a code comment: DisruptionReason=True implies the NodeClaim was already marked PendingDisruption via MarkNodeClaimPendingDisruption.
2. Extract a small helper like IsPendingDisruption(nodeClaim) used by both UpdateNodeClaim and the disruption controller. Both sides can only ever mean one thing, and the disruption controller's own tests can exercise the same predicate.

Would you consider (2)?

2. Comment scope vs code scope

The new code comment says (static NodeClaims only, see MarkNodeClaimPendingDisruption), but the condition check applies to any NodeClaim with DisruptionReason=True. If a dynamic NodeClaim ever gets DisruptionReason=True, this code path skips MarkNodeClaimActive for it too. Either drop the "static NodeClaims only" caveat from the comment, or add an explicit static-NodePool check if dynamic NodeClaims should still go through MarkNodeClaimActive here.

3. Test case for the deletion-vs-disruption ordering

The two test cases here cover the stay-put and recover paths. Could you add one more case for markedForDeletion=true while DisruptionReason=true? The code handles this correctly because the deletion branch returns before the condition check, but a test would lock that ordering in against future refactors.

[ketanjani21]

@pujitha24 - Thank you for quick turn around and addressing all the comments. The changes look good to me.

/assign
/cc @tallaxes @gjtempleton