fix: sentinel network partition failover

[mvasilenko]

No description provided.

[Copilot]

Pull request overview

Fixes failover behavior during network partitions by avoiding unnecessary healing actions when Sentinels or Valkey pods are temporarily unreachable.

Changes:

• Sentinel master discovery now skips unreachable Sentinel nodes and relies on quorum/majority reporting.
• Failover rule engine health checks now use RawNodes() to distinguish unreachable pods from misconfiguration (reducing false HealMonitor/HealPod triggers).
• Adds unit tests covering network-partition scenarios and index-mismatch healing behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
internal/valkey/failover/monitor/sentinel_monitor.go	Avoids failing master discovery on a single unreachable Sentinel; continues to allow quorum decision.
internal/ops/failover/engine.go	Adjusts node health logic to avoid fighting Sentinel failover when pods are unreachable and refines index-mismatch behavior.
internal/ops/failover/engine_test.go	Adds tests for partition/unreachable-pod scenarios to validate the updated health-check logic.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The len(rawPods) > len(inst.Nodes()) check is too broad: it skips healing whenever any pod failed to materialize as a ValkeyNode, even if the Sentinel-reported master address doesn't correspond to the missing/unreachable pod. This can mask genuine master-address inconsistencies (e.g., Sentinel points to an IP that isn't any pod) as long as some other pod is unreachable. Consider narrowing the condition by first matching monitorMaster.Address() to a specific pod in rawPods and only skipping heal if that specific pod exists but is not present/connectable in inst.Nodes().

Suggested change

	} else if len(rawPods) > len(inst.Nodes()) {
	// Unreachable pod != dead pod — healing would fight Sentinel's failover.
	logger.Info("master not in connected nodes but unreachable pods exist, allowing label update",
	"master", monitorMaster.Address())
	return nil
	} else {
	masterHost, _, addrErr := net.SplitHostPort(monitorMaster.Address())
	if addrErr != nil {
	logger.Error(addrErr, "failed to parse monitor master address; treating master as down",
	"master", monitorMaster.Address())
	} else {
	masterRawPodExists := false
	for _, pod := range rawPods {
	if pod.Status.PodIP == masterHost {
	masterRawPodExists = true
	break
	}
	}
	if masterRawPodExists {
	// The Sentinel-reported master maps to a real pod, but that pod is not
	// currently present in connected nodes. Avoid healing to prevent fighting
	// Sentinel during a transient connectivity issue.
	logger.Info("master not in connected nodes but matching raw pod exists, allowing label update",
	"master", monitorMaster.Address())
	return nil
	}
	}

[Copilot]

inst.RawNodes(ctx) is called twice in isNodesHealthy (once in the masterNode == nil branch and again before the index check). Since RawNodes lists pods from the API, consider fetching it once and reusing the result to avoid duplicate list calls in the path where masterNode == nil but healing continues.

[Copilot]

The test name TestIsNodesHealthy_IndexMismatch_WhenMasterNodeUnreachable doesn't match the scenario described in the comment/body (Sentinel master is reachable; a different pod is missing from Nodes()). Consider renaming the test so its intent matches what it's asserting.

[Copilot]

The err == ErrNoMaster || strings.Contains(err.Error(), "no such host") branch logs "master not registered" even when the error indicates DNS/host resolution failure for the Sentinel address. This is misleading for debugging network/DNS incidents; consider splitting the cases and logging a distinct message (and reason) for the "no such host" condition.

[Copilot]

If inst.RawNodes(ctx) fails here, the code logs the error but then proceeds as if the master is down and triggers CommandHealMonitor. For transient API issues this can cause unnecessary healing and churn; consider returning actor.RequeueWithError(rawErr) (or otherwise propagating the error) for consistency with the later RawNodes usage in this function.

Suggested change

	logger.Error(rawErr, "failed to get raw nodes; treating master as down")
	logger.Error(rawErr, "failed to get raw nodes")
	return actor.RequeueWithError(rawErr)

[chideat]

here can's use pod.Status.PodIP, for monitorMaster.IP many be a announce address, for example NodePort or LoadBalancer

[chideat]

Thanks for tackling sentinel network-partition tolerance — the high-level direction (skip unreachable sentinels so a quorum can still act, defer to sentinel instead of aborting) is the right one. However, I'm requesting changes: a few of the error-classification changes turn transient/normal conditions into active heal operations against a healthy cluster, and the core changed logic isn't covered by the new tests.

Merge blockers

1. isNetworkError swallows context.Canceled / context.DeadlineExceeded → spurious CommandHealMonitor
internal/valkey/failover/monitor/sentinel_monitor.go (new helper, ~L52). Verified against redigo: redis.DoContext returns ctx.Err() unwrapped, so errors.Is matches. When the reconcile context is canceled (leader change/shutdown) or the 5-min DefaultReconcileTimeout is hit mid-loop, every sentinel call returns a context error → all skipped → masterStat empty → return nil, ErrNoMaster → engine.go:291 maps ErrNoMaster to CommandHealMonitor. Pre-PR these hit return nil, err → RequeueWithError (a safe retry). Context errors should not be classified as transient/skippable.

2. Full operator↔sentinel partition now heals instead of requeues
Same sink as #1, different trigger: when all sentinels are unreachable, the old code returned the raw *net.OpError (→ RequeueWithError); the new code skips them all → ErrNoMaster → CommandHealMonitor. Please distinguish "no sentinel reachable" (requeue) from "sentinels agree there is no master" (heal).

3. idx >= len(rawPods) false-heals when rawPods is empty
internal/ops/failover/engine.go:358. Failover.RawNodes returns (nil, nil) when the StatefulSet is NotFound (failover.go:328). inst.Nodes() comes from an earlier snapshot, so it can be non-empty while a fresh RawNodes() returns empty → else branch → idx >= 0 is true for every node → CommandHealPod on a healthy cluster during a transient list/STS blip. Guard the empty case before the loop.

4. UpdateConfig skips DNS errors but not network errors (asymmetric with Master)
internal/valkey/failover/monitor/sentinel_monitor.go:467. This PR changed the line to isDNSError but didn't add the isNetworkError skip that Master() got. A network-unreachable sentinel makes UpdateConfig return err; since it runs earlier in the reconcile than the partition-tolerant Master() path, the reconcile bails on the same node the PR is trying to tolerate — defeating the fix for the config path. Add the isNetworkError skip here too.

5. Tests don't exercise the changed code
internal/ops/failover/engine_test.go: mockFailoverMonitor.Master() returns m.master directly, so the new isDNSError/isNetworkError/quorum-skip logic in SentinelMonitor.Master() — the heart of the PR — is never tested. Also, the "unreachable pod with reachable master" case never enters the new masterNode == nil block (ha-1 is the reachable master, so masterNode is non-nil). Please drive the real SentinelMonitor with injected client errors (DNS / connection-refused / context-canceled) and add coverage for the empty-rawPods branch.

Recommended (non-blocking) cleanups

• Quorum weakened: sentinel_monitor.go:255 — with unreachable sentinels skipped, masterStat[0].Count == registeredNodes lets a single minority sentinel be authoritative during a partition (e.g. 1 of 3 reachable still returns a master). Worth reconsidering.
• Fragile heuristic: engine.go:344 — len(inst.Nodes()) == len(rawPods) can hold while membership differs (rolling replacement), so the strict ordering branch runs on a mismatched set. Compare identity, not counts.
• Empty-string match: engine.go:317 — pod.Status.PodIP == monitorMaster.IP lacks a non-empty guard (a Pending pod with PodIP=="" matching an empty master IP suppresses a needed heal). Mirror the adjacent announceIP != "" check.
• Redundant API list + TOCTOU: engine.go:309 and :338 each List pods; hoist to one call and reuse — also closes the window where the two snapshots disagree.
• Convention: repo CLAUDE.md specifies Ginkgo v2 + Gomega for unit tests; the new file uses testing + testify.

Pre-existing crashes adjacent to these paths (worth fixing while here)

• internal/valkey/failover/monitor/sentinel_node.go:115 — in UpdateConfig, strings.Contains(err.Error(), ...) runs where err is already nil (after the earlier return), so any erroring pipeline result panics. Should be ret.Error.Error().
• internal/ops/failover/actor/actor_patch_labels.go:65 — actor.RequeueWithError(err) is missing a return, then masterNode.IP is dereferenced at L76 → nil-deref on any Master() error (more reachable now via #1/#2).

Happy to re-review once #1–#5 are addressed.