Pre-flight checks
Problem / Use case
Every node of a ValkeyCluster mounts the same ConfigMap, valkey-<cluster>, handed to each ValkeyNode as ServerConfigMapName. One object holds the rendered valkey.conf for nodes that are not required to be identical, and during an image change they are not.
The controller renders that file from cluster.Spec.Image, the image the user asked for. The pods reach that image one at a time, because buildClusterValkeyNode copies the new image onto every ValkeyNode immediately while the StatefulSet template update waits for Spec.WorkloadRevision. So mid-roll the desired image is 9.1 everywhere and half the running binaries are still 9.0.
That gap is only cosmetic until the config contains something version-dependent. Valkey refuses to start on a directive it does not recognise, so a 9.0 pod that restarts mid-roll for any reason, a drain, an OOM kill, a node replacement, mounts a file with a 9.1 directive in it and crash-loops. Nothing in the rollout caused the restart, so nothing sequences it.
This is not hypothetical. #307 adds version-gated directives and hits exactly this; the gap is documented there, was raised independently in review, and @bjosv suggested the structural fix quoted below. It also constrains anything else that wants to emit a directive conditionally: shutdown-on-sigterm is already emitted unconditionally today with a comment saying it needs Valkey 9.0+.
Proposed solution
From @bjosv on #307:
Maybe we should do the move from having a common ConfigMap to having one for each ValkeyNode (but keep common scripts).
Render valkey.conf per ValkeyNode, from that node's own image, and keep the health-check scripts in a shared ConfigMap since they do not vary.
With per-node rendering there is no gate to compute. A node's config is written for the binary that will read it, so there is no floor to derive and no ordering requirement between the ConfigMap write and the roll. The alternative discussed on #307, deriving a minimum version across running nodes and suppressing gated directives until every pod clears it, keeps the shared object and pays for it with a floor that has to be recomputed on every reconcile and is wrong whenever the operator cannot see a node.
A second effect worth naming: today any change to the shared file is one write that every node sees at once, whether or not the change concerns it. Per-node files make config changes as granular as the roll already is.
Scope of change
Medium (controller behaviour and object layout, no user-facing API change)
API / behaviour impact
ServerConfigMapName already lives on ValkeyNode.Spec, so the node side needs no new field. Existing clusters need the old shared ConfigMap cleaned up once every node points at its own.
Contribution
Additional context
Worth deciding early, since it changes what a fix for the #307 race should look like: with per-node ConfigMaps the observed-minimum-version machinery discussed there is not needed, and building it first would be work thrown away.
Object count grows from one per cluster to one per node plus one shared scripts ConfigMap. For the cluster sizes this operator targets that seems a fair trade for removing a class of crash-loop, but it is the obvious cost and belongs in the decision.
Raised because @jdheyburn asked for an issue to track it on #307.
Pre-flight checks
Problem / Use case
Every node of a ValkeyCluster mounts the same ConfigMap,
valkey-<cluster>, handed to each ValkeyNode asServerConfigMapName. One object holds the renderedvalkey.conffor nodes that are not required to be identical, and during an image change they are not.The controller renders that file from
cluster.Spec.Image, the image the user asked for. The pods reach that image one at a time, becausebuildClusterValkeyNodecopies the new image onto every ValkeyNode immediately while the StatefulSet template update waits forSpec.WorkloadRevision. So mid-roll the desired image is 9.1 everywhere and half the running binaries are still 9.0.That gap is only cosmetic until the config contains something version-dependent. Valkey refuses to start on a directive it does not recognise, so a 9.0 pod that restarts mid-roll for any reason, a drain, an OOM kill, a node replacement, mounts a file with a 9.1 directive in it and crash-loops. Nothing in the rollout caused the restart, so nothing sequences it.
This is not hypothetical. #307 adds version-gated directives and hits exactly this; the gap is documented there, was raised independently in review, and @bjosv suggested the structural fix quoted below. It also constrains anything else that wants to emit a directive conditionally:
shutdown-on-sigtermis already emitted unconditionally today with a comment saying it needs Valkey 9.0+.Proposed solution
From @bjosv on #307:
Render
valkey.confper ValkeyNode, from that node's own image, and keep the health-check scripts in a shared ConfigMap since they do not vary.With per-node rendering there is no gate to compute. A node's config is written for the binary that will read it, so there is no floor to derive and no ordering requirement between the ConfigMap write and the roll. The alternative discussed on #307, deriving a minimum version across running nodes and suppressing gated directives until every pod clears it, keeps the shared object and pays for it with a floor that has to be recomputed on every reconcile and is wrong whenever the operator cannot see a node.
A second effect worth naming: today any change to the shared file is one write that every node sees at once, whether or not the change concerns it. Per-node files make config changes as granular as the roll already is.
Scope of change
Medium (controller behaviour and object layout, no user-facing API change)
API / behaviour impact
v1alpha1.ServerConfigMapNamealready lives on ValkeyNode.Spec, so the node side needs no new field. Existing clusters need the old shared ConfigMap cleaned up once every node points at its own.Contribution
Additional context
Worth deciding early, since it changes what a fix for the #307 race should look like: with per-node ConfigMaps the observed-minimum-version machinery discussed there is not needed, and building it first would be work thrown away.
Object count grows from one per cluster to one per node plus one shared scripts ConfigMap. For the cluster sizes this operator targets that seems a fair trade for removing a class of crash-loop, but it is the obvious cost and belongs in the decision.
Raised because @jdheyburn asked for an issue to track it on #307.