refactor: Remove serverConfigHash from ValkeyNode

[jdheyburn]

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:

• 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)

[jdheyburn]

@greptile-apps @coderabbitai

[coderabbitai]

@jdheyburn: I will review the pull request.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6c2413e4-c2ac-43a9-9649-31b96f328b99

📥 Commits

Reviewing files that changed from the base of the PR and between a2ab45e and 8b07298.

📒 Files selected for processing (7)

• api/v1alpha1/valkeynode_types.go
• config/crd/bases/valkey.io_valkeynodes.yaml
• internal/controller/config.go
• internal/controller/config_rollhash_test.go
• internal/controller/valkeycluster_controller.go
• internal/controller/valkeycluster_controller_test.go
• internal/controller/valkeynode_controller.go

🚧 Files skipped from review as they are similar to previous changes (7)

• api/v1alpha1/valkeynode_types.go
• internal/controller/valkeynode_controller.go
• config/crd/bases/valkey.io_valkeynodes.yaml
• internal/controller/config.go
• internal/controller/valkeycluster_controller.go
• internal/controller/config_rollhash_test.go
• internal/controller/valkeycluster_controller_test.go

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

The PR removes ServerConfigHash from the public ValkeyNode API. It derives node roll hashes from configuration and TLS, excludes live-settable settings, and uses pod-template annotations with WorkloadRevision for rollout coordination.

Changes

Configuration roll hash flow

Layer / File(s)	Summary
Render and validate derived hashes api/v1alpha1/valkeynode_types.go, config/crd/bases/valkey.io_valkeynodes.yaml, internal/controller/config.go, internal/controller/config_rollhash_test.go, internal/controller/config_test.go, docs/architecture.md	The public ServerConfigHash field and CRD schema entry are removed. Rendering accepts explicit configuration and TLS inputs. Node hashes exclude live-settable keys and retain legacy SHA-256 behavior.
Reconcile nodes with workload revisions internal/controller/failover.go, internal/controller/failover_test.go, internal/controller/valkeycluster_controller.go, internal/controller/valkeycluster_controller_test.go	Node reconciliation no longer assigns Spec.ServerConfigHash. Failover-aware roll detection uses derived template hashes and WorkloadRevision.
Apply hashes to pod templates internal/controller/valkeynode_controller.go, internal/controller/valkeynode_controller_test.go	Parent-managed nodes derive the configHashKey pod-template annotation from node configuration and TLS. Standalone nodes do not receive the annotation. Tests cover hash changes and unchanged live-settable configuration.

Sequence Diagram(s)

sequenceDiagram
  participant ValkeyClusterController
  participant ValkeyNodeController
  participant ConfigRenderer
  participant PodTemplate
  participant WorkloadRevision
  ValkeyClusterController->>ValkeyNodeController: reconcile ValkeyNode without ServerConfigHash
  ValkeyNodeController->>ConfigRenderer: render node configuration and TLS
  ConfigRenderer-->>ValkeyNodeController: return derived configuration hash
  ValkeyNodeController->>PodTemplate: set configHashKey annotation for parent-managed nodes
  ValkeyClusterController->>WorkloadRevision: authorize sequential rollout
  WorkloadRevision-->>ValkeyClusterController: return desired workload revision

Loading

Merge Risk: ⚪ Minimal · up to 8b072

This refactor removes duplicated internal state while preserving configuration rollout behavior; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary change: removing serverConfigHash from ValkeyNode.
Description check	✅ Passed	The description includes all required sections, explains the refactor, documents behavior and limitations, and lists testing; only pre-commit remains unchecked.
Linked Issues check	✅ Passed	The PR removes the CRD field, uses pod-template annotations, preserves hash behavior, updates the schema, and covers the coding requirements in issue #264.
Out of Scope Changes check	✅ Passed	The implementation, tests, CRD update, and documentation changes directly support removing serverConfigHash and preserving roll behavior.

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
The command is terminated due to an 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The change removes the persisted server configuration hash from ValkeyNode resources and derives the pod-template configuration annotation from each node’s resolved configuration and TLS inputs. Focused hash-contract coverage passed for legacy compatibility, TLS configuration, live-settable configuration changes, and restart-required configuration changes.

Confidence Score: 5/5

Safe to merge: no blocking failure remains.

No blocking failure remains. The focused controller tests confirmed that derived hashes preserve the legacy rendering contract, exclude live-settable keys, and change for configuration values that require a rollout.

T-Rex Logs

What T-Rex did

• I ran the TestNodeServerConfigRollHash suite and confirmed that the derived node hash matches the frozen legacy rendering for plain and TLS configurations, remains unchanged when only maxmemory changes, and changes when appendfsync changes.
• I attempted a focused rollout-contract verifier using a controller-runtime test, but the envtest Kubernetes API server did not become ready before its timeout.
• The test run for the internal/controller package reported PASS, and an envtest readiness blocker was observed with a kube-apiserver startup timeout.
• Artifacts were collected to support review, including the focused rollout verifier source, envtest startup failure logs, and the focused hash contract test output.

_{Ran code and verified through T-Rex}

_{Reviews (3): Last reviewed commit: "Merge branch 'main' of github.com:valkey..." | Re-trigger Greptile}

[bjosv]

LGTM! The added testcase with the old render logic was smart.