fix: cluster modules + replicasPerShard=0 + replica re-integration#299
Merged
Conversation
Defect ④ (replicasPerShard=0 / masters-only topology): The ValkeyCluster mutating webhook clobbered an explicit replicasPerShard=0 to 1, and the validator rejected autoFailover=true + rps=0, making a masters-only topology impossible. The field is json:"replicasPerShard" (no omitempty) with CRD +kubebuilder:default=1, so the apiserver already honors an explicit 0 (defaults apply only to absent fields). Removed the webhook 0→1 coercion and the autoFailover+rps=0 rejection — an unset field still defaults to 1, an explicit 0 is preserved (shards masters, 0 replicas). Defect ⑥ (cluster modules): Added Modules []ModuleSpec to ValkeyClusterSpec (v1alpha1 + v1alpha2), mirroring ValkeySpec exactly. Wired vc.Spec.Modules into the cluster STSParams so module init-containers + --loadmodule apply to cluster pods, and added the same official-preset / external-Redis-Stack validation (validateModules) to the ValkeyCluster validating webhook. Regenerated CRDs and DeepCopy. Conversion is JSON byte-copy so modules round-trip automatically (test added). Defect ③ (replica re-integration): The health gate passed at the slot level (cluster_state:ok) and never checked membership, so a node that fell out of CLUSTER NODES after a restart (new node id / lost nodes.conf) was never re-added. Added ensureClusterMembership: compares CLUSTER NODES against the desired topology, then CLUSTER MEET (current pod IP) + CLUSTER REPLICATE the correct shard primary for any drifted/missing member. Idempotent — healthy members are skipped, primaries are met before their replicas. Reuses the existing valkey client helpers (new exported MeetNode / ReplicateTo wrap resolveAddrIP + replicateWithRetry). The pure decision logic (detectReintegration / buildObservedMembers) is unit-tested; the live MEET/REPLICATE path needs real-cluster e2e validation (no gossip in envtest). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a node re-joins with a new node id (e.g. after CLUSTER RESET HARD), its old/dead id lingers in the gossip table as a fail,noaddr (or slave,fail,noaddr) ghost, inflating cluster_known_nodes until Valkey's slow auto-eviction. The re-integration reconcile path now also detects such ghosts and CLUSTER FORGETs them on every healthy member. detectStaleNodes (pure) selects only clearly-dead ghosts: nodes flagged fail AND (noaddr OR an addr not backed by any current expected pod). It never selects myself, a current-pod-backed member (even if transiently failing), or handshake nodes (still converging — left to Valkey's own timeout). forgetStaleNodes mirrors the scale-in/teardown forget pattern, issuing CLUSTER FORGET from all current members for gossip propagation, with a live-id guard so a real member can never be forgotten. Gated inside ensureClusterMembership (allReady && cluster_state=ok && replicasPerShard>0) so it cannot race bootstrap. Best-effort: forget failures don't block re-integration and retry next reconcile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(defect ④)
A ValkeyCluster with spec.replicasPerShard: 0 (masters-only topology) was
silently coerced to 1. The field was a non-pointer int32 with CRD marker
+kubebuilder:default=1; with a non-pointer int the apiserver cannot tell
"omitted" from "explicit 0", so the default defeated an explicit 0.
Fix: make ReplicasPerShard *int32 (v1alpha1 + v1alpha2), drop the CRD
default=1 marker, and move nil→1 defaulting into code:
- GetReplicasPerShard() accessor: nil → 1, explicit value (incl. 0) → as-is.
- TotalNodes() and all controller reads route through the accessor.
- Mutating webhook defaults nil→1 so stored objects carry an explicit value,
while never touching an explicit 0 (masters-only preserved).
JSON byte-copy conversion stays correct: pointer + omitempty makes nil↔nil
and explicit-0↔explicit-0 round-trip exactly. Regenerated CRDs (no more
default:1, replicasPerShard no longer required) + DeepCopy (controller-gen).
Chart CRD copy re-synced (TestCRDBaseChartSync green). k8s.io/utils promoted
to a direct dependency.
Tests prove: nil → GetReplicasPerShard()==1 and TotalNodes uses 1; explicit 0
→ ==0 and TotalNodes==shards (masters-only); explicit 2 → 2; webhook defaults
nil→1 and preserves explicit 0; conversion round-trips both nil and explicit 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jongko54
added a commit
that referenced
this pull request
Jun 18, 2026
# 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.
Three operator defects found during a production incident + sharding benchmark. Independent of (and complementary to) #298.
④
replicasPerShard: 0is silently coerced to 1 (masters-only impossible)A
ValkeyClusterwithspec.replicasPerShard: 0came up withshards×2pods. Root cause was notomitempty(the field already round-trips 0 through the apiserver) but two things in the v1alpha1 webhook:0 → 1,autoFailover=true && rps=0as contradictory.Fix: drop both. Masters-only is a valid topology (no-failover is its inherent tradeoff, not a contradiction). Kept
int32(no*int32churn across ~42 call sites — the non-pointer field already persists an explicit 0). Tests added: defaulter preserves 0, validator accepts masters-only,TotalNodes/podAddressescorrect at rps=0.⑥
ValkeyClusterSpechas nomodulesfield (no valkey-search on cluster)Valkeysupportsspec.modules(official BSD presets — valkey-search/json/bloom, ADR-0032) butValkeyClusterrejected it (unknown field), so vector/secondary search could only run on a single node, never sharded.Fix: add
Modules []ModuleSpectoValkeyClusterSpec(v1alpha1 + v1alpha2, mirroringValkeySpec), wire it into the clusterSTSParamssoBuildModuleInitContainers+--loadmoduleapply to cluster pods, and add the same official-preset validation to the ValkeyCluster webhook (rejects external Redis Stack modules). CRD + DeepCopy regenerated; chart CRD synced; JSON byte-copy conversion round-trips (test added).③ Operator does not re-integrate a replica that fell out of the cluster
The slot-level health gate (
cluster_state:ok) ignored membership, so a node that lost cluster membership (e.g. new node id after restart) was never re-added — the cluster ran belowreplicasPerShardindefinitely.Fix (
internal/controller/cluster_reintegrate.go): puredetectReintegration/buildObservedMemberscompute which pods are missing fromCLUSTER NODESand which master each replica should follow (primaries before replicas); liveensureClusterMembershiprunsCLUSTER MEET <current pod IP>+CLUSTER REPLICATE <master id>, gated onallReady && cluster_state==ok && rps>0, idempotent. NewMeetNode/ReplicateToreuse existing retry/IP-resolve helpers; IP-based MEET aligns with #298's announce-ip.Verified vs needs e2e: decision logic is fully unit-tested (9 cases). The live MEET/REPLICATE sequence cannot be exercised in envtest (no real Valkey gossip) and needs real-cluster e2e to confirm gossip convergence and post-failover re-pointing. Implemented per existing patterns, not faked.
Tests
go build ./...clean;make testgreen except the pre-existing unrelatedTestChartIconURLUsesCurrentValkeyAsset(fails identically onmain). 16 files, +883/-37.🤖 Generated with Claude Code