Seed a master only where every Redis holds no data - #116
Draft
indiebrain wants to merge 3 commits into
Draft
Conversation
The operator promotes a node in two places when it finds no master: when Sentinel has no quorum, and when every node reports localhost as its master. Neither established that the nodes were empty first. A failover whose master is gone and whose replicas are still up holding data reaches both, and the node promoted is the oldest pod, which says nothing about how far its data is behind. ADR-001 already rules this out. It says the operator may seed an initial master, because every candidate in an empty cluster is equivalent, and must not choose among nodes that may hold data, because it has no sound basis for ranking them. That decision recorded a signal it needed and did not have: something able to tell a cold start from a restart. Asking who a node replicates from cannot, since a restarted pod reloads `slaveof 127.0.0.1` from its generated config while keeping its dataset. Ask the keyspace instead. `HoldsNoData` reads the keyspace section of `INFO`, which lists a line per database holding keys and comes back empty when there are none, and `CheckIfAllRedisHoldNoData` requires that of every running node. Both selecting paths now sit behind it. A node that holds keys, or that cannot be reached to be asked, stops the operator, which records `MasterUnknown` naming the cause and changes nothing. The failover with one replica is untouched: a single candidate is not a choice among nodes. This trades availability for not losing writes, which is the trade ADR-001 accepts. A no-quorum event on a cluster holding data now stops and stays stopped until a person looks, where it previously recovered by promoting a node that may have been behind.
|
A change to documentation files was detected in your PR. Please visit this link to preview changes: https://portal-staging.powerapp.cloud/docs?filters[kind]=all&filters[user]=all&filters[namespaceFilter]=2026-09-04--seed-only-an-empty-cluster |
Reading the keyspace to decide whether a node holds data has a window where it lies. `INFO` is served while Redis reads its dataset from disk, and the keyspace section fills in as keys are inserted, so a node holding a full dataset reports an empty keyspace for as long as loading takes. The node list makes that reachable. `GetRedisesIPs` selects on the pod phase being `Running` rather than on the container being ready, so a pod whose Redis is still loading is asked and answers. A whole cluster coming back after an outage is exactly when every node is loading at once, and it is also exactly when seeding a master would discard the data being loaded. Report loading as an error, so the operator holds off and a later reconcile asks again once the node can answer. `async_loading` counts the same, since a replica loading a dataset diskless reports it there. Read the default INFO sections rather than a named one: persistence and keyspace are both needed, and naming several sections in one INFO is only supported from Redis 7. The interpretation is split out from the call so it can be tested without a running Redis, including the partly-loaded keyspace that motivates it.
indiebrain
marked this pull request as draft
September 4, 2026 20:18
`CheckIfAllRedisHoldNoData` states the safe outcome as the absence of something, so every call site negated a negative to recover one fact. Ask whether any node has data instead. `HasData` on the client reports what it found, `CheckIfAnyRedisHasData` reports whether any node holds keys, and the caller reads `if hasData` where it read `if !empty`. An unreachable node still stops the operator. The value returned alongside the error changes from false to true so that a caller ignoring the error reaches the safe outcome rather than the dangerous one.
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
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.
The gap
CheckAndHealpromotes a node in two places when it finds no master:checker.go, the branch taken whenCheckSentinelQuorumreturns an error. It selected unconditionally.checker.go, the branch whereCheckIfMasterLocalhostis true. It selected behind a guard that cannot do what it is being asked to do.A failover whose master is gone and whose replicas are still up holding data reaches both.
nMastersis 0, quorum is gone, andSetOldestAsMasterpromotes byCreationTimestamp, which says nothing about how far a node's data is behind.This is already decided
ADR-001 says the operator seeds an initial master and does not otherwise choose, and that it "does not choose among nodes that may hold data". The rule held in the decision record and not in the operator.
That ADR also named the signal it needed and did not have:
This adds it.
The change
HoldsNoDataon the Redis client reads the keyspace section ofINFO, which lists a line per database holding keys and comes back empty when there are none.DBSIZEwould only answer for the database the client selected.CheckIfAllRedisHoldNoDatarequires that of every running node.checkSeedingAllowedsits in front of both selecting paths. A node that holds keys, or that cannot be reached to be asked, stops the operator, which recordsMasterUnknownnaming the cause and changes nothing.A Redis reading its dataset from disk is an error, not an empty one. This is the race the check turns on.
INFOis served whileloading:1, and the keyspace section fills in as keys are inserted, so a node holding a full dataset on disk reports an empty keyspace until enough of it is in memory. It is reachable:GetRedisesIPsselects on the pod phase beingRunning, not on the container being ready, so a pod whose Redis is still loading is asked and answers. A whole cluster returning after an outage is precisely when every node is loading at once, and precisely when seeding would discard what is being loaded.async_loadingcounts the same, for a replica loading diskless.A node that cannot be reached counts as holding data. The failure modes are not symmetric: treating an unreachable node as empty lets the operator promote over data it could not see, while treating it as holding data costs an availability stop a person can resolve.
The single-replica path is unchanged. One candidate is not a choice among nodes, and gating it would stop a standalone failover from ever getting a master.
What this costs
A no-quorum event on a cluster holding data now stops and stays stopped until someone looks, where it previously recovered by promoting a node that may have been behind. That is the trade ADR-001 accepts explicitly ("Recovery can stop and stay stopped… the deliberate price of not guessing"), extended to the path that was still guessing. Worth knowing for whoever operates these clusters: the failure is visible as the
MasterUnknowncondition inkubectl get redisfailoverrather than silent.On issue #100
This is the part of #100 that is still open and buildable. It does not implement that issue's request to select by replication offset: ADR-001 investigated and rejected that on measurement against
redis:8.10.1, so the operator does not rank nodes at all. #100 stays open regardless, since the integration coverage it asks for is not here.Also in this change
SetOldestAsMaster"is now reached only when seeding", which was not true of the code and is true after this. The paragraph about the missing cold-start signal is updated to describe the one that now exists.reportMasterUnknown's message template is generalized. It hardcoded "a Redis node could not be inspected, so one of them may still be the master", which was the only cause when it was written and is not now. Each caller supplies its own cause, so the condition names what actually happened rather than reading as a contradiction.Verification
go build,go vet,gofmt, and the fullgo test ./...pass.Two test cases are added, covering the no-quorum and first-boot paths against a cluster that holds data, asserting that nothing is promoted and the condition is recorded.
The guard is mutation-tested rather than assumed. Replacing
CheckIfAllRedisHoldNoDatawith an unconditionaltruefails the suite:Two existing test cases were changed. Both drive the seeding paths and now set the new expectation. They describe empty clusters, which is what seeding means after this change, so they assert the same outcome for a narrower case rather than a different outcome. Changing tests alongside the code they cover deserves scrutiny, so it is called out here rather than left to be noticed in review.
Not covered
No integration coverage. The harness cannot induce the states this turns on: a node that stays
Runningwhile holding data with no master anywhere, and Sentinel losing quorum on demand. This is the same limitation recorded on #106.