Skip to content

fix(cache): preserve CacheRuntimeClass template resources when unset #6165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
btxu-db wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(cache): preserve CacheRuntimeClass template resources when unset #6165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions pkg/ddc/cache/component/advanced_statefulset_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,10 @@ func (s *AdvancedStatefulSetManager) SyncComponentSpec(ctx context.Context, iden
}

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

@xliuqq xliuqq Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

// Skip patching if no changes detected
Expand Down
2 changes: 1 addition & 1 deletion pkg/ddc/cache/component/component_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type ComponentSpec struct {
// Version contains image and pull policy information
Version datav1alpha1.VersionSpec
// Resources contains CPU and memory resource requirements
Resources corev1.ResourceRequirements
Resources *corev1.ResourceRequirements
}

func NewComponentHelper(componentType common.ComponentType, client client.Client) ComponentManager {
Expand Down
6 changes: 3 additions & 3 deletions pkg/ddc/cache/component/sync_component_spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ var _ = Describe("AdvancedStatefulSetManager SyncComponentSpec", func() {
Context("when updating resources", func() {
It("should update both requests and limits", func() {
spec := ComponentSpec{
Resources: corev1.ResourceRequirements{
Resources: &corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("4"),
corev1.ResourceMemory: resource.MustParse("8Gi"),
Expand Down Expand Up @@ -265,7 +265,7 @@ var _ = Describe("AdvancedStatefulSetManager SyncComponentSpec", func() {

It("should not update when resources unchanged", func() {
spec := ComponentSpec{
Resources: corev1.ResourceRequirements{
Resources: &corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("2"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
Expand Down Expand Up @@ -302,7 +302,7 @@ var _ = Describe("AdvancedStatefulSetManager SyncComponentSpec", func() {
Image: "fluid-cache",
ImageTag: "v1.1.0",
},
Resources: corev1.ResourceRequirements{
Resources: &corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("4"),
},
Expand Down
8 changes: 4 additions & 4 deletions pkg/ddc/cache/engine/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,9 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt
manager := component.NewComponentHelper(common.ComponentTypeMaster, e.Client)
// Only sync resources if they are explicitly set (not zero-value)
// This prevents overwriting template defaults when user hasn't specified resources
var resources corev1.ResourceRequirements
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
Comment on lines +198 to +200

@btxu-db btxu-db Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
masterSpec := component.ComponentSpec{
Version: runtime.Spec.Master.RuntimeVersion,
Expand All @@ -219,9 +219,9 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt
manager := component.NewComponentHelper(common.ComponentTypeWorker, e.Client)
// Only sync resources if they are explicitly set (not zero-value)
// This prevents overwriting template defaults when user hasn't specified resources
var workerResources corev1.ResourceRequirements
var workerResources *corev1.ResourceRequirements
if runtime.Spec.Worker.Resources.Requests != nil || runtime.Spec.Worker.Resources.Limits != nil {
workerResources = runtime.Spec.Worker.Resources
workerResources = &runtime.Spec.Worker.Resources
}
workerSpec := component.ComponentSpec{
Version: runtime.Spec.Worker.RuntimeVersion,
Expand Down
74 changes: 74 additions & 0 deletions pkg/ddc/cache/engine/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
cruntime "github.com/fluid-cloudnative/fluid/pkg/runtime"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -858,4 +859,77 @@ var _ = Describe("CacheEngine Sync Tests", Label("pkg.ddc.cache.engine.sync_test
})
})
})

Describe("syncRuntimeSpec", func() {
const masterSts, workerSts = "test-runtime-master", "test-runtime-worker"

// templateResources mirrors the value the creation path derives from the
// CacheRuntimeClass template, i.e. what a sync must leave untouched.
templateResources := corev1.ResourceRequirements{
Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")},
}

// seedTemplateResources reproduces the post-creation state: the template declares
// resources and the already-created workload carries them.
seedTemplateResources := func(stsName string, template *corev1.PodTemplateSpec) {
template.Spec.Containers[0].Resources = *templateResources.DeepCopy()

sts := &workloadv1alpha1.AdvancedStatefulSet{}
key := types.NamespacedName{Name: stsName, Namespace: "default"}
Expect(fakeClient.Get(ctx.Context, key, sts)).To(Succeed())
sts.Spec.Template.Spec.Containers[0].Resources = *templateResources.DeepCopy()
Expect(fakeClient.Update(ctx.Context, sts)).To(Succeed())
}

memLimitOf := func(stsName string) string {
sts := &workloadv1alpha1.AdvancedStatefulSet{}
key := types.NamespacedName{Name: stsName, Namespace: "default"}
Expect(fakeClient.Get(ctx.Context, key, sts)).To(Succeed())
limit := sts.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceMemory]
return limit.String()
}

BeforeEach(func() {
seedTemplateResources(masterSts, &runtimeClass.Topology.Master.Template)
seedTemplateResources(workerSts, &runtimeClass.Topology.Worker.Template)
})

Context("when the CacheRuntime does not specify resources", func() {
It("should leave the template's resources untouched", func() {
Expect(runtimeObj.Spec.Master.Resources.Limits).To(BeNil())
Expect(runtimeObj.Spec.Master.Resources.Requests).To(BeNil())
Expect(runtimeObj.Spec.Worker.Resources.Limits).To(BeNil())
Expect(runtimeObj.Spec.Worker.Resources.Requests).To(BeNil())

Expect(engine.syncRuntimeSpec(ctx, runtimeObj, runtimeClass)).To(Succeed())

Expect(memLimitOf(masterSts)).To(Equal("2Gi"))
Expect(memLimitOf(workerSts)).To(Equal("2Gi"))
})
})

Context("when the CacheRuntime specifies resources", func() {
It("should apply them to the master workload only", func() {
runtimeObj.Spec.Master.Resources = corev1.ResourceRequirements{
Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("4Gi")},
}

Expect(engine.syncRuntimeSpec(ctx, runtimeObj, runtimeClass)).To(Succeed())

Expect(memLimitOf(masterSts)).To(Equal("4Gi"))
Expect(memLimitOf(workerSts)).To(Equal("2Gi"))
})

It("should apply them to the worker workload only", func() {
runtimeObj.Spec.Worker.Resources = corev1.ResourceRequirements{
Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("4Gi")},
}

Expect(engine.syncRuntimeSpec(ctx, runtimeObj, runtimeClass)).To(Succeed())

Expect(memLimitOf(workerSts)).To(Equal("4Gi"))
Expect(memLimitOf(masterSts)).To(Equal("2Gi"))
})
})
})
})
Loading