Repository navigation
fix(vm-disk): refuse to clone a golden image that does not exist - #4567
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/L
A new disk on source.image turns the name into a clone of cozy-public/vm-default-images-<name> without looking it up. When that golden does not exist, because of a typo or because the opt-in vm-default-images package is not enabled, CDI records CloneWithoutSource on the DataVolume and waits forever, while the VMDisk stays Ready and the user never sees why. Fail the render instead, naming the image and cozy-public, the same way a golden whose import Failed is already refused. The golden counts as missing only when neither its DataVolume nor its PVC exists: CDI clones the PVC, so a PVC with no DataVolume is a usable source, and a DataVolume whose PVC CDI has not created yet is an import about to start. An empty lookup only means "missing" when the release namespace itself is visible; under helm template every lookup is empty and the chart must still render. Existing disks stay exempt, as their DataVolume spec is immutable. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
5cc9052 to
5d2c82c
Compare
1a5b7fb to
3d5f9f2
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM.
This refuses a source.image disk whose golden is absent, and it does so in the one way that is safe at render time. I checked the three things that make or break a guard like this, and all three hold.
A valid golden is never refused. The miss fires only when the golden's DataVolume and its PVC are both absent, so the two orders CDI can leave a half-created golden in are each safe: a DataVolume that exists before its PVC keeps not $golden false, and a PVC that outlives its DataVolume is caught by the PVC term. Go's and short-circuits, so when the DataVolume is present the PVC and Namespace lookups never run. The "DataVolume exists before CDI created its PVC" and "golden PVC that has no DataVolume" cases cover both.
A transient read is not mistaken for absence. Helm's lookup returns empty only on a 404; any other error (timeout, Forbidden, 5xx) propagates and aborts the render, so the HelmRelease fails and retries rather than rendering as if the image were gone. I confirmed this against helm.sh/helm v3.20.2 and v4.2.3 pkg/engine/lookup_func.go, which are identical here. The read is a live dynamic-client GET, not a cached informer, so a just-created golden is seen immediately.
The error is raised, not swallowed: it is a fail, which is the whole point of the change.
The existing-disk exemption still wraps the whole golden block, so an in-place upgrade of an already-created disk is never blocked, and the immutability logic below sits under the opposite if $existingDV branch, so the two never interact. helm template stays green through the release-namespace gate, and both that trick and a cross-namespace PVC lookup in cozy-public are already in tree (packages/apps/kubernetes/templates/cluster.yaml:156, packages/apps/kubernetes-nodes/templates/nodegroup.yaml:371); the renderer runs as flux cluster-admin, so the two new lookups add no RBAC surface.
Tests pass 60/60, and each term of the new conjunction is independently pinned: reverting the missing-golden branch, the Namespace gate, or the PVC term each reddens a distinct case, which I checked by mutation. Merging current origin/main into the head keeps them green.
Two smaller things, neither blocking. The Namespace gate is pinned only by the "renders without a cluster" case; a case with a cluster present but the release namespace not visible would name the gate directly. And the body still says the PR is stacked on #4392 and should be retargeted once it merges, but the base is already main and the merge-base is #4392's own commit, so that note is stale now.
| {{- $golden := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" $desiredSource.pvc.name }} | ||
| {{- if eq (dig "status" "phase" "" $golden | toString) "Failed" }} | ||
| {{- fail (printf "source.image %q: golden DataVolume %q in cozy-public failed to import (phase Failed), so a clone of it would never complete. Ask the platform administrator to check the importer pod in cozy-public, then delete that DataVolume to retry the import." .Values.source.image.name $desiredSource.pvc.name) }} | ||
| {{- else if and (not $golden) (not (lookup "v1" "PersistentVolumeClaim" "cozy-public" $desiredSource.pvc.name)) (lookup "v1" "Namespace" "" .Release.Namespace) }} |
There was a problem hiding this comment.
[MINOR] "existing disk" here means existing DataVolume, not existing release
The golden block is skipped only when $existingDV is set, i.e. when the disk's own DataVolume is present. If that DataVolume is deleted out of band while the HelmRelease lives on, and the golden is also gone, this new branch now fails the render on every reconcile instead of recreating a DataVolume that would hang Pending forever. The direction is an improvement (a stuck disk becomes a visible error), but it is a change to the behaviour of an already-installed release that neither the comment nor a test records. Worth a line in the comment, since "an existing disk is exempt" reads as "an existing release is exempt".
There was a problem hiding this comment.
Confirmed, the exemption keys on the DataVolume, not on the release. I am keeping the behaviour, since a render error is better than a recreated DataVolume stuck in Pending. The comment should say "existing DataVolume", and I will change it when we next touch these lines.
| asserts: | ||
| - notFailedTemplate: {} | ||
|
|
||
| - it: does not look the golden up for a source other than an image |
There was a problem hiding this comment.
[NIT] this case asserts notFailedTemplate, which cannot see whether the lookup ran
notFailedTemplate only proves the render did not fail; it cannot observe that the golden lookup was skipped for a non-image source. The case has teeth only incidentally: moving the guard out of the image branch would nil-deref $desiredSource.pvc.name. The title promises it checks that the golden is not looked up, which the assertion does not actually verify.
There was a problem hiding this comment.
Agreed, the case pins that an http source is never refused, and a lookup whose result goes unused would still pass it. I will rename it when we next touch the file.
2 participants
What this PR does
Stacked on #4392, so the base is
fix/vm-default-images-health-check. Retarget tomainonce #4392 merges.A new vm-disk with
source.imagenow fails to render when the golden image it names does not exist, and the error names the image andcozy-public. Before this the chart clonedcozy-public/vm-default-images-<name>without looking, and CDI waited for that PVC forever while the VMDisk stayed Ready. That happens on a typo, and on every cluster where the opt-incozystack.vm-default-imagespackage is off, so the message also says to add it tobundles.enabledPackages.The golden counts as missing only when neither its DataVolume nor its PVC exists. CDI clones the PVC, so a PVC with no DataVolume is still a usable source, and a DataVolume whose PVC is not created yet is an import about to start.
Under
helm templateevery lookup is empty, so the check runs only when the release namespace itself is visible. The same trick is already used in the kubernetes and seaweedfs charts. Existing disks are exempt the same way #4392 exempts them, because their DataVolume spec is immutable. A disk refused while the catalog is off renders by itself once the golden appears, since app HelmReleases retry on failure.Tests are in
packages/apps/vm-disk/tests/dv_golden_image_test.yaml: refusal of a missing golden, a PVC without a DataVolume, a DataVolume before its PVC, rendering with no cluster, and an existing disk whose golden was removed.Closes #4548
Screenshots
No UI change.
Downstream repositories
Release note