fix: re-introduce cluster members whose addresses changed after a full restart - #333
Conversation
|
|
this is the right fix for the all-restart case, and it gets the two things that are easy to get wrong: sourcing the MEET target from the live cluster view ( one optional thing: on a full N-node restart every node's table lists every other peer as stale, so this issues on the order of N² MEETs before it settles. and where this sits relative to #296: the phase exists because nodes announce pod IPs, so |
|
Thanks @melancholictheory — good call on the N² MEETs. Pushed 5c88340: the heal now dedupes by the moved member's node ID, issuing one MEET per member instead of one per (viewer, peer) pair. A failed MEET doesn't mark the member as met, so another viewer retries it on the same pass. The first viewer in iteration order lists all moved peers, so the resulting MEET graph is effectively a star from that viewer — connected, and gossip converges the rest. Re-verified on the k3d repro (3 shards / 1 replica, persistence, delete all 6 pods): 6 MEETs (was 30), still ~20s to Agreed on #296: this phase is the reactive fix for IP-announcement clusters, hostname announcement is the preventive one — and if hostname mode lands, the detector's |
|
star from the first viewer is the clean way to do it, and 30 -> 6 on the repro is the payoff. one edge to keep in mind: if that first viewer is itself unreachable on a given pass, no star forms that round, but since an absent viewer just means the next reconcile picks another one, it self-heals on the requeue rather than getting stuck. so the only cost is an extra pass in that case, which is fine. nice turnaround. |
|
Thanks! Small note on that edge: it's actually cheaper than an extra pass. An unreachable viewer never makes it into the scraped state at all ( |
|
Thanks for raising this @matka12. I want to validate it works properly with reproduction steps. You mention the upgrade from 0.3.0 -> 0.4.0, perhaps you could provide that? Would we then expect the issue to be resolved with an upgrade from 0.3.0 -> |
|
Sure — two repros, both on a scratch cluster (I used k3d, kind works the same). The minimal one first since it isolates the bug from the upgrade mechanics: Repro A — minimal (any operator version, ~2 min)helm repo add valkey https://valkey.io/valkey-helm/
helm install valkey-operator valkey/valkey-operator --version 0.4.0 \
-n valkey-operator-system --create-namespace
kubectl apply -f - <<'YAML'
apiVersion: valkey.io/v1alpha1
kind: ValkeyCluster
metadata:
name: test
spec:
image: valkey/valkey:9.1.0
shards: 3
replicas: 1
persistence:
size: 1Gi
config:
appendonly: "yes"
YAML
# wait for state: Ready, then write a marker key
kubectl exec valkey-test-0-0-0 -c server -- valkey-cli -c set canary survives
# the trigger: every pod restarts at once, every pod IP changes
kubectl delete pod --allUnpatched result: pods come back Ready, but every With this PR's image: same kill, the controller logs Repro B — the 0.3.0 → 0.4.0 upgrade as triggerSame as A but install # per UPGRADE.md — CRDs first
kubectl apply --server-side -f https://raw.githubusercontent.com/valkey-io/valkey-operator/v0.4.0/config/crd/bases/valkey.io_valkeyclusters.yaml
kubectl apply --server-side -f https://raw.githubusercontent.com/valkey-io/valkey-operator/v0.4.0/config/crd/bases/valkey.io_valkeynodes.yaml
helm upgrade valkey-operator valkey/valkey-operator --version 0.4.0 -n valkey-operator-systemThe v0.4.0 controller adds the system-users auth env ( Your question — 0.3.0 → this PR directlyYes, resolved. The simultaneous roll itself still happens (the pod-template change is inherent to the version bump), but the controller doing the rolling is the patched one, so the moment the pods are back it detects the stale pairs and re-MEETs them — the cluster re-forms on its own in seconds instead of wedging. Data on the PVCs (AOF) is untouched throughout. That was exactly my original failure case: this bug turned a routine operator upgrade on a persistent production cluster into a manual-recovery incident. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
I was able to repo it and get it to work locally, so thank you! It would be great to have an e2e test for this, but I don't want to block the PR any further since I'd like to get it out for 0.5. Can you raise an issue for that and get one created once this is merged in please? Could you also take a look at the AI reviewer comments to see if its applicable? |
|
Done on both: AI reviewer comment — valid catch, fixed in d3ab0c7. e2e test — raised #352 with the scenario (create persistent cluster → kill all pods → assert |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesStale address healing
Sequence Diagram(s)sequenceDiagram
participant ClusterState
participant Reconciler
participant ValkeyNode
ClusterState->>Reconciler: Return stale viewer/live node pairs
Reconciler->>ValkeyNode: Send CLUSTER MEET to current address
ValkeyNode-->>Reconciler: Return command result
Reconciler->>Reconciler: Update status and requeue successful repairs
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: plugin(logcheck): plugin "logcheck" not found Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/valkey/clusterstate.go`:
- Around line 428-473: Update FindStaleAddressPeers to treat entries flagged
noaddr as stale when a live node with the same ID exists, bypassing the empty
hostFromClusterNodesEndpoint result for :0@0; retain the existing
address-difference check for non-noaddr entries. Add a test covering a :0@0
noaddr entry matched to a live node.
- Around line 400-419: Add coverage for real unbracketed IPv6 CLUSTER NODES
endpoints: update TestHostFromClusterNodesEndpoint in
internal/valkey/clusterstate_test.go to include unbracketed host:port@cport
cases, and mirror the stale-peer IPv6 scenarios with unbracketed endpoint
strings in internal/valkey/clusterstate_test.go:644-659 and 661-701; no direct
change is required in internal/valkey/clusterstate.go:400-419 unless needed to
support these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 313fd6ae-5fd7-417c-80cf-58fd55e27d08
📒 Files selected for processing (3)
internal/controller/valkeycluster_controller.gointernal/valkey/clusterstate.gointernal/valkey/clusterstate_test.go
|
Addressed both CodeRabbit findings in 19f641a:
|
|
The e2e failure here is inherited from main, not this branch: #341 granted Opened #355 with the test fix (move CONFIG GET to the allowed check, replace the denied slot with FLUSHALL). Once that merges I'll rebase/rerun here. |
…l restart When every pod restarts at the same time (full Kubernetes cluster restart, node pool replacement) with persistence enabled, each node comes back with a new pod IP while its persisted nodes.conf still lists the peers' old IPs. With no surviving member to gossip the new addresses, every node keeps dialing dead addresses, all peers stay fail?/fail, and the cluster never re-forms without a manual CLUSTER MEET (valkey-io#275). The operator already knows both sides: the live address of every member (ValkeyNode pod IPs) and each node's stale view (CLUSTER NODES). Add a heal phase that detects known-but-failing peers whose node ID belongs to a live member at a different address and re-introduces them with CLUSTER MEET. The handshake carries the member's node ID, so the viewer rebinds the existing entry to the new address and gossip propagates it. Also guard forgetStaleNodes against this state: it matches failing entries to ValkeyNodes by pod IP, so a live member at a changed address looked like a dead node and could be issued CLUSTER FORGET - banning it from rejoining for a minute and fighting the recovery. Verified on a 3-shard/1-replica cluster with persistence: deleting all six pods simultaneously previously left the cluster in cluster_state:fail indefinitely; with this change the operator re-forms it in ~20s (StaleAddressesHealed event, 30 peer links) with data intact. Fixes valkey-io#275 Signed-off-by: Matan David <matan.david@eon.io>
On a full N-node restart every node's table lists every peer as stale, so the heal issued ~N^2 MEETs before settling. One successful MEET per moved member is enough: the handshake also registers the viewer's current address on the target, and gossip carries both new addresses to the remaining nodes. Re-verified on the 3-shard/1-replica k3d repro: deleting all six pods now heals with 6 MEETs (was 30), still ~20s to Ready with data intact. Signed-off-by: Matan David <matan.david@eon.io>
CLUSTER NODES renders IPv6 endpoints bracketed ([fd00::2]:6379@16379). Slicing the endpoint at its last colon kept the brackets, so a failing IPv6 peer at its current address never compared equal to the bare pod IP Kubernetes reports - misclassifying it as moved and issuing spurious CLUSTER MEETs plus requeues on IPv6 clusters. Extract the host with net.SplitHostPort (after trimming the @cport and optional ,hostname suffix), which unbrackets IPv6 and leaves IPv4 untouched. Signed-off-by: Matan David <matan.david@eon.io>
noaddr entries carry the :0@0 placeholder, so endpoint parsing yields no host and the address-difference guard skipped them - the one flag that by definition never has an address to compare. Treat a noaddr entry as stale whenever a live node with the same ID exists. CLUSTER NODES reports IPv6 endpoints unbracketed (fd00::2:6379@16379); those take the last-colon fallback in hostFromClusterNodesEndpoint and already parse correctly - add parser and stale-peer coverage for them alongside the bracketed form. Signed-off-by: Matan David <matan.david@eon.io>
19f641a to
f1574c2
Compare
|
Will look to get this merged in after #338 |
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
LGTM, just a finding about the brackets.
I now read in the comments that it was added as a defense
| } | ||
|
|
||
| // hostFromClusterNodesEndpoint extracts the bare host from a CLUSTER NODES | ||
| // endpoint field (<ip:port@cport[,hostname]>). IPv6 hosts appear bracketed |
There was a problem hiding this comment.
I don't think CLUSTER NODES uses brackets by default, unlike valkeys logger?
Maybe it's when user sets in in cluster-announce-ip? (which seems to be blocked soon via valkey-io/valkey#4055)
Maybe I'm wrong, and in that case we should reuse this function in GetFailingNodes
## Summary The e2e test promised in #352, covering the stale-address heal merged in #333 (#275). ## Scenario 1. Create a 3-shard / 1-replica **persistent** ValkeyCluster (new sample manifest) and wait for Ready. 2. Write a canary key. 3. `kubectl delete pod -l valkey.io/cluster=...` — all six pods restart at once, every pod IP changes while each persisted `nodes.conf` still holds the old peer addresses. No node is isolated, so only the heal phase can re-form the cluster. 4. Assert: - a `StaleAddressesHealed` event is emitted for the cluster, - the CR returns to `Ready` with 3 ready shards, without any manual `CLUSTER MEET`, - `cluster_state:ok` and the canary key survived (AOF on the PVCs), - **no** `StaleNodeForgotten` event for the cluster during recovery — the forget guard from #333 (a forget here would 60s-ban the rejoining member). ## Verification Ran the exact scenario against an image built from current main (`5676fb9`) on a local cluster: heal event fired (`Re-introduced 6 peer link(s) whose addresses changed`), Ready ~40s after the kill, canary intact, zero forget events. Closes #352 --------- Signed-off-by: Matan David <matan.david@eon.io> Co-authored-by: Joe Heyburn <34041368+jdheyburn@users.noreply.github.com>
4 participants
Summary
Fixes #275 — a persistent
ValkeyClusternever re-forms after a full restart that changes every pod IP; it stays inReconcilinguntil someone manually runsCLUSTER MEET.Root cause: each node's persisted
nodes.conflists its peers' old pod IPs. A single restarted member is re-discovered through the survivors, but when all members restart at once there is no surviving gossip path — every node keeps dialing dead addresses and all peers stayfail?/failforever. The operator's existing MEET phase only handles isolated nodes (cluster_known_nodes <= 1); these nodes know all their peers (at dead addresses), so nothing heals them.Changes
ClusterState.FindStaleAddressPeers()(internal/valkey/clusterstate.go): detects peers flaggedfail/fail?/noaddrin a node's table whose node ID belongs to a live, reachable member at a different address.fail?is included deliberately — pfail→fail promotion needs gossip between a majority of primaries, which is exactly what a cluster-wide address change breaks.healStaleAddressPeers()(internal/controller/valkeycluster_controller.go): new reconcile phase, runs afterpromoteOrphanedReplicasand beforeforgetStaleNodes. For each stale pair it issuesCLUSTER MEET <live-ip> <port>from the viewer; the handshake carries the member's node ID, so the viewer rebinds the existing entry to the new address and gossip propagates from there. Emits aStaleAddressesHealedevent and requeues.forgetStaleNodesguard: it matches failing entries to ValkeyNodes by pod IP, so a live member at a changed address looked like a dead node and could receiveCLUSTER FORGET— a 60-second rejoin ban that actively fights the recovery. Entries whose ID matches a live scraped node are now skipped.Verification
Unit tests for the detector (full-restart, dead-peer, transient-pfail, gossip-in-progress, pending-node cases) plus live verification on a 3-shard / 1-replica cluster with persistence (k3d,
valkey/valkey:9.1.0):kubectl delete pod --all(all 6 pods, IPs change)cluster_state:fail, stuckReconciling13+ min, needs manualCLUSTER MEETcluster_state:ok+Readyin ~20s, no interventionStaleAddressesHealed: Re-introduced 30 peer link(s) whose addresses changedThe same failure is triggered by the 0.3.0 → 0.4.0 operator upgrade itself: the new auth env vars update all ValkeyNode StatefulSets in one reconcile pass, restarting every pod simultaneously — so this also makes operator upgrades on persistent clusters recover automatically.
make testpasses (90 controller specs, valkey + api packages).