fix(cache): preserve CacheRuntimeClass template resources when unset - #6165
fix(cache): preserve CacheRuntimeClass template resources when unset#6165btxu-db wants to merge 2 commits into
Conversation
Motivation:
When a CacheRuntimeClass template declares container resources and the
CacheRuntime does not set spec.master.resources / spec.worker.resources,
the template values were silently reset to {} on the first reconcile after
creation. The AdvancedStatefulSet's generation bumped from 1 to 2 and the
pods rolled once, with no error or event. A component the user capped at
2Gi could then consume the whole node.
syncRuntimeSpec already guarded against the zero value, but only when
choosing what to assign to a local variable; the zero value was passed on
to SyncComponentSpec regardless. updateResources treats an empty
ResourceRequirements as a valid desired state meaning "clear the
resources" -- a deliberate contract covered by its own unit test -- so it
faithfully wrote the empty value through. The information that the user
had not specified anything was lost at the package boundary, because
ComponentSpec.Resources is a value type and therefore cannot distinguish
"unset" from "explicitly empty".
Approach:
Make ComponentSpec.Resources a *corev1.ResourceRequirements so that nil
means "leave the workload's current resources untouched", mirroring the
existing ComponentSpec.Replicas field, which is already a pointer
documented as "nil means no change". syncRuntimeSpec now yields nil when
the user specified neither requests nor limits, and SyncComponentSpec
skips updateResources on nil, exactly as it already does for Replicas.
updateResources itself is unchanged: a non-nil value is still applied
verbatim, so explicitly clearing resources keeps working and its existing
tests keep passing.
Validation:
- gofmt -l pkg/ddc/cache/ (no output)
- go build ./...
- go vet ./pkg/ddc/cache/...
- go test -gcflags=all=-l ./pkg/ddc/cache/... -> ok
- go test ./pkg/ddc/cache/... -> 228 passed, up from 225 on the base
commit. Without the flag the suite also reports 12 failures in
ufs_test.go and one gomonkey spec in sync_test.go; those need inlining
disabled for the patches to take effect, fail identically on the base
commit, and are unrelated to this change.
- Confirmed the new specs are genuine regression tests: checking out only
sync_test.go from this branch into a worktree at the base commit --
tests present, fix absent -- fails all three with
Expected "0" to equal "2Gi". Reverting the master guard and the worker
guard individually each fails a spec too, so neither half is uncovered.
Signed-off-by: btxu-db <btxu-db@outlook.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @btxu-db. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6165 +/- ##
=======================================
Coverage 65.19% 65.20%
=======================================
Files 486 486
Lines 34150 34151 +1
=======================================
+ Hits 22263 22267 +4
+ Misses 10136 10134 -2
+ Partials 1751 1750 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes CacheRuntime reconciliation so unset resource overrides preserve CacheRuntimeClass template resources.
Changes:
- Makes component resources optional during synchronization.
- Skips resource updates when CacheRuntime resources are unset.
- Adds master and worker regression tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pkg/ddc/cache/engine/sync.go |
Passes resource overrides only when configured. |
pkg/ddc/cache/engine/sync_test.go |
Tests template preservation and component-specific overrides. |
pkg/ddc/cache/component/component_manager.go |
Makes synchronized resources pointer-valued. |
pkg/ddc/cache/component/advanced_statefulset_manager.go |
Skips nil resource updates. |
pkg/ddc/cache/component/sync_component_spec_test.go |
Adapts tests to pointer-valued resources. |
Suppressed comments (1)
pkg/ddc/cache/engine/sync.go:224
- The same regression applies to worker resources: after a user removes previously configured worker requests/limits, the persisted value has nil maps and this passes
nil, so the workload retains stale limits rather than clearing them. Please preserve a way to distinguish “no override was ever specified” from “remove the existing override”; the current value-typed CacheRuntime API cannot represent that distinction directly.
var workerResources *corev1.ResourceRequirements
if runtime.Spec.Worker.Resources.Requests != nil || runtime.Spec.Worker.Resources.Limits != nil {
workerResources = &runtime.Spec.Worker.Resources
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var resources *corev1.ResourceRequirements | ||
| if runtime.Spec.Master.Resources.Requests != nil || runtime.Spec.Master.Resources.Limits != nil { | ||
| resources = runtime.Spec.Master.Resources | ||
| resources = &runtime.Spec.Master.Resources |
There was a problem hiding this comment.
Thanks — the behaviour change is real. I've added the measurements to section 4.
"Never set" and "removed after being set" are byte-identical in the CacheRuntime spec, so syncRuntimeSpec can't tell them apart by construction. Before this PR both meant "clear to {}", which wiped the template's values — that's #6161. After it, both mean "leave it alone". Step 3 in the table is your case, and I'd rather leave a stale limit than silently drop a cap the admin declared.
The tri-state actually already exists after this change, no API change needed: omitting the field leaves the workload alone, resources: {requests: {}} clears it (I checked — empty maps survive CRD pruning and deserialize non-nil, so they pass the guard), and a set value gets applied. What's missing is falling back to the template on removal. That needs the desired pod template computed through the transform chain and diffed against the live one — the follow-up described in the PR body, which also covers the processMemory problem.
updateResources and its unit test are untouched here. Happy to add a test pinning the explicit-clear path if you want it as a contract.
| if newSpec.Resources != nil { | ||
| if s.updateResources(astsToUpdate, *newSpec.Resources, logger) { | ||
| needsUpdate = true | ||
| } |
There was a problem hiding this comment.
When calculating resource requirements, should we consider the configuration of runtimeclass?
We need to clarify in the document how the resource defined in runtimeclass and runtime takes effect, and how changes affect the final resource calculation.
My thought is: we will first take the not nil resource value defined in the runtime (high priority) or runtime class. If both nil, then the value is nil.
So,
- If runtime class sets the default resource, user can not remove resource. we will take the not nil resource value defined in the runtime class or runtime.
- If runtime class does not set the default resource, user can set the resource and then remove resource.
@cheyang What Do You Think?
Motivation: Review feedback on fluid-cloudnative#6165 asked for the resources to be resolved by priority rather than by skipping the sync: take the value from the CacheRuntime when it sets one, otherwise take the value from the CacheRuntimeClass template, and only leave the workload alone when neither declares anything. The previous commit passed nil whenever the CacheRuntime set no resources, which kept the template values in place but only because nothing was written. A workload whose resources no longer matched the template stayed that way, since the sync had no desired value to compare against. Approach: desiredComponentResources resolves the value for a component and both callers use it. Only Containers[0] is read from the template, which is what the creation path fills in. The returned value is a deep copy so updateResources cannot write through into the CacheRuntime spec or the CacheRuntimeClass template, both of which are shared objects. Passing nil still means "leave the workload's resources untouched", matching ComponentSpec.Replicas. One consequence is worth stating: once a CacheRuntimeClass template declares resources, removing resources from the CacheRuntime no longer leaves the component unconstrained, it falls back to the template value. corev1. ResourceRequirements is a value type, so an omitted field and an explicitly empty one are indistinguishable after decoding, and the fallback has to pick one meaning. Both sample docs now describe the resolution order and this limitation. Validation: - gofmt -l pkg/ddc/cache/ (no output) - go build ./... - go vet ./pkg/ddc/cache/... - go test -gcflags=all=-l ./pkg/ddc/cache/... -> ok - Confirmed the new specs are genuine regression tests: making desiredComponentResources return nil instead of the template value fails "should restore the template value when the CacheRuntime specifies none", while the five other specs in the Describe still pass. That last part also shows the specs already on this branch could not tell the two behaviours apart, since the seeded workload already carries the template value. Signed-off-by: btxu-db <btxu-db@outlook.com>
Motivation: A CacheRuntime whose worker declares both spec.worker.resources and a memory-backed tieredStore level has the tiered store quota added to the container's memory request and limit when the workload is created, and loses it again on the first reconcile afterwards. With a 4Gi baseline and an 8Gi processMemory quota the worker's AdvancedStatefulSet is created with a 12Gi limit and is rewritten to 4Gi a few seconds later, with no error and no event. The state is then stable: the sync keeps proposing 4Gi, the workload already holds 4Gi, the comparison in updateResources succeeds and nothing is ever reported again. The container is left with three numbers that disagree, each defensible on its own: the cgroup memory limit is 4Gi, /dev/shm is an 8Gi tmpfs sized from the quota and never touched by the sync, and the cache tier is configured to use 8Gi. Filling the cache gets the worker OOMKilled with nothing in any manifest to explain why. The creation path derives the container's memory in two steps: transformComponentPodTemplate writes the user's baseline over the template, then TransformRuntimeTieredStore adds the quota on top. syncRuntimeSpec rebuilds the desired state from runtime.Spec.Worker.Resources alone, reproducing only the first step, and updateResources replaces the container's resources wholesale rather than merging them, so the second step is dropped. This is distinct from fluid-cloudnative#6161. There the CacheRuntime specified no resources at all and the sync overwrote the template's values with the zero value; fluid-cloudnative#6165 fixes that by passing nil. Here the user does specify a baseline, so that guard is satisfied and the sync proceeds with an under-computed value. Approach: Extract the arithmetic that charges a memory quota to a container into withTieredStoreMemoryQuota, and the summing of memory-backed levels into tieredStoreMemoryQuota. handleProcessMemory and handleEmptyDir already held two byte-identical copies of that arithmetic; both now call the helper, and syncRuntimeSpec calls it too. The derivation has a single implementation, so the creation path and the sync path cannot compute different values again. withTieredStoreMemoryQuota returns a new value rather than mutating in place. The previous inline code wrote through the ResourceList maps that transform_common.go shares with runtime.Spec.Worker.Resources, so the transform silently modified the runtime object it was handed; a caller that reused that object within one reconcile would have accumulated the quota more than once. Master is unaffected: CacheRuntimeMasterSpec has no TieredStore field. Client is unaffected: it runs as a DaemonSet and is deliberately not synced. The nil guard from fluid-cloudnative#6165 is preserved, so a CacheRuntime that specifies no resources still leaves the template's values untouched. Validation: - gofmt -l pkg/ddc/cache/ (no output) - go vet ./pkg/ddc/cache/... - FLUID_UNIT_TEST=true go test -gcflags="all=-N -l" ./pkg/ddc/cache/... -> ok, 241 specs - The new spec compares the sync's output against the value the creation path derives, instead of asserting a hard-coded quantity, and guards that comparison against being vacuous. Copying only sync_test.go into a worktree at the base commit -- test present, fix absent -- fails with Expected "4Gi" to equal "12Gi", matching the reported symptom. - kind v1.30.0, Kubernetes v1.30.0: with the base controller the worker workload goes gen=1 mem=12Gi -> gen=2 mem=4Gi. With this change an already-broken workload is repaired in place (gen=2 mem=4Gi -> gen=3 mem=12Gi) and a freshly created one stays at gen=1 mem=12Gi. The three numbers then agree: cgroup limit 12Gi, /dev/shm 8Gi, cache tier 8Gi. Signed-off-by: btxu-db <btxu-db@outlook.com>
|
Mooncake has no POSIX mount semantics, so its CacheRuntimeClass declares only master and worker. No existing e2e case covers that shape: curvine ships a client component, so the client-less path through the controller is untested. The case pins down the behaviour that path is expected to have: - the controller does not panic and the Dataset reaches Bound with the client component omitted (a regression guard for the nil pointer dereference fixed in fluid-cloudnative#6157); - no client DaemonSet or client pods are created, and status.client.phase stays empty; - the ReportSummary script populates status.cacheStates, and cached reflects data written through the cache system's own client; - the Dataset PVC reaches Bound but cannot be mounted by application pods, which is what the docs' FAQ describes. The image is built in-repo from test/gha-e2e/mooncake/image rather than pulled from an external registry, for the same reason as the jindo oss-emulator: e2e runs on every PR, an external image going away turns the whole pipeline red, and the two scripts Fluid invokes inside the image have to be reviewable. The base image is pinned by digest and every resolved pip version is pinned explicitly, installed from wheels only so no package build script runs at image build time. The CacheRuntimeClass template deliberately declares no container resources. A template that sets them while the CacheRuntime does not currently has them overwritten with an empty value on the first reconcile (fluid-cloudnative#6161), and if the resulting rollout flips the Dataset to Failed it does not recover on its own (fluid-cloudnative#6160). Neither is what this case is meant to cover, so it stays clear of both until fluid-cloudnative#6165 lands. Signed-off-by: btxu-db <btxu-db@outlook.com>
Ⅰ. Describe what this PR does
When a CacheRuntimeClass template declares container resources and the CacheRuntime does not
set
spec.master.resources/spec.worker.resources, the template's values were silentlyreset to
{}on the first reconcile after creation. The AdvancedStatefulSet'sgenerationwent from 1 to 2 and the pods rolled once, with no error and no event — a component the user
capped at 2Gi could then consume the whole node.
Why it happened.
syncRuntimeSpecdid guard against the zero value, but only whendeciding what to assign to a local variable; the zero value was passed to
SyncComponentSpecanyway:updateResourcestreats an emptyResourceRequirementsas a valid desired state meaning"clear the resources". That is deliberate and covered by its own unit test
(
sync_component_spec_test.go, "should update to nil resources (remove limits)"), so itfaithfully wrote the empty value through. The information that the user had specified
nothing was lost at the package boundary, because
ComponentSpec.Resourcesis a value typeand cannot distinguish "unset" from "explicitly empty".
Approach. Make
ComponentSpec.Resourcesa*corev1.ResourceRequirements, sonilmeans "leave the workload's current resources untouched". This mirrors
ComponentSpec.Replicas, which is already a pointer documented as(optional, nil means no change)and already nil-checked bySyncComponentSpec:updateResourcesitself is unchanged — a non-nil value is still applied verbatim, soexplicitly clearing resources keeps working and its existing test keeps passing.
ComponentSpecis internal topkg/ddc/cache/component; no CRD or API type changes.Ⅱ. Does this pull request fix one issue?
fixes #6161
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
syncRuntimeSpechad no direct test coverage, which is how this shipped. Added aDescribe("syncRuntimeSpec")block inpkg/ddc/cache/engine/sync_test.gowith three specs,written at the behaviour level (what the AdvancedStatefulSet looks like after a sync) rather
than against
updateResources, so they survive any later refactor of the sync path:The last two matter as much as the first. Without them, simply deleting
updateResourceswould also make the suite pass; their cross-assertions additionally pin down that the two
components do not bleed into each other.
sync_component_spec_test.gois touched only to pass&corev1.ResourceRequirements{...}where
ComponentSpecliterals are built; no assertion in that file changed.Ⅳ. Describe how to verify it
Without
-gcflags=all=-lthe suite also reports 12 failures inufs_test.goand onegomonkey spec in
sync_test.go; those need inlining disabled for the patches to takeeffect, fail identically on the base commit, and are unrelated to this change.
The new specs were confirmed to be genuine regression tests: checking out only
pkg/ddc/cache/engine/sync_test.gofrom this branch into a worktree at the base commit —tests present, fix absent — fails all three with
Expected "0" to equal "2Gi". Revertingthe master guard and the worker guard individually each fails a spec too, so neither half of
the change is left uncovered.
On a cluster. kind v0.23.0 / Kubernetes v1.30.0, the manifests from #6161, only the
controller image differs between the two runs. Polling
metadata.generationandspec.template.spec.containers[0].resourceson the worker AdvancedStatefulSet, with2Gideclared in the CacheRuntimeClass template and nothing in the CacheRuntime:
before
after
Field semantics. A second run on the same cluster, this time against the master
component — whose code path is byte-identical in both builds — with
limits.memory: 2Gideclared in the CacheRuntimeClass template, walking
spec.master.resourcesthrough itsthree states:
spec.master.resources{"limits":{"memory":"2Gi"}}— template preserved{"limits":{"memory":"1Gi"}}{"limits":{"memory":"1Gi"}}— applied{"limits":{"memory":"1Gi"}}— left untouched{"requests":{}}{}— clearedStep 3 is the trade-off this PR makes: once an override has been set and then removed, the
workload keeps the last value rather than falling back to the template, because an override
that was never set and one that was removed are byte-identical in the CacheRuntime spec.
Step 4 shows an override can still be cleared explicitly —
requests: {}andlimits: {}survive CRD pruning (confirmed with
kubectl apply --dry-run=server, which returns themverbatim) and deserialize into non-nil empty maps, so they pass the guard and are applied.
Before this PR steps 1 and 3 were indistinguishable and both cleared the workload; the
distinction between "leave it alone" and "clear it" is what this change introduces.
Sampling the master pod UID after each rollout converged (
observedGenerationcaught up andupdatedReplicasready) shows the UID changes across a resources change, so the reset thisPR removes was costing a real pod recreation on every freshly created CacheRuntime, not just
a metadata bump.
Ⅴ. Special notes for reviews
The Dataset reaching
Failedin the "before" run is #6160: the spurious rollout is atransient runtime outage, and
Failedis a one-way trap. This branch does not contain thatfix, yet the Dataset stays
Boundafter this change — because the spurious rollout nolonger happens. The two issues are still independent: #6160 also triggers on legitimate
rollouts (an image or replica change), so it needs its own fix.
This is the narrow fix for the reported symptom. It does not address a second, distinct way
the same code path loses resources: when a worker uses a
processMemorytiered-store level,handleProcessMemoryadds the level's quota on top of the container's memory limit, so thestored value is
baseline + quotawhilesyncRuntimeSpeconly knows the baseline andoverwrites the sum away. That reproduces with this PR applied — a CacheRuntime with
worker.resources.limits.memory: 4GiandprocessMemory.quota: 8Gishowsgen=1 mem=12Giat 1s andgen=2 mem=4Giat 6s — and cannot be fixed by nil-handling,since the value the sync path would need does not exist in any single field. Filing that
separately; it likely wants
syncRuntimeSpecto compute the desired pod template throughthe existing transform chain and diff that, rather than assembling raw spec fields.