fix: re-introduce cluster members whose addresses changed after a full restart

[matka12]

Summary

Fixes #275 — a persistent ValkeyCluster never re-forms after a full restart that changes every pod IP; it stays in Reconciling until someone manually runs CLUSTER MEET.

Root cause: each node's persisted nodes.conf lists 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 stay fail?/fail forever. 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 flagged fail/fail?/noaddr in 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 after promoteOrphanedReplicas and before forgetStaleNodes. For each stale pair it issues CLUSTER 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 a StaleAddressesHealed event and requeues.
• forgetStaleNodes guard: it matches failing entries to ValkeyNodes by pod IP, so a live member at a changed address looked like a dead node and could receive CLUSTER 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):

	unpatched	patched
kubectl delete pod --all (all 6 pods, IPs change)	cluster_state:fail, stuck Reconciling 13+ min, needs manual CLUSTER MEET	cluster_state:ok + Ready in ~20s, no intervention
Data (AOF on PVC)	intact after manual heal	intact
Event trail	—	StaleAddressesHealed: Re-introduced 30 peer link(s) whose addresses changed

The 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 test passes (90 controller specs, valkey + api packages).

[greptile-apps]

Greptile Summary

This change improves recovery of persistent Valkey clusters after all members restart with new addresses: it detects stale peer records, reintroduces reachable members, and avoids forgetting members that are alive at a replacement address. The focused stale-address recovery test passed, but IPv6 peer addresses are parsed incorrectly: a reachable peer can be repeatedly treated as moved because Valkey brackets IPv6 addresses in CLUSTER NODES output.

Confidence Score: 3/5

Not safe to merge until T-Rex findings are addressed.

The changed stale-peer recovery path was exercised directly and incorrectly classifies a reachable IPv6 peer as moved for every failure state that triggers healing.

T-Rex reproduced 2 failing behaviors at runtime in internal/valkey/clusterstate.go; the change needs fixes before it is safe to merge.

Files Needing Attention: internal/valkey/clusterstate.go needs normalized IPv6 host parsing, with regression coverage in internal/valkey/clusterstate_test.go.

T-Rex Logs

What T-Rex did

• T-Rex produced a proof for a posted P1 finding about focused IPv6 stale-address classification, including a focused Go test source and the accompanying test output.
• T-Rex produced a second finding-comment-proof for another related P1 finding.
• T-Rex executed a Go test repro command as a general-contract-validation-proof, showing parsed_address="[fd00::2]", live_address="fd00::2", and stale_pairs=1 with exit code 0.

_{Ran code and verified through T-Rex}

Comments Outside Diff (1)

1. General comment

   Bracketed IPv6 peer addresses are always misclassified as stale when marked failing

   • Bug
     • FindStaleAddressPeers receives standard bracketed IPv6 CLUSTER NODES entries such as [fd00::2]:6379@16379, slices at the final colon, and obtains [fd00::2]. This does not equal the scraped live Pod IP fd00::2, so the method selects the peer as stale even though it is at its current address. The executed focused test reproduced this for fail, fail?, and noaddr.
   • Cause
     • internal/valkey/clusterstate.go:445 finds the final colon and internal/valkey/clusterstate.go:449 compares the resulting bracket-preserving host substring directly with the unbracketed NodeState.Address.
   • Fix
     • Parse the CLUSTER NODES endpoint with IPv6-aware logic (for example, split off the bus-port suffix and use net.SplitHostPort for the remaining endpoint), then compare its normalized host to peer.Address. Add regression cases for bracketed IPv6 with fail, fail?, and noaddr, plus an IPv4 control case.

   _{Ran code and verified through T-Rex}

_{Reviews (3): Last reviewed commit: "fix: issue one MEET per moved member ins..." | Re-trigger Greptile}

[melancholictheory]

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 (live.Address) rather than the stale table (the persisted nodes.conf is exactly what can't be trusted here), and guarding CLUSTER FORGET when the failing entry's node ID still belongs to a reachable member, otherwise cleanup would ban the very node you just re-introduced. the address != peer.Address check also keeps this from firing on a normal transient fail?, so including fail? for the all-restart case doesn't cost spurious MEETs during ordinary gossip hiccups.

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. CLUSTER MEET propagates transitively, once any reachable viewer meets a node, gossip carries that node's new address to the rest, so meeting each live node once from a single viewer is enough and is gentler on a large cluster. not a correctness issue, the N² just makes more noise before it converges.

and where this sits relative to #296: the phase exists because nodes announce pod IPs, so nodes.conf holds addresses that die on restart. if hostname announcement lands and is in use, the persisted entries are resolvable names that survive the IP change and this heal path stops having anything to do. so it's the reactive fix for IP-mode clusters and #296 is the preventive one, complementary, and the phase stays correct (just inert) under hostname mode.

[matka12]

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 Ready with data intact.

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 address != peer.Address guard means this phase simply never fires.

[melancholictheory]

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.

[matka12]

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 (GetClusterState drops nodes it can't connect to), so its stale pairs simply don't exist that round — the dedupe walks the remaining viewers' pairs, and since every reachable viewer's table lists all moved members, the next viewer in iteration order becomes the hub in the same pass. The requeue-and-retry case only kicks in for MEETs that fail mid-command, which don't mark the member as met.

[jdheyburn]

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 -> this PR?

[matka12]

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 --all

Unpatched result: pods come back Ready, but every nodes.conf (persisted on the PVCs) lists the peers' old IPs. CLUSTER NODES on any pod shows all peers fail?/fail at dead addresses, cluster_state:fail, and the CR sits in Reconciling ("Waiting for replica sync") indefinitely — I gave it 13+ minutes. The existing MEET phase doesn't fire because no node is isolated (cluster_known_nodes = 6 on all of them; they know every peer, just at dead addresses). Manual CLUSTER MEET <new-ip> 6379 per peer from any one pod recovers it — that's the workaround folks in #275 landed on.

With this PR's image: same kill, the controller logs re-introducing peer whose address changed and emits StaleAddressesHealed: Re-introduced 6 peer link(s); cluster_state:ok and CR Ready ~20s after the pods are back, canary intact. No intervention.

Repro B — the 0.3.0 → 0.4.0 upgrade as trigger

Same as A but install --version 0.3.0 first (CR unchanged). Once Ready:

# 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-system

The v0.4.0 controller adds the system-users auth env (PRIMARY_AUTH, VALKEY_USER, ACL secret mounts) to every ValkeyNode's pod template in the same reconcile pass, so all six single-replica workloads roll simultaneously — which is repro A's trigger, no infrastructure event needed. Same deadlock follows.

Your question — 0.3.0 → this PR directly

Yes, 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.

[jdheyburn]

@greptile-apps

[greptile-apps]

Bracketed IPv6 peers are falsely treated as moved

CLUSTER NODES represents IPv6 endpoints as [host]:port@bus-port, but slicing at the final colon leaves brackets in the extracted address ([fd00::2]). Kubernetes reports the live Pod IP as fd00::2, so a reachable peer carrying fail, fail?, or noaddr is incorrectly classified as having changed address. The reconciler then issues an unnecessary CLUSTER MEET and requeues, which can repeatedly interfere with normal recovery for IPv6-enabled clusters. Parse and normalize the endpoint with IPv6-aware host/port handling before comparing it with peer.Address.

Artifacts

Focused Go test source for bracketed IPv6 stale-address classification

• Temporary package-local Go test executed against current HEAD; it constructs a live IPv6 peer and standard bracketed CLUSTER NODES entry for each failure flag, demonstrating the exact method input.

Focused Go test output showing all bracketed IPv6 failure states selected as stale

• Captured output from the focused Go test command, including command, working directory, exit code, parsed and live addresses, and one stale pair for fail, fail?, and noaddr; the takeaway is that current HEAD produces false stale detections for current IPv6 peers.

_{Ran code and verified through T-Rex}

[jdheyburn]

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?