From 67f03e61c6032e7f1f8c6073844473cf413e2278 Mon Sep 17 00:00:00 2001 From: btxu-db Date: Thu, 3 Sep 2026 00:13:53 +0900 Subject: [PATCH] fix(cache): complete a partial runtime version from the class template updateImage refuses to touch the workload unless both image and imageTag are restated on the CacheRuntime. VersionSpec declares the two as independent optional strings with no defaults and no validation, so naming only imageTag is a legal edit and the obvious way to move a component onto a newer build of the same image. It silently did nothing: the AdvancedStatefulSet was not patched, the pods kept the old image, and there was no error, no event and no condition, while the CacheRuntime's generation still incremented. The creation path already knew how to fall back to the CacheRuntimeClass template when the version is incomplete, so the same value expressed the same intent before and after creation but only worked before it. desiredComponentVersion completes the missing half from the image the CacheRuntimeClass declares for the component, and both paths call it, so a component created with a partial version and one updated to it resolve to the same image instead of rolling on the next reconcile. updateImage is unchanged; its guard is now a safety net rather than the operative rule. A version naming neither half still leaves the template image alone, and so does one whose missing half cannot be recovered - a template with no image, or one pinned by digest - because guessing there would move the workload onto something nobody asked for. splitImageReference is used rather than docker.ParseDockerImage because the latter splits on every colon, which turns a registry with a port and no tag into a bogus repository and tag pair. Fixes #6178 Signed-off-by: btxu-db --- pkg/ddc/cache/engine/sync.go | 4 +- pkg/ddc/cache/engine/transform_common.go | 68 +++++++- pkg/ddc/cache/engine/transform_common_test.go | 163 ++++++++++++++++++ 3 files changed, 230 insertions(+), 5 deletions(-) create mode 100644 pkg/ddc/cache/engine/transform_common_test.go diff --git a/pkg/ddc/cache/engine/sync.go b/pkg/ddc/cache/engine/sync.go index 29b8849cd06..dede362a0da 100644 --- a/pkg/ddc/cache/engine/sync.go +++ b/pkg/ddc/cache/engine/sync.go @@ -194,7 +194,7 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt } manager := component.NewComponentHelper(common.ComponentTypeMaster, e.Client) masterSpec := component.ComponentSpec{ - Version: runtime.Spec.Master.RuntimeVersion, + Version: desiredComponentVersion(runtime.Spec.Master.RuntimeVersion, componentTemplateImage(runtimeClass.Topology.Master)), Resources: desiredComponentResources(runtime.Spec.Master.Resources, runtimeClass.Topology.Master), Replicas: &runtime.Spec.Master.Replicas, } @@ -231,7 +231,7 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt } workerSpec := component.ComponentSpec{ - Version: runtime.Spec.Worker.RuntimeVersion, + Version: desiredComponentVersion(runtime.Spec.Worker.RuntimeVersion, componentTemplateImage(runtimeClass.Topology.Worker)), Resources: workerResources, Replicas: &runtime.Spec.Worker.Replicas, } diff --git a/pkg/ddc/cache/engine/transform_common.go b/pkg/ddc/cache/engine/transform_common.go index d95f92c1199..283faa8d8b8 100644 --- a/pkg/ddc/cache/engine/transform_common.go +++ b/pkg/ddc/cache/engine/transform_common.go @@ -18,6 +18,7 @@ package engine import ( "fmt" + "strings" datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" "github.com/fluid-cloudnative/fluid/pkg/common" @@ -119,9 +120,11 @@ func (e *CacheEngine) transformComponentPodTemplate(runtimeCompSpec datav1alpha1 // transform container related config, currently only modify the first container if len(podTemplate.Spec.Containers) > 0 { - // transform Container Image name etc. - if len(runtimeCompSpec.RuntimeVersion.Image) > 0 && len(runtimeCompSpec.RuntimeVersion.ImageTag) > 0 { - podTemplate.Spec.Containers[0].Image = runtimeCompSpec.RuntimeVersion.Image + ":" + runtimeCompSpec.RuntimeVersion.ImageTag + // transform Container Image name etc. A version naming only one of image and imageTag + // takes the other half from the template rather than being dropped. + version := desiredComponentVersion(runtimeCompSpec.RuntimeVersion, podTemplate.Spec.Containers[0].Image) + if len(version.Image) > 0 && len(version.ImageTag) > 0 { + podTemplate.Spec.Containers[0].Image = version.Image + ":" + version.ImageTag } if len(runtimeCompSpec.RuntimeVersion.ImagePullPolicy) > 0 { podTemplate.Spec.Containers[0].ImagePullPolicy = (corev1.PullPolicy)(runtimeCompSpec.RuntimeVersion.ImagePullPolicy) @@ -148,3 +151,62 @@ func (e *CacheEngine) transformComponentPodTemplate(runtimeCompSpec datav1alpha1 componentValue.PodTemplateSpec.Spec.InitContainers[0].Env = append(addEnvs, componentValue.PodTemplateSpec.Spec.InitContainers[0].Env...) } } + +// desiredComponentVersion completes a partially specified runtime version against the image +// carried by the CacheRuntimeClass template. +// +// VersionSpec declares image and imageTag as independent optional strings, so naming only one +// of them is a legal way to say "the same image on a newer tag". Both the creation path and +// updateImage used to require both halves and otherwise leave the template image alone, which +// silently discards that edit. Completing the missing half here gives both paths the same +// desired image, so a component created with a partial version and one updated to it end up +// identical instead of rolling on the next reconcile. +// +// A version naming neither half is returned untouched, leaving the template image in place. So +// is one whose missing half cannot be recovered - a template with no image, or an image pinned +// by digest - because guessing there would move the workload onto something nobody asked for. +func desiredComponentVersion(runtimeVersion datav1alpha1.VersionSpec, templateImage string) datav1alpha1.VersionSpec { + if runtimeVersion.Image == "" && runtimeVersion.ImageTag == "" { + return runtimeVersion + } + if runtimeVersion.Image != "" && runtimeVersion.ImageTag != "" { + return runtimeVersion + } + + templateRepository, templateTag := splitImageReference(templateImage) + if runtimeVersion.Image == "" { + runtimeVersion.Image = templateRepository + } + if runtimeVersion.ImageTag == "" { + runtimeVersion.ImageTag = templateTag + } + + return runtimeVersion +} + +// splitImageReference splits a container image reference into its repository and tag. The tag +// is empty when the reference carries none, and both are empty for a reference pinned by +// digest, where there is no tag to complete and appending one would not be valid. +func splitImageReference(image string) (repository string, tag string) { + if image == "" || strings.Contains(image, "@") { + return "", "" + } + + lastColon := strings.LastIndex(image, ":") + // A colon before the last slash belongs to a registry port, not to a tag. + if lastColon == -1 || lastColon < strings.LastIndex(image, "/") { + return image, "" + } + + return image[:lastColon], image[lastColon+1:] +} + +// componentTemplateImage returns the image the CacheRuntimeClass declares for a component, +// which is the baseline a partially specified runtime version is completed against. +func componentTemplateImage(componentDefinition *datav1alpha1.RuntimeComponentDefinition) string { + if componentDefinition == nil || len(componentDefinition.Template.Spec.Containers) == 0 { + return "" + } + + return componentDefinition.Template.Spec.Containers[0].Image +} diff --git a/pkg/ddc/cache/engine/transform_common_test.go b/pkg/ddc/cache/engine/transform_common_test.go new file mode 100644 index 00000000000..fcda1fb945e --- /dev/null +++ b/pkg/ddc/cache/engine/transform_common_test.go @@ -0,0 +1,163 @@ +/* +Copyright 2026 The Fluid Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package engine + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" + corev1 "k8s.io/api/core/v1" +) + +var _ = Describe("CacheEngine desiredComponentVersion Tests", Label("pkg.ddc.cache.engine.transform_common_test.go"), func() { + const templateImage = "btxu/mooncake:v3" + + Describe("desiredComponentVersion", func() { + It("should leave a version naming neither half untouched", func() { + desired := desiredComponentVersion(datav1alpha1.VersionSpec{}, templateImage) + + Expect(desired.Image).To(BeEmpty()) + Expect(desired.ImageTag).To(BeEmpty()) + }) + + It("should leave a complete version untouched", func() { + desired := desiredComponentVersion(datav1alpha1.VersionSpec{ + Image: "other/mooncake", + ImageTag: "v9", + }, templateImage) + + Expect(desired.Image).To(Equal("other/mooncake")) + Expect(desired.ImageTag).To(Equal("v9")) + }) + + It("should take the repository from the template when only imageTag is named", func() { + desired := desiredComponentVersion(datav1alpha1.VersionSpec{ImageTag: "v9"}, templateImage) + + Expect(desired.Image).To(Equal("btxu/mooncake")) + Expect(desired.ImageTag).To(Equal("v9")) + }) + + It("should take the tag from the template when only image is named", func() { + desired := desiredComponentVersion(datav1alpha1.VersionSpec{Image: "other/mooncake"}, templateImage) + + Expect(desired.Image).To(Equal("other/mooncake")) + Expect(desired.ImageTag).To(Equal("v3")) + }) + + It("should not carry imagePullPolicy into the completion", func() { + desired := desiredComponentVersion(datav1alpha1.VersionSpec{ + ImageTag: "v9", + ImagePullPolicy: "Always", + }, templateImage) + + Expect(desired.ImagePullPolicy).To(Equal("Always")) + }) + + It("should stay incomplete when the template declares no image", func() { + desired := desiredComponentVersion(datav1alpha1.VersionSpec{ImageTag: "v9"}, "") + + Expect(desired.Image).To(BeEmpty()) + Expect(desired.ImageTag).To(Equal("v9")) + }) + + It("should stay incomplete when the template pins a digest", func() { + desired := desiredComponentVersion(datav1alpha1.VersionSpec{ImageTag: "v9"}, + "btxu/mooncake@sha256:067614b70d25b496e3edc3480747d558ee8a364ef47a67f669f5d96ca5098552") + + Expect(desired.Image).To(BeEmpty()) + Expect(desired.ImageTag).To(Equal("v9")) + }) + }) + + Describe("splitImageReference", func() { + It("should split a plain tagged reference", func() { + repository, tag := splitImageReference("btxu/mooncake:v3") + + Expect(repository).To(Equal("btxu/mooncake")) + Expect(tag).To(Equal("v3")) + }) + + It("should report no tag rather than inventing one", func() { + repository, tag := splitImageReference("nginx") + + Expect(repository).To(Equal("nginx")) + Expect(tag).To(BeEmpty()) + }) + + It("should not mistake a registry port for a tag", func() { + repository, tag := splitImageReference("registry.local:5000/mooncake") + + Expect(repository).To(Equal("registry.local:5000/mooncake")) + Expect(tag).To(BeEmpty()) + }) + + It("should split a tag off a reference that also carries a registry port", func() { + repository, tag := splitImageReference("registry.local:5000/mooncake:v3") + + Expect(repository).To(Equal("registry.local:5000/mooncake")) + Expect(tag).To(Equal("v3")) + }) + + It("should refuse to split a digest reference", func() { + repository, tag := splitImageReference("btxu/mooncake@sha256:0676") + + Expect(repository).To(BeEmpty()) + Expect(tag).To(BeEmpty()) + }) + }) + + Describe("componentTemplateImage", func() { + It("should return an empty image for a nil component definition", func() { + Expect(componentTemplateImage(nil)).To(BeEmpty()) + }) + + It("should return an empty image for a template with no containers", func() { + Expect(componentTemplateImage(&datav1alpha1.RuntimeComponentDefinition{})).To(BeEmpty()) + }) + + It("should return the first container's image", func() { + Expect(componentTemplateImage(&datav1alpha1.RuntimeComponentDefinition{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "worker", Image: templateImage}}, + }, + }, + })).To(Equal(templateImage)) + }) + }) + + Describe("the creation and the sync path agreeing", func() { + It("should resolve an imageTag-only version to the same image on both paths", func() { + // creation resolves against the image already on the template copy + creationVersion := desiredComponentVersion(datav1alpha1.VersionSpec{ImageTag: "v9"}, templateImage) + + // sync resolves against the CacheRuntimeClass component definition + syncVersion := desiredComponentVersion(datav1alpha1.VersionSpec{ImageTag: "v9"}, + componentTemplateImage(&datav1alpha1.RuntimeComponentDefinition{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "worker", Image: templateImage}}, + }, + }, + })) + + Expect(creationVersion).To(Equal(syncVersion)) + Expect(creationVersion.Image + ":" + creationVersion.ImageTag).To(Equal("btxu/mooncake:v9")) + }) + }) +})