fix: cluster data-loss, graceful-failover & webhook fixes + full e2e coverage - #98
Merged
Merged
Conversation
- supportedVersions now matches the API version enum (7.2/8.0/8.1/9.0/9.1); 8.2 was never a valid Version (the CRD enum rejects it) so its test groups could never pass - re-path the upgrade chains through valid versions: 8.1 -> 9.0 (crosses the RDB-format boundary guarded by SHUTDOWN NOSAVE) and 9.0 -> 9.1, keeping the latency-tracking assertion when landing on 9.0+ - wrap per-version testcases in a version-titled Context labeled v<version> so failures are attributable to a version and runs can be targeted with --ginkgo.label-filter (e.g. v9.0) (cherry picked from commit 5fd587ae22a13491814cb9e5570436bdbc280661)
On clusters with the OwnerReferencesPermissionEnforcement admission plugin (e.g. cpaas), creating/updating the sync-<pod> nodes.conf ConfigMap fails with "cannot set blockOwnerDeletion" because the instance ServiceAccount lacks update permission on the owner CR finalizers subresource. Without persisted nodes.conf, restarted cluster nodes rejoin slowly and the pre-stop failover cannot find a candidate replica in time, shutting down masters unpromoted and losing all data during rolling restarts. Copy the StatefulSet owner references with BlockOwnerDeletion cleared; background garbage collection does not need it. (cherry picked from commit 01d69b7fc8cb2f8a0a366506fd2c3b14e6660821)
Deleting a master pod triggers the pre-stop failover (candidate lookup, anti-collision sleep, failover, verify) bounded by the 300s termination grace period, and the StatefulSet cannot recreate the pod until termination completes. The 2-minute replacement timeout is shorter than a legitimate graceful handoff; raise it to 8 minutes. (cherry picked from commit e902129f1d337219637f3018d5c5763cadeaec99)
When a master pod is deleted right after an ACL/password rollout, the
master-replica replication link can still be recovering, and a plain
CLUSTER FAILOVER on the replica keeps failing ("do manual failover
failed"). The shutdown retry loop then burns the entire 300s
termination grace period (each retry also slept a random 1-50s),
the kubelet SIGKILLs the unpromoted master, and its empty restart
wipes the shard via full resync.
Escalate to CLUSTER FAILOVER FORCE after three failed plain attempts:
the node is terminating anyway, and a forced promotion with the
replica's current dataset is strictly better than dying unpromoted.
Also shorten the anti-collision jitter on retries (the large first-try
jitter is kept) so the grace budget is spent failing over, not
sleeping.
(cherry picked from commit cfb87ac24f8c2cd0dfe51dcd022ae5d12b158ff9)
A failover election needs up to 2x cluster-node-timeout (30s at the
default 15s) to gather votes, and masters refuse to re-vote within the
same window. doValkeyFailover only observed the replica for 15s after
issuing CLUSTER FAILOVER, so the shutdown retry loop re-issued the
command mid-election; valkey then logs "received a new manual failover.
Resetting the election", the prior votes are discarded, and the
election never reaches majority ("Waiting for votes, but majority still
not reached") for the whole termination grace period.
Observe for 70s per attempt instead, covering a full election round
plus one cooldown-blocked round.
(cherry picked from commit a854616200937094a457003373e5022a971197e9)
The previous two fixes interacted badly: with a 70s observation window per attempt and FORCE escalation only from the fourth attempt, the three plain attempts consumed the entire 300s termination grace before FORCE ever fired, and the master still died unpromoted. Rebalance the budget: observe plain manual failover for 35s (its protocol window is 2x cluster-node-timeout), escalate to FORCE from the second attempt with the full 70s election observation, cap the anti-collision jitter at 15s (first attempt) / 5s (retries), and log loudly if the loop ends with the master still unpromoted. This fits one plain plus three full FORCE election rounds within the default grace period, with vote-cooldown gaps between them. (cherry picked from commit 02426092cce0d8e13f9e149f3e0f82bc3d90c25a)
The User validating webhook re-validates the full spec on every update, including secret existence. Once the referenced password secret is deleted (e.g. namespace teardown), the finalizer-removing update issued by the user controller is rejected with "Secret not found", the finalizer can never be removed, and the User -- and any namespace containing it -- is stuck terminating forever. Skip validation for updates to objects that already carry a deletion timestamp; those updates are metadata-only. (cherry picked from commit bf54a32b2b07de51e761829aba840998e966ef08)
Root cause of the valkey 9.1 cluster failover breakage. The helper pre-wrote nodes.conf on first boot with an operator-derived shard-id (sha1(namespace/stsName), identical for every pod in a StatefulSet) and re-forced that shard-id on every recovery. Because a master and its replica then shared a pre-written shard-id, the replica's first post-REPLICATE announcement entered valkey's same-shard stale-packet branch instead of the safe cross-shard-move branch. On valkey 9.1.0 the tightened check (nodeEpoch(sender) > claimed, valkey-io/valkey#2811), combined with epoch-collision-resolution assigning the replica a higher epoch than its primary, made every such announcement be dropped forever ("Ignore stale message ... in shard"). Peers permanently saw the replica as an empty primary, so failover elections got zero votes ("Failover auth denied ... it is a primary node") and a killed master's shard was wiped. shard-id proves shared replication history and is valkey's to assign (there is no CLUSTER SET-SHARD-ID); the operator must not fabricate it. Seed only a deterministic node-id on first boot and preserve valkey's real shard-ids verbatim on recovery. valkey adopts the primary's shard-id via updateShardId when the node replicates. Reproduced in isolation (bare pods, operator timing): 0/52 divergence without shard-id prewrite, 4/4 with it; the fix (node-id only) is 0/4. Removes: SHARD_ID env (clusterbuilder), the heal shard-id CLI flag, and the shard-id forcing in generateValkeyCluterNodeRecord / fixClusterNodesConf. (cherry picked from commit a5d5c511e9d4beea5cdcd56750d4b31b9ccb1538)
The valkey-go cluster client fetches its slot map lazily, so right after a topology change (notably the ACL-triggered reconnects during create-and-delete-users) a slot can momentarily map to no node and an operation fails with "the slot has no valkey node", aborting the whole ordered suite. After constructing a cluster client, probe a spread of keys until routing succeeds (a failed probe forces a slot-map refresh), bounded to one minute, so callers never observe the transient gap. (cherry picked from commit 2c6fbad3fa59fab88813c46d07bd68dd36695416)
checkInstanceRead already wraps its loop in Eventually (rebuilding the client and retrying), but checkInstanceWrite built the client once and asserted each SET directly, so a transient "the slot has no valkey node" during ACL-driven connection churn failed the spec. Make the write path symmetric: rebuild and retry the loop for up to 5 minutes. (cherry picked from commit a3d5e3fe5aa8ae5edaeb3a3c4e8d1b5b87f4b18a)
A cluster client caches its slot map at construction. When the cluster is still settling (e.g. right after a config-propagation pod restart) that snapshot can miss slots, and every later command on an uncovered slot fails with "the slot has no valkey node" — the client treats the slot as empty and never re-fetches. The read-only 16-key probe passed on covered slots while the gap remained, so the 8.1 datatypes extra-test hit it on an HSET. Gate the returned client on cluster_state:ok (full 16384-slot coverage) and rebuild it between attempts so it captures a complete topology before any caller uses it. (cherry picked from commit 6b13eb4aec3e866f0393b1f647bbd9c7b46eea8f)
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.
These are the fixes and e2e coverage that came out of investigating a cluster data-loss on valkey 9.1. Grouped by concern.
🔴 Cluster data loss
nodes.confwith a deterministic per-shard SHA1shard-id. Once valkey's #2811 two-sided stale-message check is present (valkey 9.1.0, and backported to 8.0.10 / 8.1.9 / 9.0.5), a replica's first post-REPLICATErole announcement enters the same-shard "stale" branch and is dropped permanently — peers then see replicas as empty primaries, failover elections collect 0 votes (Failover auth denied … it is a primary node), and killing a master wipes its shard. Fix: seed a deterministic node-id only and let valkey adopt the master's shard onREPLICATE; the helper preserves valkey's real shard-ids verbatim on recovery.blockOwnerDeletionon synced ConfigMap owner refs. On clusters running theOwnerReferencesPermissionEnforcementadmission plugin, the helper'ssync-<pod>ConfigMap create/update is forbidden (cannot set blockOwnerDeletion), sonodes.confnever persists and restarted nodes rejoin slowly — the pre-stop failover then finds no candidate and masters shut down unpromoted, losing data on rolling restarts. Background GC does not needblockOwnerDeletion, so clear it.Graceful-shutdown failover
Webhook
Userobjects that are being deleted — otherwise aUser(and its namespace) gets stuckTerminatingonce the referenced password secret is already gone.e2e
the slot has no valkey node): a routability probe, a whole-write-loop retry, and a full-slot-coverage (cluster_state:ok) gate that rebuilds the client so it captures a complete topology before use.Validation: the full 277-spec e2e suite passed 243 Passed | 0 Failed | 34 Skipped on both amd64 (K8s 1.34) and arm64 (K8s 1.33) — all five versions × cluster / failover / replica, including the cluster kill-master resilience specs that previously lost data on 9.1.