From 2a4968a94ccbce9eb843b2a95779bfd28f9c1b93 Mon Sep 17 00:00:00 2001 From: btxu-db Date: Mon, 17 Aug 2026 12:43:36 +0900 Subject: [PATCH 1/2] fix(cache): preserve CacheRuntimeClass template resources when unset 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 --- .../component/advanced_statefulset_manager.go | 6 +- pkg/ddc/cache/component/component_manager.go | 2 +- .../component/sync_component_spec_test.go | 6 +- pkg/ddc/cache/engine/sync.go | 8 +- pkg/ddc/cache/engine/sync_test.go | 74 +++++++++++++++++++ 5 files changed, 86 insertions(+), 10 deletions(-) diff --git a/pkg/ddc/cache/component/advanced_statefulset_manager.go b/pkg/ddc/cache/component/advanced_statefulset_manager.go index bb9af248ca0..3dc43376b90 100644 --- a/pkg/ddc/cache/component/advanced_statefulset_manager.go +++ b/pkg/ddc/cache/component/advanced_statefulset_manager.go @@ -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 + } } // Skip patching if no changes detected diff --git a/pkg/ddc/cache/component/component_manager.go b/pkg/ddc/cache/component/component_manager.go index ce90dcaa51b..7f18149a8fe 100644 --- a/pkg/ddc/cache/component/component_manager.go +++ b/pkg/ddc/cache/component/component_manager.go @@ -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 { diff --git a/pkg/ddc/cache/component/sync_component_spec_test.go b/pkg/ddc/cache/component/sync_component_spec_test.go index 1af1648e43f..d719169e36b 100644 --- a/pkg/ddc/cache/component/sync_component_spec_test.go +++ b/pkg/ddc/cache/component/sync_component_spec_test.go @@ -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"), @@ -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"), @@ -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"), }, diff --git a/pkg/ddc/cache/engine/sync.go b/pkg/ddc/cache/engine/sync.go index c7c45c6c24e..ea3ef178914 100644 --- a/pkg/ddc/cache/engine/sync.go +++ b/pkg/ddc/cache/engine/sync.go @@ -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 } masterSpec := component.ComponentSpec{ Version: runtime.Spec.Master.RuntimeVersion, @@ -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, diff --git a/pkg/ddc/cache/engine/sync_test.go b/pkg/ddc/cache/engine/sync_test.go index 509a417dbb6..6f84fd38157 100644 --- a/pkg/ddc/cache/engine/sync_test.go +++ b/pkg/ddc/cache/engine/sync_test.go @@ -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" @@ -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")) + }) + }) + }) }) From 4389ec740c8bfd9af927949bb43fc6bdc612abe0 Mon Sep 17 00:00:00 2001 From: btxu-db Date: Fri, 28 Aug 2026 14:59:34 +0900 Subject: [PATCH 2/2] fix(cache): fall back to the CacheRuntimeClass template resources Motivation: Review feedback on #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 --- .../cacheruntime/cacheruntime_spec_update.md | 11 ++++ .../cacheruntime/cacheruntime_spec_update.md | 10 ++++ pkg/ddc/cache/engine/sync.go | 41 +++++++++----- pkg/ddc/cache/engine/sync_test.go | 53 +++++++++++++++++++ 4 files changed, 101 insertions(+), 14 deletions(-) diff --git a/docs/en/samples/cacheruntime/cacheruntime_spec_update.md b/docs/en/samples/cacheruntime/cacheruntime_spec_update.md index d60788cef8f..2d54d7f7f84 100644 --- a/docs/en/samples/cacheruntime/cacheruntime_spec_update.md +++ b/docs/en/samples/cacheruntime/cacheruntime_spec_update.md @@ -73,8 +73,19 @@ spec: memory: 16Gi ``` +**Resolution order**: + +`resources` is resolved as follows on every reconcile: + +1. the value set on the CacheRuntime, if it declares any `requests` or `limits`; +2. otherwise the value declared by the CacheRuntimeClass template; +3. otherwise nothing is synced and the workload keeps its current resources. + **Limitations**: - ⚠️ Cannot exceed the node's available resources. +- ⚠️ When the CacheRuntimeClass template declares `resources`, removing `resources` from the + CacheRuntime does **not** leave the component unconstrained — it falls back to the template value. + To relax a limit, set the value you want explicitly instead of removing the field. - ⚠️ **Kubernetes version requirement**: K8s >= 1.27 with the `InPlacePodVerticalScaling` Feature Gate enabled. ```bash # Check if the Feature Gate is enabled diff --git a/docs/zh/samples/cacheruntime/cacheruntime_spec_update.md b/docs/zh/samples/cacheruntime/cacheruntime_spec_update.md index d18fb8bed75..ad68460f85d 100644 --- a/docs/zh/samples/cacheruntime/cacheruntime_spec_update.md +++ b/docs/zh/samples/cacheruntime/cacheruntime_spec_update.md @@ -73,8 +73,18 @@ spec: memory: 16Gi ``` +**取值优先级**： + +每次 reconcile 时，`resources` 按以下顺序取值： + +1. CacheRuntime 上设置的值（只要声明了 `requests` 或 `limits`）； +2. 否则取 CacheRuntimeClass 模板中声明的值； +3. 两者都未声明时不做同步，工作负载保持当前的资源配置。 + **限制**： - ⚠️ 不能超过节点可用资源 +- ⚠️ 当 CacheRuntimeClass 模板声明了 `resources` 时，从 CacheRuntime 中删除 `resources` **不会** + 让组件变为不受限，而是回退到模板中的值。如需放宽限制，请显式设置目标值，而不是删除该字段。 - ⚠️ **Kubernetes 版本要求**：需要 K8s >= 1.27 且启用 `InPlacePodVerticalScaling` Feature Gate ```bash # 检查 Feature Gate 是否启用 diff --git a/pkg/ddc/cache/engine/sync.go b/pkg/ddc/cache/engine/sync.go index ea3ef178914..a197649546d 100644 --- a/pkg/ddc/cache/engine/sync.go +++ b/pkg/ddc/cache/engine/sync.go @@ -193,15 +193,9 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt Namespace: e.namespace, } 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 - if runtime.Spec.Master.Resources.Requests != nil || runtime.Spec.Master.Resources.Limits != nil { - resources = &runtime.Spec.Master.Resources - } masterSpec := component.ComponentSpec{ Version: runtime.Spec.Master.RuntimeVersion, - Resources: resources, + Resources: desiredComponentResources(runtime.Spec.Master.Resources, runtimeClass.Topology.Master), Replicas: &runtime.Spec.Master.Replicas, } if err := manager.SyncComponentSpec(ctx.Context, masterIdentity, masterSpec); err != nil { @@ -217,15 +211,9 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt Namespace: e.namespace, } 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 - if runtime.Spec.Worker.Resources.Requests != nil || runtime.Spec.Worker.Resources.Limits != nil { - workerResources = &runtime.Spec.Worker.Resources - } workerSpec := component.ComponentSpec{ Version: runtime.Spec.Worker.RuntimeVersion, - Resources: workerResources, + Resources: desiredComponentResources(runtime.Spec.Worker.Resources, runtimeClass.Topology.Worker), Replicas: &runtime.Spec.Worker.Replicas, } if err := manager.SyncComponentSpec(ctx.Context, workerIdentity, workerSpec); err != nil { @@ -240,6 +228,31 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt return nil } +// desiredComponentResources resolves the resources that should be synced to a +// component's workload. A value set on the CacheRuntime wins; when the CacheRuntime +// sets none, the CacheRuntimeClass template value is used, which is what the creation +// path rendered into the workload. A nil return means neither declares resources, and +// the workload's current resources are left untouched. +// +// Only the first container is considered, matching the creation path, which also only +// fills in resources for Containers[0]. +func desiredComponentResources(runtimeResources corev1.ResourceRequirements, componentDefinition *datav1alpha1.RuntimeComponentDefinition) *corev1.ResourceRequirements { + if runtimeResources.Requests != nil || runtimeResources.Limits != nil { + return runtimeResources.DeepCopy() + } + + if componentDefinition == nil || len(componentDefinition.Template.Spec.Containers) == 0 { + return nil + } + + templateResources := componentDefinition.Template.Spec.Containers[0].Resources + if templateResources.Requests == nil && templateResources.Limits == nil { + return nil + } + + return templateResources.DeepCopy() +} + func (e *CacheEngine) syncDatasetCacheStates(ctx cruntime.ReconcileRequestContext, runtime *datav1alpha1.CacheRuntime, runtimeClass *datav1alpha1.CacheRuntimeClass) (err error) { cacheStates, err := e.GetCacheStates(runtime, runtimeClass) if err != nil { diff --git a/pkg/ddc/cache/engine/sync_test.go b/pkg/ddc/cache/engine/sync_test.go index 6f84fd38157..f307f40c13a 100644 --- a/pkg/ddc/cache/engine/sync_test.go +++ b/pkg/ddc/cache/engine/sync_test.go @@ -931,5 +931,58 @@ var _ = Describe("CacheEngine Sync Tests", Label("pkg.ddc.cache.engine.sync_test Expect(memLimitOf(masterSts)).To(Equal("2Gi")) }) }) + + Context("when the workload no longer matches the template", func() { + // setWorkloadMemLimit edits the workload behind the runtime's back, standing in + // for a workload that drifted from the template for any reason. + setWorkloadMemLimit := func(stsName, limit string) { + 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 = corev1.ResourceRequirements{ + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse(limit)}, + } + Expect(fakeClient.Update(ctx.Context, sts)).To(Succeed()) + } + + It("should restore the template value when the CacheRuntime specifies none", func() { + setWorkloadMemLimit(workerSts, "8Gi") + 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(workerSts)).To(Equal("2Gi")) + }) + + It("should still let the CacheRuntime win over the template", func() { + setWorkloadMemLimit(workerSts, "8Gi") + 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")) + }) + }) + + Context("when neither the CacheRuntime nor the template specifies resources", func() { + It("should leave the workload's resources untouched", func() { + runtimeClass.Topology.Worker.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{} + + sts := &workloadv1alpha1.AdvancedStatefulSet{} + key := types.NamespacedName{Name: workerSts, Namespace: "default"} + Expect(fakeClient.Get(ctx.Context, key, sts)).To(Succeed()) + sts.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{ + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("8Gi")}, + } + Expect(fakeClient.Update(ctx.Context, sts)).To(Succeed()) + + Expect(engine.syncRuntimeSpec(ctx, runtimeObj, runtimeClass)).To(Succeed()) + + Expect(memLimitOf(workerSts)).To(Equal("8Gi")) + }) + }) }) })