Feat/nodedrain failure domain gate - #387
Open
geoffrey1330 wants to merge 15 commits into
Open
Conversation
wmousa
force-pushed
the
feat/nodedrain-failure-domain-gate
branch
from
August 6, 2026 12:40
43bde05 to
dc47ecb
Compare
wmousa
force-pushed
the
feat/nodedrain-failure-domain-gate
branch
from
August 11, 2026 12:32
a52c01c to
bf9889c
Compare
…count With failure domains enabled, placement guarantees at most one erasure-coding chunk per domain, so losing an entire domain at once is already tolerated the same way losing a single node is tolerated without FD � the unit maxFaultTolerance counts against becomes the domain, not the node. handleDetected now counts distinct active failure domains (via StorageNodeSet.spec.nodeFailureDomains) instead of raw drain count when FD is enabled: a candidate whose domain already has an active drain (planned or a pre-existing unhealthy node) may proceed regardless of the raw node count; a candidate that would add a new domain is gated on the distinct-domain count. Falls back to the existing node-count gate when FD is disabled or a node has no domain assignment.
…rkerFailureDomain source change
…count With failure domains enabled, placement guarantees at most one erasure-coding chunk per domain when there are at least ndcs+npcs distinct domains, so losing an entire domain at once is already tolerated the same way losing a single node is tolerated without FD � the unit maxFaultTolerance counts against becomes the domain, not the node. handleDetected now counts distinct active failure domains (via StorageNodeSet.spec.nodeFailureDomains) instead of raw drain count when FD is enabled: a candidate whose domain already has an active drain (planned or a pre-existing unhealthy node) may proceed regardless of node count; a candidate that would add a new domain is gated on the distinct-domain count against maxFaultTolerance. When the cluster has fewer domains than ndcs+npcs (meets the npcs+1 activation minimum but not full one-chunk-per-domain isolation), at least one domain necessarily carries more than one stripe chunk, so losing two different domains at once can no longer be assumed safe. In that case a candidate may still fully drain whichever single domain is already active, but opening a second distinct domain falls back to the plain node-count budget instead of a second free domain slot. Falls back to the existing node-count gate entirely when FD is disabled or a node has no domain assignment.
fdDrainGate previously only tracked whether a domain had ANY active drain (activeDrainDomains, presence-only) and gated a second domain on raw node count once one was active. Confirmed against the backend team's stated requirements for 2/3/4-domain 2+2 layouts, that allowed unsafe combinations (e.g. one node in FD1 plus two more in FD2 on a 2-domain cluster) -- piling onto an active domain isn't free once that domain has already absorbed more chunks than it can safely re-lose. Replaced with a per-domain risk budget of npcs: each domain's contribution to the risk is capped at chunksPerDomain = ceil((ndcs+npcs)/domainsAvailable) -- the worst-case chunk count any single domain can hold under placement's even-spread strategy. A domain already at or above that count has maxed its contribution, so further nodes in the same domain are free; otherwise the summed capped risk across all affected domains plus the candidate must stay within npcs. This mirrors simplyblock_core's _check_ftt_allows_node_removal formula exactly, so the two stay in lockstep. activeDrainDomains (presence-only) replaced by activeDrainDomainCounts (per-domain counts, what the risk formula needs). Added ParityChunksFromErasureCodingScheme (npcs) alongside the existing RequiredNodesFromErasureCodingScheme (ndcs+npcs) to source both halves of the formula from the cluster's erasureCodingScheme.
reconcileActivate fired POST /activate as soon as Spec.Action==Activate and the cluster UUID resolved, with no check on node count, domain count, or balance. If the backend correctly refused an under-provisioned FD cluster (fewer than npcs+2 distinct domains, or unequal per-domain host counts -- see simplyblock_core's fd_activation_domain_count_violation), the CR got permanently stuck: failActivate sets ActionStatus.State=Failed, and nothing in Reconcile/reconcileActivate ever resets ActionStatus away from Failed back to Running, so the next reconcile falls straight through to the GET-polling tail and loops forever without ever retrying the POST. Added a readiness check at the very top of reconcileActivate, before any ActionStatus mutation: for FD-enabled clusters, aggregate each host's failure domain across every StorageNodeSet belonging to the cluster (clusterFailureDomainHosts) and validate domain count/balance (fdActivationDomainCountViolation, mirroring the Python-side check exactly). On failure, just requeue -- ActionStatus is left completely untouched, so there's nothing to get stuck once enough domains show up. No-op for FD-disabled clusters, confirmed via the existing activate tests passing unchanged.
reconcileActivate (StorageCluster controller) already refuses to POST /activate until fdActivationDomainCountViolation clears, but there is a second, independent path that fires the same POST: the StorageNodeSet controller's maybeActivateCluster, triggered whenever ShouldActivateCluster's online/healthy node count matches the erasure coding scheme. That check has no notion of failure domains at all, so on a live 2+2/3-domain deployment it kept firing /activate every reconcile, and the backend's own defense-in-depth check (fd_activation_domain_count_violation) synchronously rejected and reverted it -- observed on the GCP OKD cluster as a repeating unready -> in_activation -> unready cycle roughly every 7 minutes. Add the same npcs+2-domain gate to maybeActivateCluster, reusing clusterFailureDomainHosts/fdActivationDomainCountViolation so the two call sites stay in lockstep. No-op for FD-disabled clusters.
…ciler Rebasing this branch onto main hit a real conflict, not a mechanical one: main merged #397 ("design ClusterOps CR to decouple cluster operations from the StorageCluster reconciler"), which removed reconcileActivate/ reconcileExpand/failActivate/failExpand entirely from simplyblockstoragecluster_controller.go and moved them into a new StorageClusterOpsReconciler (storageclusterops_controller.go). The failure-domain activation-readiness gate added earlier in this branch was written against the old location, which no longer exists. Ported the same gate (clusterFailureDomainHosts + fdActivationDomainCountViolation, unchanged and kept in the old file -- maybeActivateCluster in simplyblockstoragenodeset_controller.go depends on them too) into the top of the new StorageClusterOpsReconciler. reconcileActivate, before the activate POST ever fires, preserving the original semantics: ops.Status is left completely untouched on failure, just requeue and re-check. Also fixed maybeActivateCluster's now-stale cross-reference comment pointing at the old file/function. Dropped the old file's now-dead reconcileActivate/reconcileExpand/ failActivate/failExpand and their unit tests (they test a signature and state machine that no longer exists post-#397), and ported the two FD-gate-specific tests (waits-for-readiness / proceeds-once-ready) to storageclusterops_controller_unit_test.go against the new reconciler, following that file's existing test conventions (the "proceeds" case now asserts it reaches the real, no-backend-available activate attempt and fails there, rather than staying stuck on the readiness check -- same pattern already used by TestStorageClusterOps_AcquiresLockAndTransitionsOutOfPending).
…ot-ready blip reconcileSpdkProxyEndpointSlices deleted a StorageNodeSet's per-port spdk-proxy EndpointSlice the moment isSpdkProxyPodReady returned false for its pod in a single reconcile pass -- no debounce, no consecutive- failure counter. isSpdkProxyPodReady requires BOTH containers' ContainerStatus.Ready simultaneously, and that condition can flip false for one missed/slow probe tick well short of whatever failureThreshold would actually restart the container or emit an Unhealthy event -- so a live, healthy, never-restarted pod could still lose its EndpointSlice for the one reconcile where the check landed on a bad tick (matches watch-triggered reconciles firing exactly on Ready-condition changes). Losing the slice means CoreDNS has nothing to resolve for that node's StorageNode.rpc_client() hostname (used instead of the plain mgmt IP specifically for TLS SNI/cert-hostname matching), producing a NameResolutionError against a node that was never actually down. This matches the circumstantial evidence from the 2026-08-06 incident (rlhzx: continuous uptime, zero restarts, zero Unhealthy events, yet its own hostname transiently failed to resolve) that was never conclusively nailed down at the time. Split the "does this port have a live pod" check from the "is that pod ready" check: portsWithAnyPod is computed from every pod regardless of readiness (RPC_PORT is a static env var, readable from pod.Spec the moment it's scheduled, well before any readiness probe runs). The delete pass now only removes a slice when its port has NO pod at all -- genuinely gone (scaled down, node removed) -- not merely not-ready. A transiently-not-ready pod simply keeps its last-known-good EndpointSlice in place (stale but still correct) until the next reconcile finds it ready again and refreshes it via the existing create/update pass.
wmousa
force-pushed
the
feat/nodedrain-failure-domain-gate
branch
from
August 12, 2026 09:32
03d97aa to
c913423
Compare
drainValidate only checked for pinned/unmanaged volumes before advancing to Suspending. Nothing checked whether removing the node would violate the cluster's failure-domain balance rule -- that was left entirely to the backend's own admission check (check_fd_admission_for_remove), which only runs much later, in drainRemove's DELETE call. Confirmed live (2026-08-13): a node whose removal would have dropped a failure domain out of balance got suspended anyway (Suspending runs before Removing), then the backend correctly refused the DELETE -- but by then the node was already suspended, with the reconciler just retrying the same permanently-doomed DELETE every drainRequeueSuspend interval. The suspend has no path back on its own; sbcli's matching fix (400-not-500 on refusal) makes the retry loop terminate and resume the node once redeployed, but this still suspends it unnecessarily first. Adds fdRemovalBalanceCheck (storagenodeops_controller.go), called from drainValidate before Suspending: fetches the parent StorageNodeSet, computes the cluster's per-domain host map via the existing clusterFailureDomainHosts helper (excluding the node being removed), and calls the new fdRemovalBalanceViolation -- mirroring simplyblock_core's fd_balance_violation (+/-1 spread, 2-hosts-per-domain floor) so the two stay in lockstep. Blocks with a requeue (same pattern as the existing PinnedVolumeBlocking/UnmanagedVolumeBlocking checks) rather than failing outright, since restoring balance elsewhere can make a later retry succeed. Also: clusterFailureDomainHosts now skips already-removed nodes when aggregating per-domain host counts, mirroring simplyblock_core's failure_domain_host_map (which excludes STATUS_REMOVED for the same reason) -- a removed node's stale domain assignment must not inflate that domain's apparent host count for either this new gate or the existing activation-readiness gate that also calls it.
…iled handleDeletion only checked sn.Status.ActiveOpsRef before removing the finalizer -- but releaseLock (storagenodeops_controller.go) clears ActiveOpsRef identically whether the remove ops Succeeded or Failed. Failed means the backend node was never actually removed: blocked by a precondition (e.g. the failure-domain balance gate added earlier in this branch), or resumed instead of removed after the backend rejected the DELETE call. Removing the finalizer anyway would let Kubernetes delete the StorageNode CR while the backend node is still alive and online, with nothing left in the cluster to track it -- worse than the CR just sitting there waiting. handleDeletion now looks up the <name>-remove StorageNodeOps and, if its phase is Failed, blocks deletion, emits a RemoveOpsFailed event with the failure message, and requeues instead of removing the finalizer. A human must either fix whatever blocked the removal and delete the failed ops to retry, or restore the worker to spec.workerNodes to keep it. The Succeeded path is unchanged.
Two changes based on live testing feedback: 1. drainValidate now fails the ops outright (r.failOps) when the failure-domain balance check blocks a removal, instead of patching ops.Status.Message and requeuing every 60s like the pinned/unmanaged- volume checks above it. Those two resolve by acting ON THE NODE ITSELF (drop the pinned annotation, delete the unmanaged volume), so letting the same ops notice and proceed makes sense. Restoring failure-domain balance never does -- it needs a deliberate, cluster-wide action (add a host, or remove a different node instead) that this ops has no way to detect on its own, so silently polling forever just produces a Running ops that's actually permanently stuck and easy to miss in `kubectl get storagenodeops`. Failing here is safe specifically because handleDeletion no longer removes the StorageNode's finalizer while its remove ops is Failed (previous commit on this branch) -- the CR stays, nothing about the still-online backend node gets orphaned. 2. fdRemovalBalanceCheck was missing the explicit EnableFailureDomains gate that check_fd_admission_for_remove (simplyblock_core, the Python side it mirrors) has as its very first line. It happened to be harmless in practice -- clusterFailureDomainHosts already skips nodes with no FailureDomain reported, and an FD-disabled cluster's single implicit domain would need only one surviving host to avoid the 2-per-domain floor -- but relying on that incidentally is fragile. Added the same early-out explicitly, gated on the StorageCluster it now fetches alongside the StorageNodeSet. Extracted the shared 7-node (FD1=2/FD2=2/FD3=3) fixture into sevenNodeTopology and added a StorageCluster fixture (newTestStorageClusterWithFD) to the existing tests, plus two new ones: FD-disabled is a no-op even with the identical unbalancing topology, and drainValidate's wiring actually reaches Phase=Failed (not just fdRemovalBalanceCheck's return value).
newTestStorageNodeSet and newTestStorageClusterWithFD always receive the same name argument across their current call sites -- golangci-lint's unparam correctly flags that. Matches the existing convention on newTestStorageNode/newTestStorageNodeOps in this same file (also always-constant params, also nolint'd) rather than dropping the parameter, since these are test-fixture helpers likely to gain varying callers later.
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
3 participants
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.
Fixes; issue-383
Per-failure-domain drain concurrency gate
Fixes #383.
When
enableFailureDomains=true, the node drain coordinator now gates concurrent drains by failure domain instead of by node count. Workers in the same failure domain can drain in parallel (they share a fault boundary), while workers from different domains are blocked until an active domain finishes — capped atmaxFaultToleranceactive domains at a time.Changes
handleDetected: FD-enabled path checks active domains viaactiveDrainDomains; FD-disabled path keeps the existing node-count gate unchangedworkerFailureDomain: reads failure domain fromstatus.nodes[](populated from backend API) instead of spec, so nodes added outside the operator are handled correctlyactiveDrainDomains: usesworkerFailureDomainto resolve per-node overrides viaspec.nodeConfigs[worker].failureDomain"in_shutdown"/"in_restart"string literals with existing constants to fix lintTests added
maxFaultToleranceworkerFailureDomainreads from status, returns(0, false)for unassigned nodes