fix(cache): charge tiered store memory quota on every reconcile - #6167
Open
btxu-db wants to merge 2 commits into
Open
fix(cache): charge tiered store memory quota on every reconcile#6167btxu-db wants to merge 2 commits into
btxu-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>
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>
|
[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 34161 +11
==========================================
+ Hits 22263 22280 +17
+ Misses 10136 10132 -4
+ Partials 1751 1749 -2 ☔ 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)) |
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 secondcommit only; 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, and the summing of memory-backed levels intotieredStoreMemoryQuota.handleProcessMemoryandhandleEmptyDiralready held twobyte-identical copies of that arithmetic; both now call the helper, and
syncRuntimeSpeccalls it too. The derivation has a single implementation, so the creation path and the sync
path cannot compute different values again.
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.
One spec in
sync_test.go: "should keep the tiered store quota when syncing after creation".It runs the creation path and then the sync path against a fake client and asserts the two
agree. Two details make it a regression guard rather than a snapshot:
12Gi, so itkeeps testing the invariant if the quota rules ever change.
engine.getRuntime()separately for each phase, the way two consecutive reconcileswould. Reusing one in-memory runtime object lets the transform's side effect on the shared
ResourceListmaps leak into the sync, and the test then passes against the unfixed code.A
Cmpguard asserts the creation path actually charges the quota, so the main comparisoncannot pass vacuously.
The existing specs in
transform_tiered_store_test.gocover the extracted helper's behaviour— quota charged, container without memory constraints left alone, Memory-backed emptyDir,
multiple levels accumulating — and 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.