Repository navigation
Conversation
|
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6186 +/- ##
==========================================
+ Coverage 65.47% 65.68% +0.21%
==========================================
Files 486 486
Lines 34307 34417 +110
==========================================
+ Hits 22463 22608 +145
+ Misses 10085 10057 -28
+ Partials 1759 1752 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // 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 |
| // 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. |
There was a problem hiding this comment.
please add an example for this case in the comment, like registry:5000/myapp:v1
| // 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, "@") { |
There was a problem hiding this comment.
what's the behavior when runtimeclass template pod use ‘@’ digest image, does the worker/master pod start and sync normally? Should we just not support this format image?
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>
The runtimeVersion row pinned the drop-the-tag behaviour as a known bug of fluid-cloudnative#6178. With a partial version now completed from the CacheRuntimeClass template, it expects what that row said it should become: fluid/cache:v2 on both the create and the update path. Signed-off-by: btxu-db <btxu-db@outlook.com>
67f03e6 to
c925535
Compare
Signed-off-by: btxu-db <btxu-db@outlook.com>
Spell out what the colon-before-slash check guards against with a concrete pair: "registry:5000/myapp" carries no tag, while "registry:5000/myapp:v1" splits into "registry:5000/myapp" and "v1". Signed-off-by: btxu-db <btxu-db@outlook.com>
There was a problem hiding this comment.
🟡 Changes recommended
DataLoad fallback still ignores partial worker versions and can run a different image from the worker.
2 open findings
What changed in this PR
Completes partial CacheRuntime image versions from CacheRuntimeClass templates for consistent creation and synchronization.
Changes:
- Adds image-reference parsing and version completion.
- Applies completion during creation and runtime sync, with expanded tests.
| File | Description |
|---|---|
pkg/ddc/cache/engine/transform_common.go |
Adds version completion helpers. |
pkg/ddc/cache/engine/transform_common_test.go |
Tests image completion scenarios. |
pkg/ddc/cache/engine/sync.go |
Resolves versions before component synchronization. |
pkg/ddc/cache/engine/sync_test.go |
Tests rollback to template images. |
pkg/ddc/cache/engine/composition_matrix_test.go |
Updates partial-version expectations. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| // override. A version whose missing half cannot be recovered - a template with no image, no tag, | ||
| // or an image pinned by digest - stays incomplete, because guessing there would move the | ||
| // workload onto something nobody asked for. | ||
| func desiredComponentVersion(runtimeVersion datav1alpha1.VersionSpec, templateImage string) datav1alpha1.VersionSpec { |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
The premise of #6178 checks out and the fix is sound: both reviewers independently reproduced the silent drop of imageTag-only/image-only versions on the merge base and confirmed the PR resolves it on both the create and update paths, with the 12 pre-existing failures unchanged. The two reviews disputed nothing — each independently confirmed every finding of the other — but together they surfaced one merge-blocking bug beyond the PR's own claims: desiredComponentVersion completes the missing tag of a digest-pinned runtime image (runtimeVersion {image: "repo@sha256:..."} with no imageTag), rendering the invalid reference "repo@sha256:...:v1" onto the workload on both paths — proven unparseable with the same reference parser the kubelet uses, and a regression against base, which safely dropped this input; the one-line guard (skip tag completion when the supplied image contains '@') was verified green by both harnesses. Also worth addressing: getDataOperationImage still requires both halves, so DataLoad pods now diverge from the upgraded worker image for partial versions; class template image edits now propagate to every existing empty-version runtime at the next sync (intended? release note?); unrecoverable partial versions are still dropped with no signal; and the image-only half of the sync path is untested. Copilot (advisory) independently flags the same DataLoad image divergence, plus one cosmetic test-name nit.
| if runtimeVersion.Image == "" { | ||
| runtimeVersion.Image = templateRepository | ||
| } | ||
| if runtimeVersion.ImageTag == "" { |
There was a problem hiding this comment.
desiredComponentVersion refuses to complete from a digest-pinned template (splitImageReference returns empty for '@'), but never checks the runtime-supplied image half. With runtimeVersion {image: "repo@sha256:..."} and no imageTag against a tagged class template, the tag is completed and both renderers (transformComponentPodTemplate on the create path, updateImage via syncRuntimeSpec on the update path) emit "repo@sha256:...:v1" — not a parseable image reference, so pods fail with "couldn't parse image reference". This contradicts the function's own documented policy ("a version whose missing half cannot be recovered ... stays incomplete") and regresses against base, where the both-halves guard safely dropped such an edit. One-line fix, verified by both reviewers' harnesses: leave the tag incomplete when runtimeVersion.Image contains '@' (if runtimeVersion.ImageTag == "" && !strings.Contains(runtimeVersion.Image, "@")).
| // override. A version whose missing half cannot be recovered - a template with no image, no tag, | ||
| // or an image pinned by digest - stays incomplete, because guessing there would move the | ||
| // workload onto something nobody asked for. | ||
| func desiredComponentVersion(runtimeVersion datav1alpha1.VersionSpec, templateImage string) datav1alpha1.VersionSpec { |
There was a problem hiding this comment.
When the template image is tagless or digest-pinned, an image-only or imageTag-only edit completes to nothing and is dropped on both the create and sync paths with no event, log line, or condition — the same 'accepted and discarded' UX issue #6178 objects to, in a corner the PR deliberately does not guess in. The code comment's reasoning (guessing would move the workload onto something nobody asked for) is sound; a V(1) log or Warning event when the completion fails would close the loop for the user.
| 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)), |
There was a problem hiding this comment.
Because desiredComponentVersion also completes an empty version to the template image, syncRuntimeSpec now (a) rolls a workload back to the class template when a runtimeVersion override is removed — documented and tested by the author — (b) propagates CacheRuntimeClass template image edits to every live runtime of that class that set no version at the next periodic sync — previously the image was frozen at creation — and (c) on controller upgrade, runtimes created under the old partial-version fallback roll to the completed image on their first sync. The behavior mirrors the sibling resources path (desiredComponentResources), so it reads as deliberate, but (b) makes a shared, cluster-scoped class newly mutating for existing workloads and (c) is an upgrade-time fleet roll. Intended? If yes, a release note would help upgraders.
| if runtimeVersion.Image != "" && runtimeVersion.ImageTag != "" { | ||
| return runtimeVersion | ||
| } | ||
|
|
There was a problem hiding this comment.
For an image-only version against a tagged template (e.g. {image: "other/repo"} + template "btxu/mooncake:v3" → "other/repo:v3"), the completed tag is transplanted across repositories and may not exist there, causing ImagePullBackOff. Issue #6178 explicitly asks for the symmetric image-only behavior and the PR's own test codifies the transplant, so this reads as intended — confirming the risk is accepted rather than accidental. Note the failure mode is loud (ImagePullBackOff with a clear event), unlike the silent-drop class the issue objects to.
| seedTemplateResources(workerSts, &runtimeClass.Topology.Worker.Template) | ||
| }) | ||
|
|
||
| Context("when the CacheRuntime drops a runtimeVersion it had set", func() { |
There was a problem hiding this comment.
The PR's new sync test covers only imageTag-only and the empty-version rollback; image-only appears only at the unit layer (transform_common_test.go:210), and the composition matrix has no image-only row either — the create path is equally unpinned for that half. The two halves exercise genuinely different branches of splitImageReference (template repository vs. port-aware template tag extraction), so the sync path deserves its own image-only assertion.
| Expect(desired.ImageTag).To(Equal("v3")) | ||
| }) | ||
|
|
||
| It("should not carry imagePullPolicy into the completion", func() { |
There was a problem hiding this comment.
The test named 'should not carry imagePullPolicy into the completion' actually asserts the policy IS preserved (Equal("Always")). Rename it to describe preservation so a failure is reported with the intended behavior. Cosmetic only.
desiredComponentVersion refused to complete against a digest-pinned
template, but never looked at the image the CacheRuntime supplied. A
runtimeVersion of {image: "repo@sha256:..."} with no imageTag, against a
tagged template, had the template tag appended and both the creation
path and updateImage rendered "repo@sha256:...:v1", which no runtime can
parse, so the pods failed to start. Before the completion was added the
both-halves guard dropped this input and kept the template image.
A digest reference takes no tag, so leave the version incomplete when
the supplied image contains '@', matching what the function already does
for a digest-pinned template.
Signed-off-by: btxu-db <btxu-db@outlook.com>
The image-only half of the completion was only pinned at the unit level. Add a composition matrix row, which runs both the creation and the sync path, over a template whose registry carries a port, so the port is not taken for the tag, and a syncRuntimeSpec case that sets an image only and then drops it again. Signed-off-by: btxu-db <btxu-db@outlook.com>
The case asserted that the runtime imagePullPolicy is kept through the completion, while its name said the opposite. Signed-off-by: btxu-db <btxu-db@outlook.com>
getDataOperationImage, which picks the image for a DataLoad that names none, still required both image and imageTag on the worker runtimeVersion. With only imageTag set the worker now runs the completed image while the DataLoad fell back to the old template image. Move the "complete the version, else keep the template image" step into componentImage and use it both for the component workload and for data operations, so the two cannot drift apart again. Signed-off-by: btxu-db <btxu-db@outlook.com>
When the template image has no tag or is pinned by digest, or the supplied image is pinned by digest, desiredComponentVersion leaves an image-only or imageTag-only version incomplete rather than guessing, and the edit is dropped on both the creation and the sync path. Nothing recorded that, which is the same accepted-and-discarded experience fluid-cloudnative#6178 is about. Log it at V(1) from both paths. A Warning event would repeat on every periodic sync for as long as the CacheRuntime keeps the version. Signed-off-by: btxu-db <btxu-db@outlook.com>
… and the upgrade update Consequences of completing a partial runtimeVersion that operators need to know: - An image-only version takes the template's tag, not latest, so the tag has to exist in the new repository. - Editing the image in a CacheRuntimeClass template now reaches every CacheRuntime using that class that does not fully set runtimeVersion, on its next reconcile. - Removing runtimeVersion returns the component to the template image, except when that image has no tag or is pinned by digest. - After upgrading, a CacheRuntime that set only image or imageTag gets it applied on the first reconcile, which updates the pods once. Signed-off-by: btxu-db <btxu-db@outlook.com>
…en its limits - State up front that the CacheRuntimeClass template image is the baseline on creation and on every reconcile, and that image and imageTag in the CacheRuntime override it. - A full image + imageTag is used as is, even with a digest-pinned template. A partial version that cannot be completed uses the template image on creation and keeps the current image on update. - Data operations fall back to the template image when the worker's version cannot be completed, so they can differ from such a worker. - Template image edits re-resolve enabled masters and workers and roll them only when the resolved image changes. - The upgrade note updates a component once only when it resolves to a complete image different from the current one, which also covers a runtimeVersion removed under an earlier release. Signed-off-by: btxu-db <btxu-db@outlook.com>
Additive verification harness for the review of PR fluid-cloudnative#6186 (partial runtime version completion). Production code is untouched - the diff against the PR head outside docs/verification/ and the three verify_pr6186_*_test.go files is empty. - P0 (premise, runs against base): imageTag-only runtimeVersion is dropped on both the create and update paths - confirmed on merge-base d8b37f2. - F1 (contract, red on PR head): a digest-pinned runtimeVersion.image with no tag gets the template tag appended, rendering the invalid reference repo@sha256:...:v1 on both paths (invalidity proven with distribution/reference in refcheck/). - F2 (canary, green on PR head, flips on base): CacheRuntimeClass template image edits now propagate to existing empty-version runtimes at next sync.
… probe, F2 promoted to contract Additive harness only; production code untouched.
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 2 on 0361cc0..382d926 — the round-1 blockers are fixed and verified. getDataOperationImage now resolves data-operation images through the same componentImage completion the worker uses (99659bf), so an imageTag-only upgrade no longer splits worker and DataLoad images; and the digest guard (7c7abfb) stops a digest-pinned runtimeVersion.image from gaining the template tag and rendering an unpullable repo@sha256:...:v1. Both fixes flipped red→green in both reviewers' harnesses, the class-template propagation semantics are now documented en+zh (including the reconcile-delay caveat and the upgrade note), and the full engine suite shows no failures beyond the 12 pre-existing mount-handling ones. CI is green at this head across k8s v1.22–v1.33 (tide is queued, not a check).
What remains is small but real: the only signal for the deliberately un-completable corner cases is a V(1) log, the shipped chart pins --development=false, and the binary registers no verbosity flag — so that signal can never fire in production, and the docs' -v=1 hint would actually crash-loop the controller (Copilot's C4276849880, verified by both of us). Logging at V(0) or emitting a Warning event from syncRuntimeSpec would meet issue #6178's stated minimum. The one meaningful disagreement: Codex closed the template-propagation concern as fixed-by-documentation, while this synthesis keeps a maintainer-confirmation question open — documentation makes a fleet-wide behavior change legible, but intent is a maintainer's call.
Next round
[MUST] (none — no open PR-introduced blocker/major; F1 and F4 are verified fixed)
[SHOULD] Make the discarded-edit signal visible under the shipped config (V(0) log or Warning event from syncRuntimeSpec) and fix the -v=1 doc hint (F6)
[SHOULD] Maintainer: confirm reconcile-time-only class-template propagation is the intended contract (F2)
[SHOULD] Guard or document the pre-existing digest+tag both-halves render (F5, pre-existing — issue candidate, not this PR's)
[SHOULD] Add a dedicated e2e case for the partial-version update path (F7); one doc clause for the symmetric tag-only hazard (N1)
[Filed] (none — no tracking issues opened this round)
Non-blocking observations (6) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/ddc/cache/engine/transform_common.go:204 — Residual accept-and-drop corners for un-completable partial versions, now documented [minor]
- pkg/ddc/cache/engine/transform_common.go:249 — Discarded-edit signal is invisible under the shipped controller config, and the docs' -v=1 hint breaks startup [minor]
- pkg/ddc/cache/engine/transform_common.go:205 — A version setting both image (digest-pinned) and imageTag renders an invalid reference [minor] — pre-existing, not introduced by this PR
- pkg/ddc/cache/engine/transform_common.go:204 — Rollback-to-template and reconcile-time-only template propagation semantics — maintainer confirmation reque... [minor]
- test/e2e:1 — No in-repo e2e case for the partial-version update path [minor]
- docs/en/samples/cacheruntime/cacheruntime_spec_update.md:56 — docs: warn about the symmetric tag-only + template-repo-change hazard [nit]
Verified review delta
- F1: fixed (initial-pr)
- F4: fixed (initial-pr)
- F3: open (initial-pr)
- F6: open (initial-pr)
- F5: open (preexisting)
- F2: open (initial-pr)
- F7: open (initial-pr)
- N1: open (initial-pr)
Code: pending. CI: success; attribution: baseline. Required CI checks still apply.
CacheRuntime has not shipped in a release yet, so there is no earlier behavior to upgrade from. The rules above already describe how a partial runtimeVersion, a removed runtimeVersion and a template image edit are resolved. Signed-off-by: btxu-db <btxu-db@outlook.com>
…flag A runtimeVersion naming one half the template cannot complete was only logged at V(1), which the shipped controller never enables, and the docs' -v=1 hint is not a flag the controller accepts. Log it at the default verbosity and emit a RuntimeVersionIncomplete Warning event on the CacheRuntime from syncRuntimeSpec, and point the docs at the event. Signed-off-by: btxu-db <btxu-db@outlook.com>
…pository change The image-only case already warns that the template's tag must exist in the new repository. The tag-only case has the mirror hazard: when the template's repository changes, the instance pulls its own tag from the new repository. Signed-off-by: btxu-db <btxu-db@outlook.com>
Setup waits for the components to be ready before the reconcile reaches Sync. A component created from a template image that never becomes ready therefore never showed the event, although the docs promise it in both cases. Report from transformMaster, transformWorker and transformClient with the runtime object, the same way syncRuntimeSpec does. Signed-off-by: btxu-db <btxu-db@outlook.com>
…vent Assert the Warning type alongside the reason, and that each report emits the event once. Signed-off-by: btxu-db <btxu-db@outlook.com>
…n on creation only The client is not synced after creation, so an edit to its runtimeVersion never reaches the update path that emits the event. Signed-off-by: btxu-db <btxu-db@outlook.com>
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 3 on 382d926..902625a — the round-2 SHOULD items landed, and the round-2 failed gate is now green. The discarded-edit signal is real: reportIncompleteVersion (8430488, a1bf08b) logs at the default verbosity and emits a RuntimeVersionIncomplete Warning event on the CacheRuntime, on the sync path and on the creation path (the report moved out of transformComponentPodTemplate, correctly — Setup is gated behind !IsSetupDone, so a workload stuck unready never reaches Sync), and the docs' -v=1 hint, which crash-looped the controller, is gone (that closes Copilot's round-2 C4276849880). Both of us verified it independently: the observability contracts are red on the previous head and green on this one, a revert-to-V(1) bites check turns them red again, the report call sites pass the exact template image the completion consumes, and the PR's own tests pin the event type and count. Everything earlier stays fixed: F1 (worker/DataLoad image split) and F4 (digest-tag completion) are untouched by this delta and green in both harnesses, the symmetric tag-only hazard got its doc clause in both languages, the dropped upgrade note is factually right (no release contains the CacheRuntime — verified against git tags), and the full engine suite shows only the 12 recorded pre-existing mount-handling failures (316/12 and 308/12 in our two runs). CI is green at this head across k8s v1.22–v1.33; tide is the merge queue, not a check.
Remaining, all non-blocking: F2 — whether reconcile-time-only class-template propagation to live instances is the intended product contract is still a maintainer call (documented, nobody has confirmed); F5 — the pre-existing digest+tag both-halves render (repo@sha256:...:v9), a guard or doc line away from closed, an issue candidate rather than this PR's; F7 — still no e2e case for the partial-version update path; and two nits on the new signal: N2, the event message's "so the image is left unchanged" reads wrong on the creation path, and N3, the event re-fires on every sync pass for a persistently un-completable version (the k8s event correlator aggregates it, but first-appearance-only emission would be tidier). The reviewers' one recorded disagreement this round was cosmetic and is resolved in the ledger: Codex kept F3 open as "documented design" where Claude marked it fixed — same disposition, no remaining ask, unified as fixed (the silence was the concern, and the event ends it).
Next round
[MUST] (none — no open PR-introduced blocker/major; F1, F4, F6, N1 and the round-2 failed gate AC6 are verified fixed at this head)
[SHOULD] Maintainer: confirm reconcile-time-only class-template propagation is the intended contract (F2)
[SHOULD] Guard or document the pre-existing digest+tag both-halves render (F5, pre-existing — issue candidate, not this PR's)
[SHOULD] Add a dedicated e2e case for the partial-version update path (F7)
[COULD] Neutral wording for the RuntimeVersionIncomplete event message on the creation path (N2); emit the event only on first appearance, or accept the correlator aggregation (N3)
[Filed] (none — no tracking issues opened this round)
Non-blocking observations (2) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/ddc/cache/engine/transform_common.go:255 — Event message 'so the image is left unchanged' reads wrong on the creation path [nit]
- pkg/ddc/cache/engine/transform_common.go:254 — The RuntimeVersionIncomplete event and log repeat for a persistently un-completable version [nit]
Verified review delta
- F6: fixed (initial-pr)
- F3: fixed (initial-pr)
- N1: fixed (initial-pr)
- N2: open (initial-pr)
- N3: open (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.
… and update The message said the image is left unchanged, which only holds on update: on creation the component starts from the template image. Say the version is ignored instead, which is true on both paths. Signed-off-by: btxu-db <btxu-db@outlook.com>
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 4 on 12d9b90 (delta 902625a..12d9b90, a single wording-only commit): the RuntimeVersionIncomplete event and log now say the partial version is ignored, which reads correctly on both the create path (component built from the template image) and the update path (image kept unchanged). Re-ran the verification contracts on the new head — partial-version completion on create and sync, data-operation image consistency, the digest/tag guard, corner cases, and the F6 observability contracts (V(0) log plus Warning event) all pass; CI green. LGTM.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 4 on 902625af..12d9b900 is one commit, and it does exactly what the last review asked: the RuntimeVersionIncomplete event and log no longer say "so the image is left unchanged" — true only on the update path — and now say the partial version "is ignored", which reads correctly on the creation path too, where the component is created from the template image. That matters because reportIncompleteVersion is the single emission point shared by both paths. Both of us proved it independently: each harness gained a wording contract that is red on the previous head and green on this one (Claude's full suite went 316/13 → 317/12 with the red failure quoting the old message verbatim; Codex's two wording specs failed on "unchanged" at 902625af and pass 2/2 at head). No production patch was involved — the flip is the bites check. Nothing else moved: the delta is two format strings and a comment in transform_common.go, image.go/sync.go are byte-identical, the PR's own tests pin the event prefix rather than the message, the old wording survives only in a vendored grpc comment, and the full engine suite still shows only the 12 recorded pre-existing mount-handling failures (317/12 and 310/12 in our two runs). Everything earlier stays verified fixed: F1 (worker/DataLoad image split), F4 (digest-tag completion), F6 (the round-2 failed observability gate), F3, and the N1 doc clause. CI is green at this head (22/22 checks across k8s v1.22–v1.33; tide is the merge queue, not a check).
No disagreement to record this round — both cross-critiques confirm every carried finding; the only residuals are evidence caveats that change no verdict. Reviewer C (Copilot) reviewed this head and raised no inline findings; its round-2 -v=1 crash-loop item remains closed under F6, and its latest overview asks for the same human confirmation as our one open question.
Remaining, all non-blocking: F2 — whether reconcile-time-only class-template propagation to live instances is the intended contract is still a maintainer call (documented; the /lgtm on the PR is this pipeline's own automation, not a human confirmation); F5 — the pre-existing digest+tag both-halves render (repo@sha256:...:v9), a guard or doc line away from closed, an issue candidate rather than this PR's; F7 — still no e2e case for the partial-version update path; and N3 — the event re-fires on every sync pass for a persistently un-completable version (the k8s correlator aggregates it; first-appearance-only emission would be tidier).
Next round
[MUST] (none — no open PR-introduced blocker/major; N2, the round-4 delta itself, is verified fixed)
[SHOULD] Maintainer: confirm reconcile-time-only class-template propagation is the intended contract (F2)
[SHOULD] Guard or document the pre-existing digest+tag both-halves render (F5, pre-existing — issue candidate, not this PR's)
[SHOULD] Add a dedicated e2e case for the partial-version update path (F7)
[COULD] Emit the RuntimeVersionIncomplete event only on first appearance, or accept the correlator aggregation (N3)
[Filed] (none — no tracking issues opened this round)
Verified review delta
- N2: fixed (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.
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.