Feat/nodedrain failure domain gate - #387
Open
geoffrey1330 wants to merge 10 commits into
Open
Conversation
…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.
wmousa
force-pushed
the
feat/nodedrain-failure-domain-gate
branch
from
August 6, 2026 12:40
43bde05 to
dc47ecb
Compare
staticcheck SA1019 flagged the read: Requeue is deprecated in favor of
RequeueAfter. reconcileActivate's init-action step actually returns
Result{Requeue: true} (unrelated production behavior, not changed
here), so check res.IsZero() instead of reading the deprecated field
directly -- still verifies a requeue was requested without referencing
Requeue.
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.
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