fix(cache): charge tiered store memory quota on every reconcile - #6167
Open
btxu-db wants to merge 6 commits into
Open
fix(cache): charge tiered store memory quota on every reconcile#6167btxu-db wants to merge 6 commits into
btxu-db wants to merge 6 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 #6167 +/- ##
==========================================
+ Coverage 65.19% 65.22% +0.02%
==========================================
Files 486 486
Lines 34150 34185 +35
==========================================
+ Hits 22263 22296 +33
- Misses 10136 10138 +2
Partials 1751 1751 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes worker memory reconciliation so tiered-store quotas remain included, building on stacked PR #6165.
Changes:
- Extracts non-mutating tiered-store memory accounting helpers.
- Applies quota-adjusted resources during synchronization.
- Adds creation-versus-reconciliation regression coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pkg/ddc/cache/engine/transform_tiered_store.go |
Centralizes quota calculation. |
pkg/ddc/cache/engine/sync.go |
Reconciles quota-adjusted resources. |
pkg/ddc/cache/engine/sync_test.go |
Adds synchronization regression tests. |
pkg/ddc/cache/component/component_manager.go |
Adds optional resource semantics from #6165. |
pkg/ddc/cache/component/advanced_statefulset_manager.go |
Skips unspecified resource updates. |
pkg/ddc/cache/component/sync_component_spec_test.go |
Updates tests for pointer resources. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+226
to
+227
| resources := withTieredStoreMemoryQuota(runtime.Spec.Worker.Resources, | ||
| tieredStoreMemoryQuota(&runtime.Spec.Worker.TieredStore)) |
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>
The previous revision recomputed the worker's tiered store memory quota from spec.worker.tieredStore on every sync. tieredStore is not a supported update field and SyncComponentSpec only patches resources, so editing the quota from 8Gi to 16Gi moved the container to baseline+16Gi while the tmpfs volume stayed at the 8Gi it was created with -- an unsupported edit half-applying, and a fresh disagreement between the container's memory and the volume it has to cover. Sum the size limits of the workload's memory-backed tiered store volumes instead. Those volumes are written by the same two handlers that charge container memory, so the sum is exactly what the creation path charged, and the container and its tmpfs cannot diverge. Editing tieredStore now continues to have no effect, as documented in cacheruntime_spec_update.md. Reading the quota from the volume rather than the container is also what lets a workload that an earlier release already stripped be repaired in place: the container's copy is gone, but the volume still carries it. Route the generated volume names through a shared prefix constant, so the recovery cannot select a tmpfs the CacheRuntimeClass template declares itself. Tests: two specs in sync_test.go covering the edited-quota and stripped-workload paths, and four in transform_tiered_store_test.go covering the recovery in isolation, including the volumes it must skip. Signed-off-by: btxu-db <btxu-db@outlook.com>
Motivation: Charging the tiered store quota builds the desired memory value by adding to the baseline. resource.Quantity caches the string it was parsed from, and Add clears that cache, while the value decoded from the workload still carries it. The two are numerically identical but not structurally identical, so the reflect.DeepEqual in updateResources read every reconcile as a change. The workload itself was never modified, because the resulting merge patch body is empty, so no generation bump and no pod restart. What it did produce was a PATCH request and a misleading "resources changed, will update" log line on every reconcile. Observed on a kind cluster against a CacheRuntime that sets resources and declares a processMemory tier: the line appeared every 90 seconds while the AdvancedStatefulSet stayed at generation 1 and the worker pod at 0 restarts. This only shows up once the quota is charged on every reconcile, so it arrived with this branch rather than being pre-existing. Approach: Use equality.Semantic.DeepEqual, which apimachinery registers a Quantity-aware comparison for. It treats the recomputed value as unchanged without loosening the comparison: a genuinely different quantity is still reported and still applied. Validation: - gofmt -l pkg/ddc/cache/ (no output) - go build ./... - go vet ./pkg/ddc/cache/... - go test -gcflags=all=-l -count=1 ./pkg/ddc/cache/... -> ok - Confirmed the new spec is a genuine regression test: restoring reflect.DeepEqual fails "should not report a change when an equal quantity was produced by arithmetic" while the other 43 specs pass. The companion spec, "should still report a change when the quantity really differs", passes under both implementations and guards against fixing this by making the comparison too permissive. Signed-off-by: btxu-db <btxu-db@outlook.com>
Resolving resources by priority means the baseline the tiered store quota is charged on top of can now come from the CacheRuntimeClass template, not only from the CacheRuntime. That combination had no coverage: every existing spec in this area sets spec.worker.resources first. The new spec leaves the CacheRuntime's resources unset, seeds the workload the way creation leaves it (template value plus quota, with the tmpfs volume carrying the quota as its size limit), and asserts the sync lands on the same value rather than dropping back to the bare template value. Confirmed it is a genuine regression test: restricting the quota to the case where the CacheRuntime sets resources explicitly, which is what the code did before this branch was rebased, fails this spec while the other 250 pass. Validation: - gofmt -l pkg/ddc/cache/ (no output) - go build ./... - go vet ./pkg/ddc/cache/... - go test -gcflags=all=-l -count=1 ./pkg/ddc/cache/... -> ok Signed-off-by: btxu-db <btxu-db@outlook.com>
btxu-db
force-pushed
the
fix/cacheruntime-tieredstore-memory-quota
branch
from
August 28, 2026 08:12
ac6c182 to
5d4b068
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ⅰ. Describe what this PR does
This is stacked on #6165 and its commit is included in this diff. The
syncRuntimeSpectest scaffolding this PR extends does not exist on
masteryet. Please review the second andthird commits; I'll rebase once #6165 merges.
A CacheRuntime whose worker declares both
spec.worker.resourcesand a memory-backedtieredStorelevel has the quota added to the container's memory request and limit when theworkload is created, and loses it again on the first reconcile afterwards. With a 4Gi
baseline and an 8Gi
processMemoryquota:After that the state is stable — the sync keeps proposing 4Gi, the workload already holds
4Gi, the comparison in
updateResourcessucceeds and nothing is ever reported again. Thecontainer is left with three numbers that disagree, each defensible on its own:
4Gi(rewritten by the sync)/dev/shm8Gi(tmpfs, sized from the quota, untouched)8Gi(what the runtime was told to use)Filling the cache then gets the worker OOMKilled, with nothing in any manifest to explain why.
Why it happens. The creation path derives the container's memory in two steps —
transformComponentPodTemplatewrites the user's baseline over the template, thenTransformRuntimeTieredStoreadds the quota on top:syncRuntimeSpecrebuilds the desired state from the raw spec, reproducing only the firststep:
and
updateResourcesreplaces the container's resources wholesale rather than merging them,so the second step is dropped.
Approach. Extract the arithmetic that charges a memory quota to a container into
withTieredStoreMemoryQuota.handleProcessMemoryandhandleEmptyDirheld twobyte-identical copies of it; both now call the helper, and so does
syncRuntimeSpec.The quota the sync passes in comes from the workload, not from the spec.
chargedTieredStoreMemoryQuotasums the size limits of the workload's memory-backed tieredstore volumes. Those are written by the same two handlers that charge container memory, so
the sum matches what creation charged. Their shared volume name prefix is now a constant.
This matters because
tieredStoreis not a supported update field andSyncComponentSpeconly patches
resources. If the sync recomputed the quota from the spec, editing it from8Gi to 16Gi would push the container to 20Gi while /dev/shm stayed at 8Gi. Reading it from
the volume also means a workload already stripped by an earlier release can be repaired:
the quota is still on the volume even after the container's copy is gone.
withTieredStoreMemoryQuotareturns a new value rather than mutating in place. The previousinline code wrote through the
ResourceListmaps thattransform_common.goshares withruntime.Spec.Worker.Resources, so the transform silently modified the runtime object it washanded.
Master is unaffected —
CacheRuntimeMasterSpechas noTieredStorefield. Client isunaffected — it runs as a DaemonSet and is deliberately not synced. The nil guard from #6165
is preserved, so a CacheRuntime that specifies no resources still leaves the template's
values untouched.
Ⅱ. Does this pull request fix one issue?
fixes #6166
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Three specs in
sync_test.go, under "when the CacheRuntime declares a memory tiered store":path and then the sync path against a fake client and asserts the two agree. It compares
against the value the creation path derives, not a hard-coded
12Gi, so it keepstesting the invariant if the quota rules ever change; and it calls
engine.getRuntime()separately for each phase, the way two consecutive reconciles would, because reusing one
in-memory runtime object lets the transform's side effect on the shared
ResourceListmapsleak into the sync and the test then passes against the unfixed code. A
Cmpguard assertsthe creation path actually charges the quota, so the comparison cannot pass vacuously.
16Giaftercreation and expects the container to stay at
12Giwith /dev/shm at8Gi. Reverting thesync to a spec-derived quota fails this spec.
4Gibaselineonto the container, the way the old sync did, and expects the next sync to bring it back to
12Gi. This is what pins the recovery to the volume rather than the container.Four specs in
transform_tiered_store_test.gocoverchargedTieredStoreMemoryQuotainisolation: an empty pod spec, summing across levels, a round-trip against
TransformRuntimeTieredStore, and the volumes it must skip — hostPath levels, disk-backedemptyDir, a memory volume with no size limit, and a tmpfs the CacheRuntimeClass template
declares itself. Dropping the volume name prefix check fails the last one.
The pre-existing specs in that file — quota charged, container without memory constraints left
alone, Memory-backed emptyDir, multiple levels accumulating — pass unchanged, which is the
evidence that the extraction is behaviour-preserving.
Ⅳ. Describe how to verify it
Unit:
Copying only
sync_test.gointo a worktree at the base commit — test present, fix absent —fails with
Expected "4Gi" to equal "12Gi", matching the reported symptom.End to end on kind (Kubernetes v1.30.0), using the manifests from #6166:
gen=1 mem=12Gi→gen=2 mem=4Gigen=2 mem=4Gi→gen=3 mem=12Gi, repaired in placegen=1 mem=12GiThe three numbers then agree — cgroup limit 12Gi,
/dev/shm8Gi, cache tier 8Gi.Ⅴ. Special notes for reviews
Distinct from #6161 despite the similar symptom. #6161 is "the CacheRuntime specifies no
resources and the template's values get cleared"; this is "the CacheRuntime specifies a
baseline and the tiered store quota on top of it gets dropped". The reproduction above was run
with #6165 already applied, which is why it is filed separately.
handleEmptyDirhad the same hole formedium: Memorylevels, so the extracted helper closesboth media in one place.
The Copilot review flagged that the second commit derived the sync's quota from
spec.worker.tieredStore, which let an edit to that unsupported field half-apply — thecontainer moved to
baseline + new quotawhile the tmpfs volume kept its original size.Confirmed, and fixed in the third commit: the quota now comes from the workload's volumes, so
editing
tieredStorestill has no effect, as documented incacheruntime_spec_update.md.