fix(cache): complete a partial runtime version from the class template - #6186
fix(cache): complete a partial runtime version from the class template#6186btxu-db wants to merge 1 commit into
Conversation
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 fluid-cloudnative#6178 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 #6186 +/- ##
==========================================
+ Coverage 65.24% 65.27% +0.03%
==========================================
Files 486 486
Lines 34194 34226 +32
==========================================
+ Hits 22309 22341 +32
Misses 10135 10135
Partials 1750 1750 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
What this PR does
Fixes #6178.
updateImagerefused to touch the workload unless both halves of the image reference were restated on the CacheRuntime:VersionSpecdeclaresimage,imageTagandimagePullPolicyas three independent optional strings, with no defaults and no validation, so setting onlyimageTagis a legal edit and the obvious way to say "same image, newer build". It did nothing: no patch, no event, no condition, while the CacheRuntime'sgenerationstill incremented, so from the outside the edit looked accepted.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.
How
desiredComponentVersioncompletes the missing half from the image the CacheRuntimeClass declares for the component, and both the creation path andsyncRuntimeSpeccall it. A component created with a partial version and one updated to it now resolve to the same image instead of rolling on the next reconcile.updateImageitself is unchanged. Its guard is now a safety net rather than the operative rule.Two cases are deliberately left alone, because guessing would move the workload onto something nobody asked for:
splitImageReferenceis used rather thandocker.ParseDockerImagebecause the latter splits on every colon, which turns a registry with a port and no tag (registry.example.com:5000/repo) into a bogus repository and tag pair.Unit tests
New
transform_common_test.gocoversdesiredComponentVersionandsplitImageReference: tag-only, image-only, both, neither, an empty template image, a digest-pinned template, and a registry with a port.pkg/ddc/cache/enginehas 12 failing specs on master already, all inufs_test.goandsync_test.goaround mount handling. This branch has the same 12 and adds 16 passing ones (243 passed / 12 failed onf2785f84, 259 passed / 12 failed here), so the pre-existing failures are untouched by this change.Verified on a cluster
kind v0.23.0, Kubernetes v1.30.0, controller built from this branch and loaded into the cluster.
Creation with only
imageTag. Two runs, each with a CacheRuntimeClass whose worker template pins an image and a CacheRuntime naming only a tag:busybox:1.36worker.runtimeVersion.imageTag: "1.37"busybox:1.37mooncake:v3worker.runtimeVersion.imageTag: "v9"mooncake:v9In both runs the master component, which named no
runtimeVersionat all, stayed on the template image. That is the "neither half named, change nothing" branch.Updating a live component to only
imageTag. This is the case the issue reports. On a runtime already past setup with a healthy worker, whose spec carriedruntimeVersion: {imageTag: v9}against a template ofmooncake:v3, flipping the tag tov3:Both lines carry the same
reconcileID, and the AdvancedStatefulSet moved tomooncake:v3with its generation going 5 to 6 within ten seconds. Before this change the same edit produced no patch at all.One note on the setup, since it shaped how the second case had to be exercised. A first attempt used an image tag the cluster could not pull, which left the worker in
ImagePullBackOff; the runtime then never left setup, sosyncRuntimeSpecwas never called and the update path could not be reached at all. That is a property of the test environment rather than of the change, but it is worth knowing that this path only runs once a runtime is past setup.