fix: disable replica migration and validity factor in cluster config

[bjosv]

This PR closes #216

Summary

Add cluster-allow-replica-migration=no to prevent Valkey from moving replicas between shards autonomously, which conflicts with the operator's topology management.

Add cluster-replica-validity-factor=0 so replicas always attempt failover regardless of disconnection time. With cluster-node-timeout at 2s, the default factor of 10 gives only a 30s window before replicas refuse to failover, causing stuck clusters under disruption.

Reorder PlanDrainMove to check slot count before primary existence, preventing a spurious error on already-drained shards. Returning an error due to the primary would cause the caller (drainExcessShards) to propagate it up, halting scale-down and leaving stale ValkeyNodes around forever.
This problem solved itself previously when cluster-allow-replica-migration=yes.

Testing

This problem was found by using #203 and running on a machine with moderate load:
CHAOS_MAX_SHARDS=15 CHAOS_SCENARIOS="scale-shards" make test-chaos

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one issue.
• Commit message explains what changed and why
• Tests are added or updated.
• Documentation files are updated.
• I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

[greptile-apps]

Greptile Summary

This PR fixes two operational issues in the Valkey cluster operator: it adds cluster-allow-replica-migration=no and cluster-replica-validity-factor=0 to the immutable base config so the operator — not Valkey's gossip layer — controls replica placement, and it reorders the slot-count vs. primary-existence checks in PlanDrainMove to avoid a spurious error that halted scale-down when an already-drained shard had no primary.

• config.go: Two new directives are appended to the base config that users cannot override, preventing autonomous replica migration and ensuring stale replicas still attempt failover regardless of disconnection duration.
• cluster_rebalance.go: PlanDrainMove now checks srcCount == 0 (early exit) before resolving the primary node, so an already-drained shard with a missing primary returns nil, nil instead of a fatal error that blocked all further scale-in.
• cluster_rebalance_test.go: A clarifying comment is added to TestPlanDrainMove_EmptySrc; the new nil-primary-with-zero-slots scenario is not explicitly tested.

Confidence Score: 4/5

Safe to merge — the logic changes are small, targeted, and correct; the config additions are well-justified by the operator's design intent.

The reordering in PlanDrainMove directly addresses the described race during scale-in, and the two new Valkey directives match the operator's philosophy of immutable topology management. The only gap is that the test suite doesn't cover the exact scenario (zero slots + nil primary) that drove the fix; the existing TestPlanDrainMove_EmptySrc always supplies a valid primary, so the pre-fix error path is never exercised by the new or existing tests.

A second look at cluster_rebalance_test.go is worthwhile to add a test covering the nil-primary + empty-slots combination that the reordering was meant to handle.

Important Files Changed

Filename	Overview
internal/controller/config.go	Adds cluster-allow-replica-migration=no and cluster-replica-validity-factor=0 to the operator-managed base config; both are well-justified and follow the existing immutable-config pattern.
internal/valkey/cluster_rebalance.go	Reorders slot-count check before primary-existence check in PlanDrainMove; correct fix to prevent a scale-in-halting error on already-drained shards with no primary.
internal/valkey/cluster_rebalance_test.go	Adds a comment to the empty-src test case, but doesn't add a test for the specific broken scenario: zero slots with a nil primary node.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PlanDrainMove called] --> B{maxSlots <= 0 or no dsts?}
    B -- yes --> C[return nil, nil]
    B -- no --> D[Count src slots]
    D --> E{srcCount == 0?}
    E -- yes --> F["return nil, nil (Nothing to drain)"]
    E -- no --> G[Resolve srcPrimary]
    G --> H{srcPrimary == nil?}
    H -- yes --> I[return error]
    H -- no --> J[Find dstPrimary]
    J --> K{dstPrimary == nil?}
    K -- yes --> L[return error]
    K -- no --> M[Compute SlotMove and return]

Loading

Comments Outside Diff (1)

1. internal/valkey/cluster_rebalance_test.go, line 163-177 (link)

   Missing test for the exact fixed scenario

   TestPlanDrainMove_EmptySrc uses newPrimaryShard, which always populates PrimaryId and includes a matching node in Nodes, so GetPrimaryNode() returns a non-nil value here. The regression that cluster-allow-replica-migration=no exposed — a shard with zero slots and a nil primary (primary has transitioned to a replica role during draining) — is never exercised. A test with an empty PrimaryId or an empty Nodes slice (causing GetPrimaryNode to return nil) alongside a nil slot list would directly verify the reordering protects against the error that halted scale-in.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

_{Reviews (1): Last reviewed commit: "fix: disable replica migration and valid..." | Re-trigger Greptile}

[melancholictheory]

Nice fix — the stuck-cluster failure mode from a too-strict validity window is real and easy to hit under load.

One thing worth flagging on cluster-replica-validity-factor 0: it's an availability-vs-data-freshness tradeoff, and setting it unconditionally cluster-wide may surprise consistency-sensitive users. With the validity gate off, a replica is always eligible to fail over regardless of how far it has fallen behind — so if a primary dies while its only reachable replica is very stale, that replica gets promoted and silently drops the writes it never received. For availability-first / cache-like clusters that is exactly what you want (keep the shard serving; stale data is acceptable). For durable / consistency-first clusters it is arguably the wrong default — you might prefer the shard to stay down over promoting stale data.

(The offset-ranking still applies: among eligible replicas the best-offset one wins, so the factor only changes behavior when every replica is past the window — i.e. the choice is "promote the freshest-of-the-stale" vs "no election at all".)

In our operator we ended up scoping this per workload class — cluster-replica-validity-factor 0 for availability-first clusters, and leaving the default gate in place for durable ones. Might be worth either gating the directive behind a similar availability/durability signal here, or at least documenting the freshness tradeoff alongside it.

Either way, thanks for chasing this down — the chaos-suite repro is a great catch.