feat: stage pod template rolls behind cluster permit by daanvinken · Pull Request #338 · valkey-io/valkey-operator

daanvinken · 2026-07-27T14:48:25Z

This PR closes #337

Summary

ValkeyCluster already updates ValkeyNode specs carefully: one node at a time, replicas before primaries, with a failover attempt before rolling a primary.

That care stopped at the ValkeyNode object. Each node rewrites its single-pod StatefulSet (or Deployment) as soon as the computed pod template differs, with no coordination across nodes. Image changes and operator upgrades that rewrite the built template could restart every pod in the cluster at once.

This PR extends the same staged idea to the pod template apply path.

Features / Behaviour Changes

Cluster-owned nodes no longer apply StatefulSet or Deployment pod template updates until the ValkeyCluster controller grants a permit (valkey.io/allow-workload-revision).
Permits are granted one node at a time (replicas first, proactive failover before a primary), sharing the same in-flight rule as Spec rolls so Spec and workload rolls cannot race.
While waiting, the node surfaces a WorkloadDrift condition and records the desired template revision for the cluster controller to grant.
Standalone ValkeyNodes (not owned by a cluster) still apply template updates immediately.
Create path is unchanged: new workloads are still created when missing.
A few API-server defaults on containers and probes are owned in the builder so leftover default noise does not keep looking like a real template change.

Implementation

workload_roll.go: hash the desired pod template, read/write permit annotations, decide grant vs wait, shared in-flight check used by both Spec and workload paths.
ValkeyNode controller: compare desired vs live template; if a cluster-owned node would roll without a matching permit, set WorkloadDrift, leave the live workload alone, and requeue.
ValkeyCluster controller: when a node reports drift (or needs a Spec update), grant at most one allow annotation per reconcile after the existing replica-first / failover ordering.
ValkeyNodeConditionWorkloadDrift on the ValkeyNode API for visibility while waiting.

Review focus: permit grant/clear transitions, the shared in-flight check when allow=* is used for Spec rolls, and that waiters do not thrash events on every requeue.

Limitations

Does not add a new user-facing CRD field for roll strategy; behaviour is automatic for cluster-owned nodes.
Permit value is either a template hash or * for Spec-driven rolls where the cluster does not know the hash yet.
Docs for the new condition / annotation are not expanded beyond the API type comment in this PR.

Testing

Unit tests for template hash stability, permit matching, drift condition helpers, and ValkeyNode envtest coverage that a cluster-owned node defers the StatefulSet template update until a permit is present, then applies and clears drift.
go test ./internal/controller/ passes.
Manual kind repro before/after: multi-node cluster, operator/template change; before fix all pods restarted together; after fix StatefulSet applies were staged and the cluster stayed healthy through the roll.

Checklist

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)

Operator image upgrades that change the ValkeyNode pod template were rewriting every 1-pod StatefulSet in one reconcile wave, taking the whole cluster down at once. Gate cluster-owned StatefulSet/Deployment template updates on valkey.io/allow-workload-revision (granted one node at a time by the ValkeyCluster controller, replicas first with proactive failover). Surface WorkloadDrift while waiting. Own API-server defaults so residual default noise does not re-trigger rolls. Spec and workload grants share a single in-flight permit. Standalone ValkeyNodes still apply immediately. Create path unchanged. Signed-off-by: daanvinken <daanvinken@tythus.com>

CI Check formatting failed on third-party vs local import grouping. Signed-off-by: daanvinken <daanvinken@tythus.com>

greptile-apps · 2026-07-27T15:00:27Z

Greptile Summary

This change adds revision-based authorization for ValkeyNode workload-template rollouts, defers unapproved workload drift, preserves one-node-at-a-time ordering, and normalizes workload fields to avoid unnecessary updates.

The rollout path was exercised with a production reconciliation harness. A revision-only update on an existing primary is currently treated as a pod roll even when the pod template is unchanged, which triggers an unnecessary proactive CLUSTER FAILOVER before the revision is persisted.

Merge is not safe until revision backfill is separated from actual workload-template rollout handling.

Confidence Score: 4/5

Not merge-safe: existing primary nodes can be failed over solely to initialize rollout metadata.

The score is 4 because one verified P1 reliability defect remains. The exercised reconciliation path showed that an unchanged workload template with only an empty-to-computed WorkloadRevision change still sends CLUSTER FAILOVER before persisting the revision.

Files Needing Attention: internal/controller/valkeycluster_controller.go and internal/controller/failover.go need adjustment so revision-only updates do not enter the proactive rollout path.

T-Rex Logs

What T-Rex did

T-Rex produced a proof for the posted P1 finding and attached two artifacts: the harness source code and the harness execution log, linked to the review comment that describes the finding.

_{Ran code and verified through T-Rex}

_{Reviews (5): Last reviewed commit: "fix: include WorkloadRevision in roll pr..." | Re-trigger Greptile}

Drop unused anyNodeHasInFlightWorkloadRoll. Split reconcileValkeyNode helpers so gocyclo stays under the limit. When the pod template already matches, still sync labels, owner, and Spec so non-template controller fields do not go stale. Signed-off-by: daanvinken <daanvinken@tythus.com>

jdheyburn

I did a quick scan on the review, but I don't know if I'm sold on the design yet. I would rather understand what is causing the ValkeyCluster controller to push the change out to so many nodes at once. I spent a bit of time on the original PRs to implement safe sequential rolls, so I'm interested to see what's happening.

jdheyburn · 2026-07-27T22:07:50Z

+
+// syncDeploymentWithoutRoll updates labels, owner, and Spec when the pod
+// template would not roll.
+func (r *ValkeyNodeReconciler) syncDeploymentWithoutRoll(ctx context.Context, node *valkeyiov1alpha1.ValkeyNode, dep *appsv1.Deployment, desired *appsv1.Deployment) error {


There's a lot of similarity with syncStatefulSetWithoutRoll. Are we able to DRY these? Something like syncWorkloadWithoutRoll, that might help cut the LOC down.

Fair! We can for sure

melancholictheory · 2026-07-28T09:23:14Z

this is the right extension. the sequencing the cluster does at the CR level was being undone the moment each node rewrote its own template, so gating the template apply behind the same permit closes a real gap. and it's good that maybeProactiveFailoverBeforeRoll fires for the workload path too, not just spec rolls: without that, staging alone would still restart a primary's pod as primary and drop writes. the failover-before-primary is the part that actually matters here, and you kept it.

it also composes well with #317: now that the operator owns the api-server defaults, the template only differs on genuine changes (image, real builder changes), so this stages the changes that should be staged rather than defaulting churn. the two together are the full fix for #337.

one thing to make sure is observable: the workload permit is a single cluster-wide token, so a node whose new pod never becomes Ready (bad image, failing probe) holds it indefinitely and no other node's workload roll proceeds. that's the safe behaviour, you don't want to cascade a broken rollout, but it means one stuck node wedges every other node's template roll silently. the AwaitingRollPermit condition helps, but it's worth distinguishing "waiting its turn" from "the holder is stuck" (the same updating-normally vs stuck-updating distinction from #267), so an operator can tell a slow rollout from a jammed one. a bounded wait or a stuck-holder event would do it.

(the DRY point on syncStatefulSetWithoutRoll seems right too, but that's cosmetics next to the above.)

daanvinken · 2026-07-28T12:39:17Z

Let's take the discussion on the bug itself to #337 (comment)

Replace the allow-workload-revision annotation permit with a Spec field owned by ValkeyCluster. The cluster sets WorkloadRevision to the hash of the built pod template; the node applies rolling template updates only when that hash matches. Operator upgrades and other builder drift become normal one-at-a-time Spec rolls (replica-first, failover before primary). Standalone nodes still apply immediately. Signed-off-by: daanvinken <daanvinken@tythus.com>

anyNodeRequiresRoll must set desired Spec.WorkloadRevision the same way reconcileValkeyNode does, or every settled node looks like it needs a roll and the topology scrape runs every reconcile. Also default Probe.TimeoutSeconds and PeriodSeconds so user probe patches do not thrash the pod template. Signed-off-by: daanvinken <daanvinken@tythus.com>

daanvinken

E2E as a follow up I'd say.

(Meta) comment: A big chunk of the node-controller diff isn't new behavior, it's restructuring ensureStatefulSet/ensureDeployment and the cluster-controller extractions (maybeProactiveFailoverBeforeRoll, handleUnchangedValkeyNode).

We could split that out if this is found hard to review, let me know

I'll squash commits once this is approved.

daanvinken · 2026-07-30T20:08:38Z

+	// proactive failover still runs before killing a primary.
+	aclSecret, err := r.getClusterACLSecret(ctx, cluster)
+	if err != nil {
+		// Bootstrap: secret may not exist yet; hash without ACL annotations.


When the ACL secret isn't ready the cluster hashes without template annotations, but the gate only engages when podTemplateWouldRoll is treu. On create the node bypasses the gate. It converges once the secret exists.

jdheyburn · 2026-08-01T13:26:40Z

@greptile-apps

greptile-apps · 2026-08-01T13:36:23Z

+	needsSpecRoll := currentExists && nodeRequiresRoll(current, desired)
+
+	if deferred := r.maybeProactiveFailoverBeforeRoll(ctx, cluster, clusterState, current, needsSpecRoll); deferred {


Revision-only updates trigger an unnecessary failover

nodeRequiresRoll treats an empty Spec.WorkloadRevision on an existing node as a pod roll even when the live workload template already matches the desired template. That result reaches maybeProactiveFailoverBeforeRoll before CreateOrUpdate persists the revision, so upgrading an existing cluster can deliberately fail over every primary solely to backfill this new bookkeeping field. Gate proactive failover on an actual workload-template change, or persist the revision without considering it a roll.

Artifacts

Revision-only failover executable harness source

The exact temporary Go test source that uses the production reconciliation path and a local RESP server; it asserts an unchanged pod template, an empty-to-desired revision-only change, and receipt of CLUSTER FAILOVER.

Revision-only failover harness output

Captured successful `go test` output showing `podTemplateWouldRoll=false`, `CLUSTER FAILOVER`, failover events before `ValkeyNodeUpdated`, persisted revision, command, working directory, and exit code 0.

_{Ran code and verified through T-Rex}

jdheyburn

I did a local test and it worked great, so thank you! I had a couple of comments.

Can you update the PR title and description with the new setup?
Can you check up on some of the AI code review comments?
Can you check on the failing e2e test?
I agree that we can park an e2e test for this, but let's capture an issue for it so we don't lose track of it

jdheyburn · 2026-08-01T14:03:00Z

+	// Spec.WorkloadRevision has not yet authorized that template. Status True
+	// means a rolling update is deferred until the ValkeyCluster controller
+	// advances Spec.WorkloadRevision (one node at a time today).
+	ValkeyNodeConditionWorkloadDrift = "WorkloadDrift"


Can you document this in docs/status-conditions.md please?

jdheyburn · 2026-08-01T14:09:00Z

+			dep.Spec.Template.Annotations = buildPodTemplateAnnotations(node, aclSecret)
+		}
+		return dep.Spec.Template, nil
+	default:


Can we case on StatefulSet, then error on default?

daanvinken added 2 commits July 27, 2026 16:33

fix: gofmt import order in workload_roll files

8371e8c

CI Check formatting failed on third-party vs local import grouping. Signed-off-by: daanvinken <daanvinken@tythus.com>

greptile-apps Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread internal/controller/valkeynode_controller.go Outdated

jdheyburn reviewed Jul 27, 2026

View reviewed changes

daanvinken added 2 commits July 30, 2026 21:48

daanvinken commented Jul 30, 2026

View reviewed changes

greptile-apps Bot reviewed Aug 1, 2026

View reviewed changes

jdheyburn mentioned this pull request Aug 1, 2026

feat: apply ACL changes live without rolling pods #319

Open

5 tasks

jdheyburn reviewed Aug 1, 2026

View reviewed changes

jdheyburn mentioned this pull request Aug 1, 2026

[chore] Remove serverConfigRollHash from public ValkeyNode CRD spec #264

Open

4 tasks

		needsSpecRoll := currentExists && nodeRequiresRoll(current, desired)

		if deferred := r.maybeProactiveFailoverBeforeRoll(ctx, cluster, clusterState, current, needsSpecRoll); deferred {

Uh oh!

Conversation

daanvinken commented Jul 27, 2026

Summary

Features / Behaviour Changes

Implementation

Limitations

Testing

Checklist

Uh oh!

greptile-apps Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

T-Rex Logs

Uh oh!

Uh oh!

jdheyburn left a comment

Choose a reason for hiding this comment

Uh oh!

jdheyburn Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

daanvinken Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

melancholictheory commented Jul 28, 2026

Uh oh!

daanvinken commented Jul 28, 2026

Uh oh!

daanvinken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daanvinken Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn commented Aug 1, 2026

Uh oh!

greptile-apps Bot Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn left a comment

Choose a reason for hiding this comment

Uh oh!

jdheyburn Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn Aug 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Jul 27, 2026 •

edited

Loading

daanvinken left a comment •

edited

Loading