chore(chart): rename values key runtime.cache to runtime.cacheruntime - #6137
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Pull request overview
This PR renames the Helm chart values key for configuring the CacheRuntime controller from runtime.cache to runtime.cacheruntime, aligning the values structure with the controller/CRD naming and reducing ambiguity.
Changes:
- Renamed the
runtime.cachevalues subtree toruntime.cacheruntimeinvalues.yaml. - Updated the CacheRuntime controller Deployment template to read from
runtime.cacheruntimeand added a render-time guard for staleruntime.cacheusage. - Updated the Kind deployment helper script to use
--set runtime.cacheruntime.enabled=true.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| charts/fluid/fluid/values.yaml | Renames the values subtree and adds clarifying comments for the CacheRuntime controller settings. |
| charts/fluid/fluid/templates/controller/cacheruntime_controller.yaml | Switches template lookups to runtime.cacheruntime and adds a guard to fail on deprecated runtime.cache. |
| .github/scripts/deploy-fluid-to-kind.sh | Updates Helm --set usage to the new values key. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.runtime.cache }} | ||
| {{- fail "values key `runtime.cache` has been renamed to `runtime.cacheruntime`, please update your values file or --set flags" }} | ||
| {{- end }} |
There was a problem hiding this comment.
Good catch — fixed in ba76963. The guard now tests for the presence of the key instead of a truthy value:
{{- if hasKey .Values.runtime "cache" }}
{{- fail "values key `runtime.cache` has been renamed to `runtime.cacheruntime`, please update your values file or --set flags" }}
{{- end }}
Verified against a live cluster, all of these now fail loudly (only the first one did before):
| input | result |
|---|---|
--set runtime.cache.enabled=true |
fails with the rename message |
--set runtime.cache=false |
fails with the rename message |
runtime.cache: {} in a values file |
fails with the rename message |
runtime.cache: null in a values file / --set runtime.cache=null |
fails with the rename message |
--set runtime.cacheruntime.enabled=true |
renders replicas: 1 |
| no override | renders replicas: 0 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6137 +/- ##
=======================================
Coverage 65.08% 65.08%
=======================================
Files 485 485
Lines 33989 33989
=======================================
Hits 22122 22122
Misses 10126 10126
Partials 1741 1741 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The sub-keys under `runtime:` are named after cache engines (alluxio,
jindo, juicefs, thin, efc, vineyard), so a sibling named `cache` reads
like yet another engine, while CacheRuntime is actually a mechanism that
integrates cache systems declaratively via CacheRuntimeClass. Worse,
`runtime.cache.kubeClientQPS` / `workQueueQPS` look like informer cache
tuning knobs rather than settings of the CacheRuntime controller.
Rename the key to `runtime.cacheruntime`, matching the CRD name and the
`cacheruntime-controller` Deployment. The old key never shipped in a
release tag, so no compatibility shim is needed; instead fail the render
loudly whenever the deprecated key is still present, because a leftover
`runtime.cache` would otherwise silently render `replicas: 0` and scale
the controller down without any error. The guard tests for the presence
of the key rather than for a truthy value, so stale but falsy leftovers
such as `runtime.cache: {}` are reported as well.
Signed-off-by: cheyang <cheyang@163.com>
2a05795 to
ba76963
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
charts/fluid/fluid/templates/controller/cacheruntime_controller.yaml:3
hasKeyrequires its first argument to be a map/dict. If a user supplies an invalid override likeruntime: null(or a non-map), this will error out with a type failure before reaching the intendedfailmessage. Consider guarding with a type check (e.g., only callhasKeywhen.Values.runtimeis a map) so the chart fails deterministically with your clearer message.
{{- if hasKey .Values.runtime "cache" }}
{{- fail "values key `runtime.cache` has been renamed to `runtime.cacheruntime`, please update your values file or --set flags" }}
{{- end }}
charts/fluid/fluid/templates/controller/cacheruntime_controller.yaml:27
- This annotation is currently driven only by
replicas > 1, independent ofruntime.cacheruntime.enabled. If a user setsreplicas: 3whileenabled: false, the Deployment still rendersreplicas: 0but retains an annotation suggesting multiple replicas, which is inconsistent and can confuse debugging/operations. Consider gating this condition onenabledas well (or otherwise deriving the annotation from the effective replica count).
{{ if gt (.Values.runtime.cacheruntime.replicas | int) 1 -}}
controller.runtime.fluid.io/replicas: {{ .Values.runtime.cacheruntime.replicas | quote }}



What this PR does
Renames the Helm values key for the CacheRuntime controller:
runtime.cache→runtime.cacheruntime.Why
The sub-keys under
runtime:are named after cache engines —alluxio,jindo,juicefs,thin,efc,vineyard. A sibling namedcachetherefore reads like yet another engine, whileCacheRuntimeis actually a mechanism: it integrates arbitrary cache systems declaratively throughCacheRuntimeClass(topology +executionEntries), rather than being one more built-in engine.There is also a concrete readability trap:
cachein the Kubernetes ecosystem usually means the client-go informer cache, soreads like controller cache tuning rather than settings of the CacheRuntime controller.
runtime.cacheruntimestutters slightly underruntime:, but it lines up exactly with the CRD (cacheruntimes.data.fluid.io), the Deployment (cacheruntime-controller) and the binary, so there is no ambiguity left.Compatibility
The old key never shipped in a release tag (latest tag is
v1.0.8; CacheRuntime exists only on master), so no compatibility shim is needed. Instead the template now fails the render when a staleruntime.cacheis present:Without that guard the old key would silently render
replicas: 0and scale the controller down with no error at all — which is exactly what happens to anyone on master who keepsruntime.cache.enabled: truein their values.Verification
Tested against a live 3-node Kubernetes v1.36 cluster running Fluid from master:
defaultVersionpinned to the images the cluster was already running, the manifest produced byhelm upgrade --dry-runis byte-identical tohelm get manifest fluid(2837 lines, same md5) — the rename touches only the values layer, no rendered object changes.helm upgradesucceeded,helm get valuesnow showsruntime.cacheruntime.enabled: true,cacheruntime-controllerstayed1/1and its pod was not even restarted.--set runtime.cache.enabled=truefails loudly (message above); the new key rendersreplicas: 1; the default rendersreplicas: 0..github/scripts/deploy-fluid-to-kind.shis updated to the new key as well; no other references to the old key remain in the repo (docs, tests, CI, Makefile all checked).