feat: stage pod template rolls via Spec.WorkloadRevision - #338
Conversation
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[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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
left a comment
jdheyburn
left a comment
There was a problem hiding this comment.
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.
|
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 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 (the DRY point on |
|
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>
There was a problem hiding this comment.
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.
| // 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. |
There was a problem hiding this comment.
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
left a comment
jdheyburn
left a comment
There was a problem hiding this comment.
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
Do not run proactive failover when Spec only backfills an empty WorkloadRevision (template unchanged). Scrape topology only when a failover-aware roll is needed. Document WorkloadDrift, case StatefulSet explicitly in buildNodePodTemplate, DRY non-template workload sync, and scope the ACL-hash e2e pod lookups to the sample cluster. Signed-off-by: daanvinken <daanvinken@tythus.com>
daanvinken
changed the title
|
Thanks @jdheyburn , sorry was a bit rushed on Thursday.
|
📝 WalkthroughWalkthroughThe change adds workload revision authorization and drift reporting. Controllers compute stable pod-template revisions, coordinate failover decisions, and defer cluster-owned workload rolls until revisions match. Probe defaults are normalized, and tests cover rollout gating. ChangesWorkload rollout coordination
Sequence Diagram(s)sequenceDiagram
participant ValkeyClusterController
participant ACLSecret
participant failover
participant ValkeyNodeController
participant StatefulSet
ValkeyClusterController->>ACLSecret: read ACL data
ValkeyClusterController->>ValkeyNodeController: pass ACL and live template snapshots
ValkeyClusterController->>failover: compute desired revision and roll decision
failover->>ValkeyClusterController: permit or defer reconciliation
ValkeyNodeController->>ValkeyNodeController: compare authorized and desired revisions
alt Revision matches
ValkeyNodeController->>StatefulSet: apply template update
else Revision mismatch
ValkeyNodeController->>ValkeyNodeController: record drift and requeue
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: plugin(logcheck): plugin "logcheck" not found Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/controller/valkeynode_controller_test.go (1)
677-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
ctrl, and add coverage for the standalone immediate-apply path.Two points on this new test:
ctrl := trueshadows the conventionalctrlalias forsigs.k8s.io/controller-runtime. Rename it toisController, or useptr.To(true).- The gate has two branches. This test covers only the cluster-owned branch.
gateRollingWorkloadUpdatereturnstrueimmediately whenisClusterOwned(node)is false, and the PR states that standalone nodes apply template changes without waiting. Add a case that changesSpec.Imageon a node with no controller owner reference and asserts that the StatefulSet template advances in the same reconcile and that noWorkloadDriftcondition appears.Consider also asserting
result.RequeueAfter == 30 * time.Secondon the deferred reconcile, so the backoff contract inReconcileis pinned.♻️ Proposed rename
- ctrl := true + isController := true @@ - Controller: &ctrl, + Controller: &isController,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_controller_test.go` around lines 677 - 698, Rename the local ctrl boolean in the cluster-owned rolling-update test to isController (or use the established pointer helper), then add coverage for the standalone path: update Spec.Image on a ValkeyNode without a controller owner, reconcile once, and assert the StatefulSet template advances immediately without a WorkloadDrift condition. Also assert the deferred cluster-owned reconcile returns a 30-second RequeueAfter.internal/controller/valkeynode_controller.go (2)
444-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe "fresh read" uses the cache, and it mutates the caller's
node.Two points:
r.Getreads through the manager cache, so it can still return the staleSpec.WorkloadRevision. The comment promises a fresh read.ValkeyNodeReconcileralready holdsAPIReaderfor uncached reads. Either useAPIReaderhere or soften the comment, because the current code relies on the watch plus the 30-second requeue to converge.node.Spec = fresh.Specmutates the caller's object inside a function that reads as a pure predicate.desiredwas already built from the previous spec, sonode.Specanddesireddisagree after this line. No current caller depends on the mutation, so removing it makes the contract clearer.♻️ Proposed change
- // Fresh read: cluster may have advanced Spec.WorkloadRevision after this reconcile started. + // Uncached read: the cluster may have advanced Spec.WorkloadRevision after + // this reconcile started and the cache may not have observed it yet. fresh := &valkeyiov1alpha1.ValkeyNode{} - if err := r.Get(ctx, client.ObjectKeyFromObject(node), fresh); err != nil { + if err := r.APIReader.Get(ctx, client.ObjectKeyFromObject(node), fresh); err != nil { return false, err } - node.Spec = fresh.Spec if workloadRevisionAllows(fresh, desiredHash) {
APIReaderis nil in the unit tests ininternal/controller/valkeynode_controller_test.go, so set it when you adopt this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_controller.go` around lines 444 - 449, Update the fresh-read logic in the relevant ValkeyNodeReconciler predicate to use APIReader for an uncached read, and remove the node.Spec = fresh.Spec mutation so the predicate does not alter its caller. Ensure APIReader is initialized in the unit-test setup where it is currently nil, while preserving the existing error return and desired-state evaluation.
528-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the signature, and note that workload annotations are never synced.
The helper takes seven parameters, two of them
any, while it already receivesobj client.Object. Labels and owner references are readable fromobj, and the kind is available from the object type. Passing the specs asanyremoves compile-time type checking; a future call site that swaps aStatefulSetSpecfor aDeploymentSpecwould compile and always report "changed".Separately, neither this helper nor the two callers copy
desired.Annotationsonto the live object. IfbuildValkeyNodeStatefulSetorbuildValkeyNodeDeploymentsets object-level annotations, those annotations are applied only at create time and never reconciled afterwards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_controller.go` around lines 528 - 546, Refactor the maybeUpdateWorkloadWithoutRoll function signature to remove redundant parameters (beforeLabels, afterLabels, beforeOwners, afterOwners, kind, name) by extracting labels and owner references directly from the obj parameter and deriving the kind from obj's type. Replace the `any` types for beforeSpec and afterSpec with properly typed specs to restore compile-time type safety in the comparison logic. Additionally, add annotation synchronization to copy desired annotations onto the live object during the update, ensuring annotations set by buildValkeyNodeStatefulSet or buildValkeyNodeDeployment are reconciled on every update and not just at creation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/valkeycluster_controller.go`:
- Around line 546-552: Update
internal/controller/valkeycluster_controller.go#L546-L552 in
reconcileValkeyNodes to preserve the successful ACL Secret snapshot, continue
treating only NotFound as absent, and return any other getClusterACLSecret
error. Update internal/controller/valkeycluster_controller.go#L620-L626 to pass
that snapshot into reconcileValkeyNode and use it for WorkloadRevision
calculation instead of performing a second Secret read.
- Around line 694-699: When the proactiveFailover call fails or times out at the
error handling block around lines 694-699, change the return value from false to
true. This will cause the controller to requeue before proceeding with the
workload/template update (which would otherwise be authorized by the false
return at CreateOrUpdate), ensuring the primary is not rolled immediately after
a failed proactive failover.
In `@internal/controller/valkeynode_controller.go`:
- Around line 150-159: Reorder the reconciliation flow in
valkeynode_controller.go so that the call to applyLiveConfig (and any clearing
of stale conditions) executes before the workload drift check using
meta.IsStatusConditionTrue and
valkeyiov1alpha1.ValkeyNodeConditionWorkloadDrift. Move the early return with
RequeueAfter to occur after the live config application so that the status
update propagates before requeuing, allowing drifted nodes waiting for
Spec.WorkloadRevision to unblock cluster progression by first applying live
config.
- Around line 422-431: The getACLSecret function does not validate that the
clusterName parameter is non-empty before constructing the internal secret name
via getInternalSecretName, causing silent failures when the valkey.io/cluster
label is missing. Add a validation check at the start of getACLSecret to return
an error with a clear message if clusterName is empty, allowing the controller
to fail fast instead of continuing through the reconciliation path.
---
Nitpick comments:
In `@internal/controller/valkeynode_controller_test.go`:
- Around line 677-698: Rename the local ctrl boolean in the cluster-owned
rolling-update test to isController (or use the established pointer helper),
then add coverage for the standalone path: update Spec.Image on a ValkeyNode
without a controller owner, reconcile once, and assert the StatefulSet template
advances immediately without a WorkloadDrift condition. Also assert the deferred
cluster-owned reconcile returns a 30-second RequeueAfter.
In `@internal/controller/valkeynode_controller.go`:
- Around line 444-449: Update the fresh-read logic in the relevant
ValkeyNodeReconciler predicate to use APIReader for an uncached read, and remove
the node.Spec = fresh.Spec mutation so the predicate does not alter its caller.
Ensure APIReader is initialized in the unit-test setup where it is currently
nil, while preserving the existing error return and desired-state evaluation.
- Around line 528-546: Refactor the maybeUpdateWorkloadWithoutRoll function
signature to remove redundant parameters (beforeLabels, afterLabels,
beforeOwners, afterOwners, kind, name) by extracting labels and owner references
directly from the obj parameter and deriving the kind from obj's type. Replace
the `any` types for beforeSpec and afterSpec with properly typed specs to
restore compile-time type safety in the comparison logic. Additionally, add
annotation synchronization to copy desired annotations onto the live object
during the update, ensuring annotations set by buildValkeyNodeStatefulSet or
buildValkeyNodeDeployment are reconciled on every update and not just at
creation time.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6a1513a-c8af-4320-a563-c02db82839a9
📒 Files selected for processing (13)
api/v1alpha1/valkeynode_types.goconfig/crd/bases/valkey.io_valkeynodes.yamldocs/status-conditions.mdinternal/controller/failover.gointernal/controller/failover_test.gointernal/controller/valkeycluster_controller.gointernal/controller/valkeynode_controller.gointernal/controller/valkeynode_controller_test.gointernal/controller/valkeynode_resources.gointernal/controller/valkeynode_resources_test.gointernal/controller/workload_roll.gointernal/controller/workload_roll_test.gotest/e2e/valkeycluster_test.go
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Use live StatefulSet/Deployment template hashes so empty WorkloadRevision backfill skips proactive failover only when the live template already matches. Share one ACL secret snapshot for preflight and Spec updates. Defer Spec rolls when proactive failover fails. Apply live config before WorkloadDrift requeue so drifted nodes do not block on stale LiveConfig. Signed-off-by: daanvinken <daanvinken@tythus.com>
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
This PR looks great to me, just a comment about the condition.
(there is also something with ACL-roll in the e2e, I have no clue yet though)
Drift reads as an error; this is expected staging while Spec.WorkloadRevision catches up. Reason stays AwaitingWorkloadRevision. Signed-off-by: daanvinken <daanvinken@tythus.com>
Spec.WorkloadRevision applies ACL template updates one node at a time, so items[0] may keep the old hash while another node has already rolled. Assert at least one cluster pod shows a new internal-acl-hash. Signed-off-by: daanvinken <daanvinken@tythus.com>
|
About the E2E; creates a Valkey Cluster deployment (end of the happy path):
Before Now an ACL hash change is a template change. Apply waits on Spec.WorkloadRevision, which the cluster advances one node at a time, with failover-aware handling when live template ≠ authorized hash. Fixing by keeping the secret recreated + fallback log checks. Drop the assumption of an immediate flip on a single arbitrary pod (items[0]). Assert that ACL hash progresses for the cluster (any or all pods under the cluster |
|
@jdheyburn @bjosv I think we're good here now. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/valkeycluster_test.go (2)
1647-1649: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the existing zone label before mutation.
The lookup error is discarded. If this lookup fails but the later label command succeeds, cleanup treats the prior value as empty and removes an existing zone label. Fail before changing the node label when the lookup fails.
Proposed fix
- original, _ := utils.Run(exec.Command("kubectl", "get", "node", w, + original, err := utils.Run(exec.Command("kubectl", "get", "node", w, "-o", "jsonpath={.metadata.labels['topology.kubernetes.io/zone']}")) + Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Failed to get zone label for node %s", w)) original = strings.TrimSpace(original)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_test.go` around lines 1647 - 1649, Handle the error returned by utils.Run when retrieving the node’s existing zone label in the mutation setup around the kubectl lookup. Abort and return the error before executing any label-changing command if the lookup fails, while preserving the trimmed original label value on success.
605-619: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: Internal · Exploitability: Moderate
Do not log the decoded operator password in command arguments.
operatorPasswordis decoded from a Secret and passed into commands thatutils.Runlogs viaGinkgoWriter, so the password is exposed in e2e run output. Use secret-backed client-pod configuration, environment injection inside the client pod, or avoid logging the full command arguments.
test/e2e/valkeycluster_test.go#L495-550: remove-a operatorPasswordfrom the initialvalkey-cli ACL LISTcommand.test/e2e/valkeycluster_test.go#L605-619: removeoperatorPasswordfrom thesh -cargument.test/e2e/valkeycluster_test.go#L671-676: removeoperatorPasswordfrom thesh -cargument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_test.go` around lines 605 - 619, Stop exposing the decoded operator password through logged command arguments in the Valkey e2e tests: remove the -a operatorPassword usage from the initial ACL LIST command and remove operatorPassword from the sh -c arguments in the command blocks at test/e2e/valkeycluster_test.go lines 605-619 and 671-676. Use secret-backed client-pod configuration or environment injection so authentication still works without logging the password.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/e2e/valkeycluster_test.go`:
- Around line 1647-1649: Handle the error returned by utils.Run when retrieving
the node’s existing zone label in the mutation setup around the kubectl lookup.
Abort and return the error before executing any label-changing command if the
lookup fails, while preserving the trimmed original label value on success.
- Around line 605-619: Stop exposing the decoded operator password through
logged command arguments in the Valkey e2e tests: remove the -a operatorPassword
usage from the initial ACL LIST command and remove operatorPassword from the sh
-c arguments in the command blocks at test/e2e/valkeycluster_test.go lines
605-619 and 671-676. Use secret-backed client-pod configuration or environment
injection so authentication still works without logging the password.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ff33d4e-7853-4c30-ac8a-91506f8910a5
📒 Files selected for processing (4)
config/crd/bases/valkey.io_valkeynodes.yamlinternal/controller/valkeycluster_controller.gointernal/controller/valkeynode_resources_test.gotest/e2e/valkeycluster_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/valkeycluster_controller.go
- internal/controller/valkeynode_resources_test.go
- config/crd/bases/valkey.io_valkeynodes.yaml
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
LGTM and works fine in manual tests (like upgrading the operator with new default)
ACL was carried in the pod template annotations, so an ACL edit changed Spec.WorkloadRevision and rolled every node one at a time (valkey-io#338). The operator can reload the ACL on a running server with ACL LOAD, so a roll is unnecessary: this takes ACL out of the WorkloadRevision hash and reloads the mounted aclfile live instead. - buildPodTemplateAnnotations no longer stamps the ACL hash, so ACL edits do not enter the workload revision and never roll a pod. - the ValkeyNode controller reloads the mounted aclfile (ACL LOAD) on reconcile and watches the internal ACL Secret, so an edit is picked up promptly rather than on the next resync. It reports an ACLApplied condition once the desired user set and password hashes are observably live. - the operator user gains +acl|load, +acl|getuser and +acl|users. - drop the now-dead ACL-secret threading through the cluster controller and the failover roll preflight. Recovering an operator locked out by a deleted password Secret needs a staged recovery through the cluster controller and is left as a follow-up.
ACL was carried in the pod template annotations, so an ACL edit changed Spec.WorkloadRevision and rolled every node one at a time (valkey-io#338). The operator can reload the ACL on a running server with ACL LOAD, so a roll is unnecessary: this takes ACL out of the WorkloadRevision hash and reloads the mounted aclfile live instead. - buildPodTemplateAnnotations no longer stamps the ACL hash, so ACL edits do not enter the workload revision and never roll a pod. - the ValkeyNode controller reloads the mounted aclfile (ACL LOAD) on reconcile and watches the internal ACL Secret, so an edit is picked up promptly rather than on the next resync. It reports an ACLApplied condition once the desired user set and password hashes are observably live. - the operator user gains +acl|load, +acl|getuser and +acl|users. - drop the now-dead ACL-secret threading through the cluster controller and the failover roll preflight. Recovering an operator locked out by a deleted password Secret needs a staged recovery through the cluster controller and is left as a follow-up. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
ACL was carried in the pod template annotations, so an ACL edit changed Spec.WorkloadRevision and rolled every node one at a time (valkey-io#338). The operator can reload the ACL on a running server with ACL LOAD, so a roll is unnecessary: this takes ACL out of the WorkloadRevision hash and reloads the mounted aclfile live instead. - buildPodTemplateAnnotations no longer stamps the ACL hash, so ACL edits do not enter the workload revision and never roll a pod. - the ValkeyNode controller reloads the mounted aclfile (ACL LOAD) on reconcile and watches the internal ACL Secret, so an edit is picked up promptly rather than on the next resync. It reports an ACLApplied condition once the desired user set and password hashes are observably live. - the operator user gains +acl|load, +acl|getuser and +acl|users. - drop the now-dead ACL-secret threading through the cluster controller and the failover roll preflight. Recovering an operator locked out by a deleted password Secret needs a staged recovery through the cluster controller and is left as a follow-up. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
<!-- Thanks for contributing to Valkey Operator! Please make sure you are aware of our contributing guidelines [available here](https://github.com/valkey-io/valkey-operator/blob/main/CONTRIBUTING.md) --> This PR closes #264 ### Summary This is a refactor to remove `serverConfigHash` from ValkeyNode. Since #338 introduced a `WorkloadRevision` to ValkeyNode, there is duplicated functionality which means we no longer need a `serverConfigHash`. ### Features / Behaviour Changes - ValkeyNode.spec.serverConfigHash is removed from the CRD (internal, operator-managed field; nothing outside the operator reads it). - No runtime behaviour changes: config-change rolls, live-config apply (CONFIG SET), and ACL live-apply are unchanged. A frozen pin test (internal/controller/config_rollhash_test.go) guarantees the derived hash is byte-identical to the previously stamped one, so upgrading the operator rolls zero pods. ### Implementation Simply removing a field that has had its function duplicated elsewhere. - The ValkeyNode pod-template builders now derive the config-hash annotation themselves (nodeServerConfigRollHash), gated on `spec.serverConfigMapName` - The render core in config.go is parent-agnostic so cluster and node sides produce the same output bytes - The cluster controller no longer computes or threads a config hash, shrinking the parent→ValkeyNode contract: set the real inputs, call setDesiredWorkloadRevision. - `spec.workloadRevision` remains the single roll-control field ### Limitations During a mixed state, its possible that the operator would cause some rolls and failovers, however once CRDs and operator is synced up then this is not expected. ### Testing Beyond unit tests, tested on a kind cluster to verify that: - change in maxmemory-policy is applied live and does not cause a pod roll - change in io-threads causes a pod roll ### Checklist Before submitting the PR make sure the following are checked: - [x] This Pull Request is related to one issue. - [x] Commit message explains what changed and why - [x] Tests are added or updated. - [x] Documentation files are updated. - [ ] I have run pre-commit locally (`pre-commit run --all-files` or hooks on commit) Signed-off-by: Joseph Heyburn <jdheyburn@gmail.com>
4 participants
This PR closes #337
Summary
ValkeyCluster already stages ValkeyNode Spec updates carefully (one node at a time, replicas first, failover before primary). That care stopped at the ValkeyNode object: each node rewrote its single-pod StatefulSet/Deployment as soon as the computed pod template differed, so operator upgrades and other builder-only template changes could restart every pod at once.
This PR makes the authorized pod template a Spec field the cluster controller owns.
Features / Behaviour Changes
ValkeyNode.spec.workloadRevision: hash of the fully built pod template, set by the ValkeyCluster controller.spec.workloadRevision.rollingStrategyonly needs to change how many Specs advance per reconcile.workloadRevision(operator upgrade onto this feature) does not run proactive failover; it is bookkeeping only.WorkloadRollPending/AwaitingWorkloadRevisionwhen the node is waiting for Spec to catch up.Implementation
computeWorkloadRevision/buildNodePodTemplate).WorkloadRevisionon desired Spec; scrape topology only when a failover-aware roll is needed.Limitations
Testing
go test ./internal/controller/andmake lintpass.Checklist
pre-commit run --all-filesor hooks on commit)