Repository navigation
fix(vm-disk)!: refuse a golden-image clone CDI would leave unbound - #4796
Aleksei Sviridkin (lexfrei) wants to merge 1 commit into
Conversation
github-actions
Bot
added
area/virtualization
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change adds a render-time refusal for golden-image clones that CDI would leave unbound. It exempts existing disks and renders when lookup data is missing. No actionable merge-blocking risk is evident from the supplied context. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #2908 requires vm-disk to warn or refuse when a new golden-image clone targets a StorageClass different from the golden image's class. The template refuses only when CDI selects an advanced clon… |
For a new golden-image disk with known, different source and target StorageClasses, warn or fail independently of the CDI fallback predicates. Add or update Helm tests for all supported cross-class cases. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Out of Scope Changes check The template guard, Helm tests, README update, and release-pin comments support the golden-image cross-StorageClass behavior in issue #2908. The existing-disk and unavailable-lookup handling supports …Docstring Coverage No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: vm-disk now refuses golden-image clones that CDI could leave unbound.
Full details: Linked Issues check
-
Explanation
Issue
#2908requires vm-disk to warn or refuse when a new golden-image clone targets a StorageClass different from the golden image's class. The template refuses only when CDI selects an advanced clone, the source and target use the same provisioner and volume mode, the target allows expansion, and a LINSTOR target lacks remote access. It still renders known cross-class cases that CDI would host-copy, including different provisioners, different volume modes, and classes without expansion. The tests confirm these exceptions.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
-
🧪 Generate unit tests (beta)
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
bfe411e to
fa78367
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The refusal mirrors CDI's planner correctly. Two things keep it from merging: it blocks a working cross-class clone with no per-disk way out, and its "existing disk is exempt" assumption holds only while the DataVolume is present.
Findings
- [MAJOR]
packages/apps/vm-disk/templates/dv.yaml:107, refuses a working cross-class clone with no per-disk opt-out
- [MAJOR]
packages/apps/vm-disk/templates/dv.yaml:44, existing-disk exemption keyed only on the DataVolume, not the PVC
- [MINOR]
packages/apps/vm-disk/templates/dv.yaml:94, false pass when the common snapshot class object is missing
- [MINOR]
packages/apps/vm-disk/templates/dv.yaml:104, target volume mode read from an unpinned list element
- [MINOR]
packages/apps/vm-disk/templates/dv.yaml:65, guard mirrors CDI planner internals with nothing tying the two to a version
- [NIT]
packages/apps/vm-disk/templates/dv.yaml:106, provisioner read from the StorageClass, not the source PV
Caveats
- Breaking change is partly justified: the node-local case (a
localclass,allowRemoteVolumeAccess: "false") genuinely hangs today, and a loud render error beats a silent forever-pending PVC. The two MAJORs are about the refusal being wider than that case and the exemption being narrower than the real "existing disk" state. - The "clone actually completes on two remote-accessible classes" half of the first MAJOR is a live-cluster property not exercised here (hermetic review); the StorageClass parameters and the author's own note are the basis. A cluster test cloning
replicatedtoreplicated-asyncwould settle it. - Author-acknowledged follow-ups, not graded: a non-expandable target of the same provisioner at equal size still hangs and is not refused; several default classes compare nothing (CDI picks the newest);
source.diskcross-class clones are not covered.
Recommended follow-ups
- Ship the per-disk opt-out the body names, then the refusal can stay strict.
| {{- $targetMode = dig "volumeMode" "Filesystem" (first $propertySets) | toString }} | ||
| {{- end }} | ||
| {{- if and $advanced (eq $goldenProvisioner (dig "provisioner" "" $targetClass | toString)) (eq $targetMode (dig "spec" "volumeMode" "Filesystem" $goldenPVC | toString)) (dig "allowVolumeExpansion" false $targetClass) }} | ||
| {{- fail (printf "source.image %q: golden image %q lives in StorageClass %q, and a clone into StorageClass %q of the same provisioner gets no host-assisted fallback from CDI and may never bind. Set storageClass to %q, or ask the platform administrator to set cloneStrategyOverride to copy in the kubevirt-cdi package." .Values.source.image.name $desiredSource.pvc.name $goldenSC $targetSC $goldenSC) }} |
There was a problem hiding this comment.
[MAJOR] refuses a working cross-class clone with no per-disk opt-out
The guard fires on any target StorageClass of the same provisioner with the same volume mode and expansion, so a golden in replicated cloned into a second remote-accessible LINSTOR class (replicated-async, replicated-nvme) is refused. It never reads allowRemoteVolumeAccess, the one parameter that separates node-local local (line 95, false, where the clone really hangs) from a remote-accessible class (line 115, true, where it binds):
$ grep -c allowRemoteVolumeAccess packages/apps/vm-disk/templates/dv.yaml
0
$ grep -n allowRemoteVolumeAccess hack/e2e-post-install-prep.sh
95: linstor.csi.linbit.com/allowRemoteVolumeAccess: "false"
115: linstor.csi.linbit.com/allowRemoteVolumeAccess: "true"
replicated-async is a shipped class (changelogs v1.3.1, v1.4.0) and v1.6.0 propagates remote-accessible classes to tenants, so this is a normal layout. The PR body itself says such a pair "needs an explicit opt-out", but none ships, and the only escape the error offers is cluster-wide cloneStrategyOverride: copy, which degrades every clone on the platform including same-class ones. Ship a per-disk acknowledge-and-proceed before refusing, or narrow the guard to the node-local case it is actually about.
There was a problem hiding this comment.
I narrowed the guard rather than adding an opt-out. It now refuses only a LINSTOR target whose remote access isn't "from anywhere", read the way linstor-csi v1.11.2 reads it: the key with or without the linstor.csi.linbit.com/ prefix, the value through strconv.ParseBool, and the layer list when the parameter is absent. The mechanism is in linstor-csi's placement step. The clone lands where the source data is, and the scheduler stops early only if one of those nodes is in the requisite set, which this policy decides. So replicated to replicated-async renders now, and other drivers are no longer refused for this.
Two cases may still be refused without need. One is a class without remote access but with Immediate binding, where the requisite set should cover every node; I haven't checked that against external-provisioner. The other is a clone placed on a node that already holds a replica of the golden. Anyone who hits either can use the golden's class or cloneStrategyOverride: copy.
| class, else the Kubernetes default class. CDI breaks a tie between | ||
| several defaults by creation time; that is not reproduced, so a | ||
| tie compares nothing. */}} | ||
| {{- $goldenSC := dig "spec" "storageClassName" "" ($goldenPVC | default dict) | toString }} |
There was a problem hiding this comment.
[MAJOR] existing-disk exemption keyed only on the DataVolume, not the PVC
The new-disk guard block runs under if not $existingDV, and the only existence check is the disk's DataVolume:
$ grep -n 'existingDV :=' packages/apps/vm-disk/templates/dv.yaml
1:{{- $existingDV := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" .Release.Namespace .Release.Name }}
CDI can garbage-collect a completed DataVolume (dataVolumeTTLSeconds), and a PVC can be restored from backup or the DV deleted by hand, leaving a populated PVC with no DataVolume. In that state the chart treats the disk as new and refuses it on the next reconcile or platform upgrade, with no exit because storageClass is immutable. The sibling kubernetes-nodes chart handles exactly this state (tests/worker_image_golden_pvc_only_test.yaml); vm-disk does not. Gate the exemption on the disk's PVC as well as its DataVolume.
There was a problem hiding this comment.
The exemption now checks the disk's PVC as well as its DataVolume. A PVC left behind by DataVolume GC, or restored on its own, skips the golden checks and the class guard. There are tests for both.
| {{- $commonSnapshotClass = "" }} | ||
| {{- end }} | ||
| {{- if $commonSnapshotClass }} | ||
| {{- $advanced = eq (dig "driver" "" (lookup "snapshot.storage.k8s.io/v1" "VolumeSnapshotClass" "" $commonSnapshotClass | default dict) | toString) $goldenProvisioner }} |
There was a problem hiding this comment.
[MINOR] false pass when the common snapshot class object is missing
Under the snapshot strategy, when both StorageProfiles name the same status.snapshotClass but no VolumeSnapshotClass object of that name exists, lookup ... | default dict yields driver "", $advanced stays false and the disk renders. CDI's GetVolumeSnapshotClass returns NotFound and errors the clone, so it never binds, the exact symptom this PR exists to prevent. Low probability (the status field is copied from an admin-set spec), but it is a silent pass on the failure mode.
There was a problem hiding this comment.
This case is refused now, with its own message. GetVolumeSnapshotClass gets the class by name, and the planner returns NotFound as an error before any fallback. It only fires when both sides share a driver, because without a common driver CDI falls back before the lookup.
| {{- $propertySets := dig "status" "claimPropertySets" list $profile }} | ||
| {{- $targetMode := "" }} | ||
| {{- if $propertySets }} | ||
| {{- $targetMode = dig "volumeMode" "Filesystem" (first $propertySets) | toString }} |
There was a problem hiding this comment.
[MINOR] target volume mode read from an unpinned list element
$targetMode is taken from first $propertySets. A real LINSTOR StorageProfile carries several claimPropertySets (Block first, Filesystem last), so first is load-bearing, yet mutating it to last keeps the whole suite green (32/32 pass). With last the guard compares Filesystem against the golden's Block, the volume-mode conjunct goes false, and the guard silently skips a clone it should refuse. Add a case whose target profile has more than one claim property set so this element choice is pinned.
There was a problem hiding this comment.
Added a case where the target profile lists Block first and Filesystem second. With last it goes red.
| {{- end }} | ||
| {{- end }} | ||
| {{- if and $goldenSC $targetSC (ne $goldenSC $targetSC) }} | ||
| {{- /* Mirror CDI's planner (pkg/controller/clone/planner.go at |
There was a problem hiding this comment.
[MINOR] guard mirrors CDI planner internals with nothing tying the two to a version
The refusal reimplements CDI's clone planner (strategy resolution, snapshot-class compatibility, the csi-clone provisioner/mode/expansion fallback). Verified faithful against pkg/controller/clone/planner.go at the pinned v1.66.1. The exposure is drift: a kubevirt-cdi bump that reorders or adds a fallback silently turns this into over- or under-refusing, and the unit tests encode the same assumptions so none would catch it. Leave a re-check marker on the kubevirt-cdi version bump.
There was a problem hiding this comment.
There's a re-check note next to RELEASE in packages/system/kubevirt-cdi-operator/Makefile, the line a CDI bump edits. The guard now also mirrors how linstor-csi parses its parameters, so the same note sits next to LINSTOR_CSI_VERSION in packages/system/linstor/Makefile.
| {{- if $propertySets }} | ||
| {{- $targetMode = dig "volumeMode" "Filesystem" (first $propertySets) | toString }} | ||
| {{- end }} | ||
| {{- if and $advanced (eq $goldenProvisioner (dig "provisioner" "" $targetClass | toString)) (eq $targetMode (dig "spec" "volumeMode" "Filesystem" $goldenPVC | toString)) (dig "allowVolumeExpansion" false $targetClass) }} |
There was a problem hiding this comment.
[NIT] provisioner read from the StorageClass, not the source PV
$goldenProvisioner comes from the golden StorageClass's provisioner, while CDI's GetCommonDriver reads the bound PV's spec.csi.driver. They agree for a dynamically provisioned LINSTOR golden and diverge only for a statically provisioned golden PV, where this would mis-decide. Reasoned, not reproduced.
There was a problem hiding this comment.
The golden's driver now comes from its bound PV's spec.csi.driver, with the class provisioner as the fallback. That's the same order as GetCommonDriver.
When CDI clones a golden image into a StorageClass other than the one the golden PVC lives in, its csi-clone path, and its snapshot path once it gets a VolumeSnapshotClass of the provisioner, check the two classes for a common driver, a matching volume mode and room to expand, and never compare the class names. A target class of the same provisioner therefore gets no host-assisted fallback and the clone is left to the CSI driver. LINSTOR creates that clone next to the source data, so on a target class that does not allow remote volume access from every node, such as a node-local one, the temporary clone PVC stays unbound with nothing surfaced to the user. Refuse such a disk at render time, mirroring the planner's inputs: the CDI override or the target StorageProfile's strategy, the snapshot class CDI would pick, the golden's driver as its PV records it, the target's provisioner, the volume modes and the target's expansion setting, and refuse only a LINSTOR target whose remote access policy, parsed the way linstor-csi parses it, is not plainly "from anywhere". A snapshot class both StorageProfiles name but that does not exist is refused as well, because CDI then fails to plan the clone instead of falling back. An empty storageClass is resolved the way CDI resolves it for a kubevirt DataVolume. A disk whose DataVolume or PVC already exists is exempt, since CDI can garbage-collect a completed DataVolume. Combinations that fall back to a host-assisted copy, targets reachable from any node, and anything else the lookups cannot find still render. This changes behaviour for a new disk in a LINSTOR class without remote access from every node, and for one whose StorageProfiles name a snapshot class that does not exist: both rendered before and are refused now. The way out is the golden image's class, or the copy clone strategy. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
fa78367 to
d288342
Compare
github-actions
Bot
added
size/XXL
|
IvanHunters all six points are addressed in d288342, details in the threads. With the refusal limited to LINSTOR targets without remote access from anywhere, I didn't add a per-disk opt-out. |
2 participants
What this PR does
A VM disk created from a golden image into a StorageClass other than the image's one can hang forever: its temporary clone PVC stays unbound, and the user sees nothing. With the default
csi-clonestrategy, CDI checks the source and target classes for a common provisioner, a matching volume mode and room to expand. It never compares class names. So a target class backed by the same provisioner gets no host-assisted fallback, and the clone is left to the CSI driver. LINSTOR creates it next to the source data, so on a target class that does not allow remote volume access from every node (a node-local class, for example) the PVC may never bind.The vm-disk chart now refuses such a disk at render time and tells the user which StorageClass to set. The check follows CDI's own clone planner. The effective strategy comes from the CDI override, or else from the target StorageProfile. It has to be
csi-clone, orsnapshotwhen CDI gets a VolumeSnapshotClass of the provisioner. Once the snapshot path has such a class it runs the same checks as csi-clone, and LINSTOR ships one. Both classes have to share a driver (the golden's is read from its PV, as CDI does), the volume modes have to match, and the target class has to allow expansion. On top of that the chart refuses only a LINSTOR target whoseallowRemoteVolumeAccess, read the way linstor-csi reads it, is not true; without the parameter that means a layer list with no DRBD or NVMe layer. A remote-accessible LINSTOR class and other drivers render. When the provisioner or the volume mode differs, CDI falls back to a host-assisted copy, which completes, so the disk renders as before. A target class without expansion is left alone too, see the follow-ups.The class check compares nothing when a lookup it needs returns nothing: under
helm template, before the golden PVC exists, or without the CDI resource. An emptystorageClassis compared as the class CDI would pick for a kubevirt DataVolume, which is the default virt class, otherwise the Kubernetes default class. Existing disks are exempt, because their DataVolume spec is immutable. A disk counts as existing when its DataVolume or its PVC exists, since CDI can garbage-collect a completed DataVolume. Undersnapshot, a snapshot class that both StorageProfiles name but that does not exist is refused too, for any driver, because CDI then fails to plan the clone instead of falling back.This is a breaking change: a new disk in a LINSTOR class without remote access from every node rendered before and is refused now, and so is one whose StorageProfiles name a snapshot class that does not exist. The first case can include a clone that would happen to work: on a class with
Immediatebinding, which I have not verified, or when the node the clone is placed on already holds a replica of the golden image. The way out is the golden image's class or thecopyclone strategy.How this covers the three fix directions in the issue:
cloneStrategyOverridewhen it is empty already landed on main in fix(cdi): omit the CDI clone strategy override when unset #4138: the template skips the field for an empty or null value, the values schema accepts the empty string, and the chart tests pin both. This PR does not touch it.csi-cloneas the default on purpose. Same-class clones are the common case, andcsi-cloneis the fast path for them. Changing the default is a separate decision, tracked in Decide whether csi-clone stays the platform default CDI clone strategy #4850.Follow-ups this PR does not cover:
snapshoton a cluster that does not serve the snapshot API, StorageProfiles naming a snapshot class still make CDI fail, and the chart does not look that class up.source.disk) into another class of the same provisioner can hang the same way, and nothing refuses it yet.csi-cloneshould remain the platform default now that it can be turned off: Decide whether csi-clone stays the platform default CDI clone strategy #4850.Closes #2908
Screenshots
Not a UI change.
Downstream repositories
No values, schema or ApplicationDefinition change. Re-check notes for the next CDI and linstor-csi bumps sit next to the version pins in the kubevirt-cdi-operator and linstor Makefiles. The new note in the vm-disk README reaches the website through the regular reference-page regeneration.
Release note
Summary by CodeRabbit