fix: shardExistsInTopology only matches shards with a primary

[bjosv]

Summary

When a Deployment rollout replaces a primary pod, the old and new pods briefly coexist (maxSurge). If the old primary dies before the replica can failover, the shard has no primary and no slots, just an orphaned replica. The old code matched these orphaned replicas, blocking slot assignment to the replacement primary indefinitely.

Now shardExistsInTopology only matches shards that have a primary with slots, allowing the replacement primary to proceed with slot assignment.

This fixes CI issues like this.

Testing

Found using #203 running the delete-recreate-cluster scenario. This scenario deletes a ValkeyCluster, using Deployment, and wait for all resource to be gone, and then recreates it again.

CHAOS_SCENARIOS=delete-recreate-cluster CHAOS_WORKLOAD_TYPE=Deployment make test-chaos

It needed 3 to 357 iterations before hitting the issue.

---

The reason for restarting ValkeyNode pods during cluster creation, according to added debug logs in testruns, are annotation changes:

oldAnnotations: {"valkey.io/internal-acl-hash":"8fc9713c..."}
newAnnotations: {"valkey.io/config-hash":"1b746d50...", "valkey.io/internal-acl-hash":"8fc9713c..."}

The config-hash annotation was missing when the Deployment was created (because serverConfigHash was empty due to the informer cache race?). When the ValkeyNode controller reconciles again and sees the hash, it adds the annotation to the pod template.
Any change to the pod template spec (including annotations) causes Kubernetes to create a new ReplicaSet and roll the pods.

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

Fixes a race condition during Deployment rollouts where an orphaned replica (shard with no primary and no slots) could block a replacement primary from receiving slot assignment. The shardExistsInTopology helper now skips shards that lack a primary or lack slot assignments before comparing node IPs.

• The guard shard.PrimaryId == \"\" || len(shard.Slots) == 0 uses || (OR), which is slightly broader than the described "completely orphaned" scenario (both fields empty); changing it to && (AND) would more narrowly target that case and avoid skipping shards that are momentarily missing a primary ID during an active failover election.
• The function's docstring lists two handled cases but does not mention the newly excluded "orphaned shard" scenario, which could confuse future readers.

Confidence Score: 4/5

Safe to merge; the change is a small, targeted guard in a single helper and addresses a well-documented, reproducible deadlock during Deployment rollouts.

The fix correctly unblocks slot assignment when a shard is fully orphaned, and the two existing call sites behave properly under the new condition. The main open question is whether || is the right operator: using it also skips shards that momentarily have no PrimaryId but still carry assigned slots during a transient mid-failover window, whereas && would more precisely target the described scenario. In practice the Valkey cluster would reject a conflicting ADDSLOTSRANGE and the operator would retry, so this is unlikely to cause lasting damage.

internal/controller/utils.go — specifically the || vs && choice in the new guard and the outdated docstring for shardExistsInTopology.

Important Files Changed

Filename	Overview
internal/controller/utils.go	Adds an early-continue guard in shardExistsInTopology to skip shards where PrimaryId is empty or Slots is empty, preventing orphaned replicas from blocking slot assignment to a replacement primary; the `

Sequence Diagram

sequenceDiagram
    participant K8s as Kubernetes
    participant Op as Operator Reconciler
    participant VC as Valkey Cluster

    Note over K8s: Deployment rollout begins
    K8s->>K8s: Old primary pod dies before failover
    Note over VC: Shard state: PrimaryId="", Slots=[]
    K8s->>K8s: New replacement primary pod starts

    Op->>VC: Fetch ClusterState
    Op->>Op: assignSlotsToPendingPrimaries()
    Op->>Op: "shardExistsInTopology(shard=0)"

    alt OLD behavior (bug)
        Op->>Op: Orphaned replica IP matches shard node
        Op->>Op: Returns true → skip slot assignment
        Note over Op: Replacement primary blocked indefinitely
    else NEW behavior (fix)
        Op->>Op: "Shard skipped: PrimaryId=="" or Slots==[]"
        Op->>Op: Returns false → proceed with slot assignment
        Op->>VC: CLUSTER ADDSLOTSRANGE → replacement primary gets slots
        Op->>VC: CLUSTER REPLICATE → orphaned replica attaches
    end

Loading

Comments Outside Diff (1)

1. internal/controller/utils.go, line 177-188 (link)

   Stale function docstring after behavior change

   The docstring still says the function covers exactly two cases ("post-failover completed" and "mid-failover in progress"), but the implementation now has a third, excluded case: shards whose PrimaryId is empty or whose Slots list is empty are silently skipped. A reader relying on the comment won't know about this exclusion and may be surprised when a shard-match is not returned for an orphaned replica. The comment should mention the new "orphaned shard" guard alongside the existing cases.

   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: shardExistsInTopology only matches ..." | Re-trigger Greptile}

[greptile-apps]

|| guard could mask a transient mid-failover state

The condition skips a shard when shard.PrimaryId == "" regardless of whether shard.Slots is non-empty. During a Valkey failover election, there is a brief window where the old primary has been removed from the cluster view (clearing PrimaryId) but the slot ranges still appear in shard.Slots. In that transient state, the new condition would skip the shard, causing shardExistsInTopology to return false, and the replacement primary could attempt CLUSTER ADDSLOTSRANGE for slots that are mid-reassignment.

In practice this race appears narrow and the Valkey cluster itself would reject a conflicting ADDSLOTSRANGE, triggering a retry, so it's unlikely to cause lasting harm. But tightening the guard to shard.PrimaryId == "" && len(shard.Slots) == 0 would more precisely target the described "completely orphaned" scenario (no primary and no slots) while keeping the mid-failover shard visible.

[bjosv]

@greptileai Tell me when this case is possible.
Also see clusterstate.go and function GetClusterState where shard.Slots is set.

			if node.IsPrimary() {
				ranges, _ := parseSlotsRanges(node.GetSlots())
				shard.Slots = ranges
				shard.PrimaryId = node.Id
			}

[greptile-apps]

You're right. Looking at GetClusterState, both fields are set in the exact same if node.IsPrimary() block:

if node.IsPrimary() {
    ranges, _ := parseSlotsRanges(node.GetSlots())
    shard.Slots = ranges
    shard.PrimaryId = node.Id
}

shard.PrimaryId and shard.Slots are always set together. There is no code path that sets one without the other. My hypothesized state — PrimaryId == "" with non-empty Slots — is structurally impossible. The || condition is fine, and the len(shard.Slots) == 0 check is actually redundant given the PrimaryId == "" check already covers the case completely.

My original concern was wrong. The PR's logic is correct as written.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[jdheyburn]

Good find!

When a Deployment rollout replaces a primary pod, the old and new pods briefly coexist (maxSurge).

We should set Deployment update strategy to be Recreate instead of the default RollingUpdate @bjosv

[bjosv]

Closing this since we now use the Recreate deployment strategy and cluster-allow-replica-migration=no, so the code path has changed. The issue it fixes is not reproducible anymore.