fix(cache): preserve CacheRuntimeClass template resources when unset

[btxu-db]

Ⅰ. 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 silently
reset to {} on the first reconcile after creation. The AdvancedStatefulSet's generation
went 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. syncRuntimeSpec did guard against the zero value, but only when
deciding what to assign to a local variable; the zero value was passed to
SyncComponentSpec anyway:

var workerResources corev1.ResourceRequirements
if runtime.Spec.Worker.Resources.Requests != nil || runtime.Spec.Worker.Resources.Limits != nil {
    workerResources = runtime.Spec.Worker.Resources
}

updateResources treats an empty ResourceRequirements as 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 it
faithfully wrote the empty value through. The information that the user had specified
nothing was lost at the package boundary, because ComponentSpec.Resources is a value type
and cannot distinguish "unset" from "explicitly empty".

Approach. Make ComponentSpec.Resources a *corev1.ResourceRequirements, so nil
means "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 by SyncComponentSpec:

// 1. Update replicas if specified and changed
if newSpec.Replicas != nil {
    if s.updateReplicas(astsToUpdate, *newSpec.Replicas, logger) {

// 3. Update resources if specified
if newSpec.Resources != nil {
    if s.updateResources(astsToUpdate, *newSpec.Resources, logger) {

updateResources itself is unchanged — a non-nil value is still applied verbatim, so
explicitly clearing resources keeps working and its existing test keeps passing.
ComponentSpec is internal to pkg/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.

syncRuntimeSpec had no direct test coverage, which is how this shipped. Added a
Describe("syncRuntimeSpec") block in pkg/ddc/cache/engine/sync_test.go with 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:

• CacheRuntime specifies no resources -> master and worker both keep the template's values
• CacheRuntime specifies master resources -> master is updated, worker is left alone
• CacheRuntime specifies worker resources -> worker is updated, master is left alone

The last two matter as much as the first. Without them, simply deleting updateResources
would 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.go is touched only to pass &corev1.ResourceRequirements{...}
where ComponentSpec literals are built; no assertion in that file changed.

Ⅳ. Describe how to verify it

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 -gcflags=all=-l 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.

The new specs were confirmed to be genuine regression tests: checking out only
pkg/ddc/cache/engine/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 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.generation and
spec.template.spec.containers[0].resources on the worker AdvancedStatefulSet, with 2Gi
declared in the CacheRuntimeClass template and nothing in the CacheRuntime:

before

  1s  gen=1 res={"limits":{"memory":"2Gi"}}  dataset=NotBound
 33s  gen=2 res={}                           dataset=Bound
 61s  gen=2 res={}                           dataset=Failed

after

  1s  gen=1 res={"limits":{"memory":"2Gi"}}  dataset=NotBound
  6s  gen=1 res={"limits":{"memory":"2Gi"}}  dataset=Bound
        (unchanged through 90s)

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: 2Gi
declared in the CacheRuntimeClass template, walking spec.master.resources through its
three states:

step	spec.master.resources	resulting AdvancedStatefulSet	generation
1	omitted	{"limits":{"memory":"2Gi"}} — template preserved	1
2	{"limits":{"memory":"1Gi"}}	{"limits":{"memory":"1Gi"}} — applied	2
3	omitted again	{"limits":{"memory":"1Gi"}} — left untouched	2
4	{"requests":{}}	{} — cleared	3

Step 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: {} and limits: {}
survive CRD pruning (confirmed with kubectl apply --dry-run=server, which returns them
verbatim) 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 (observedGeneration caught up and
updatedReplicas ready) shows the UID changes across a resources change, so the reset this
PR removes was costing a real pod recreation on every freshly created CacheRuntime, not just
a metadata bump.

Ⅴ. Special notes for reviews

The Dataset reaching Failed in the "before" run is #6160: the spurious rollout is a
transient runtime outage, and Failed is a one-way trap. This branch does not contain that
fix, yet the Dataset stays Bound after this change — because the spurious rollout no
longer 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 processMemory tiered-store level,
handleProcessMemory adds the level's quota on top of the container's memory limit, so the
stored value is baseline + quota while syncRuntimeSpec only knows the baseline and
overwrites the sum away. That reproduces with this PR applied — a CacheRuntime with
worker.resources.limits.memory: 4Gi and processMemory.quota: 8Gi shows
gen=1 mem=12Gi at 1s and gen=2 mem=4Gi at 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 syncRuntimeSpec to compute the desired pod template through
the existing transform chain and diff that, rather than assembling raw spec fields.

[fluid-e2e-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ronggu for approval by writing /assign @ronggu in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fluid-e2e-bot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.21%. Comparing base (0e24a95) to head (2a4968a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6165      +/-   ##
==========================================
+ Coverage   65.19%   65.21%   +0.01%     
==========================================
  Files         486      486              
  Lines       34150    34151       +1     
==========================================
+ Hits        22263    22270       +7     
+ Misses      10136    10132       -4     
+ Partials     1751     1749       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.4% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

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.

[btxu-db]

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.

[xliuqq]

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,

1. 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.
2. If runtime class does not set the default resource, user can set the resource and then remove resource.

@cheyang What Do You Think?