fix: self-heal partial-slot outage (slots stuck on failed master)#300
Merged
Conversation
A ValkeyCluster could report cluster_state:ok and cluster_slots_assigned:16384 while cluster_slots_ok was below 16384 — one shard's slots were owned by a master flagged `fail`, leaving ~1/3 of the keyspace DOWN. The self-heal health gate only checked cluster_state / slots_assigned, so this partial outage was never healed. Detection (pure, unit-tested in internal/valkey/stuckslots.go): - IsClusterDegraded(): degraded when state != ok, OR slots_ok < 16384, OR any `master,fail` node still owns slots. - DetectStuckSlotHeals(): per stuck fail-master, picks a healthy replica (no fail/pfail flags, link connected; deterministic by node ID) as the takeover target; fail-masters with no healthy replica are excluded (data-preservation) and surfaced as a warning event. Action (internal/controller/cluster_stuckslots.go, wired into Reconcile step 10.6): issues CLUSTER FAILOVER TAKEOVER on the chosen replica (vk.ClusterFailoverTakeover) so the replica succeeds the slots and they return to ok. Idempotent (no-op once recovered) and conservative — the `fail` flag is only set by Valkey after node-timeout + gossip agreement, so converging clusters are not thrashed; pfail/fail? are not acted on. Also: ClusterReady condition and decidePhase now factor in slots_ok, so a partial outage is no longer mis-reported as Running/Ready (Reason PartialSlotOutage, Resharding cadence to retry the heal). New metric valkey_cluster_stuck_slot_takeover_total. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # internal/controller/valkeycluster_controller.go
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
1 participant
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.
Found in a production incident: a
ValkeyClusterreportedcluster_state:okandcluster_slots_assigned:16384butcluster_slots_ok:10922— ~5462 slots (one shard) were owned by amaster,failnode, so 1/3 of the keyspace was actually DOWN. The operator's INC-0001 self-heal stopped because its health gate only checkedcluster_state, not whether all slots wereokor whether a failed node still owned slots. The partial outage was left unhealed (recovered manually withCLUSTER FAILOVER TAKEOVER).Fix
internal/valkey/stuckslots.go):IsClusterDegradednow also trips whencluster_slots_ok < 16384OR amaster,failnode still owns slots — not only oncluster_state != ok.DetectStuckSlotHeals: for each stuck failed master that still owns slots, pick a healthy replica (no fail/pfail flags, link connected; deterministic by node id) as the takeover target. Masters with no healthy replica are excluded (data-preservation) and surfaced as aPartialSlotOutagewarning event rather than risking an unsafe promotion.internal/controller/cluster_stuckslots.go, Reconcile step 10.6): issueCLUSTER FAILOVER TAKEOVERon the chosen replica (newClusterFailoverTakeoverprimitive — go-redis'sClusterFailovercan't do the consensus-free TAKEOVER needed when the master is alreadyfail). Emits events + newvalkey_cluster_stuck_slot_takeover_totalmetric.decidePhase/ClusterReadynow factor inslots_ok, so a partial outage is reported asPartialSlotOutageinstead of being mislabeled Running/Ready.Conservative & idempotent: only acts on the
failflag (set only after node-timeout + gossip agreement), never transientpfail/fail?, so it respects node-timeout and won't thrash a converging cluster.Why not reuse failover.go
That path (
REPLICAOF NO ONE, offset-based, Pod-Ready-driven) is for Replication mode and does not transfer cluster slot ownership/epoch, so it cannot heal a Cluster-mode stuck-slot outage. Added the minimal cluster-specific primitive instead; reused existing dial/TLS/query abstractions.Tests
go build ./...clean;go test ./internal/valkey/... ./internal/controller/...pass, incl. new unit tests (incident snapshotslots_ok=10922+ fail-master → degraded + correct takeover target; no-candidate/unhealthy-skip/determinism;decidePhasepartial-outage). Pre-existing unrelatedTestChartIconURLUsesCurrentValkeyAsset(fails on main) ignored. 8 files, +497/-8.Needs live e2e (not coverable by envtest — no real gossip): that TAKEOVER against a genuinely
failmaster returnsslots_okto 16384 and convergence timing holds. Follow-up: pick takeover replica byslave_repl_offset(most up-to-date) instead of lowest id.🤖 Generated with Claude Code