Skip to content

Commit fa9f09e

Browse files
lexfreilexfrei
authored
fix(vm-disk)!: refuse a golden-image clone CDI would leave unbound (#4796)
## 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-clone` strategy, 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`, or `snapshot` when 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 whose `allowRemoteVolumeAccess`, 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 empty `storageClass` is 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. Under `snapshot`, 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 `Immediate` binding, 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 the `copy` clone strategy. How this covers the three fix directions in the issue: 1. Omitting `cloneStrategyOverride` when it is empty already landed on main in #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. 2. I kept `csi-clone` as the default on purpose. Same-class clones are the common case, and `csi-clone` is the fast path for them. Changing the default is a separate decision, tracked in #4850. 3. vm-disk refuses the mismatch where the clone can hang, as described above. That is narrower than the issue proposed, which was to refuse every mismatch: every case CDI falls back on completes, and a clone into a remote-accessible LINSTOR class should complete too. The last part comes from reading the CDI and linstor-csi sources, I have not run it on a cluster. Follow-ups this PR does not cover: - When several classes claim the default, CDI picks the newest one. The chart does not reproduce that tie-break and compares nothing in that case. - The chart does not compare sizes. A target class without volume expansion is never refused for remote access, even when the disk is no larger than the golden image and CDI would attempt the clone. - Under `snapshot` on 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. - A disk cloned from another disk (`source.disk`) into another class of the same provisioner can hang the same way, and nothing refuses it yet. - Whether `csi-clone` should remain the platform default now that it can be turned off: #4850. Closes #2908 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: 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 ```release-note fix(vm-disk)!: a new disk from a golden image is now refused with a clear error when CDI would CSI-clone it into another LINSTOR StorageClass that does not allow remote volume access from every node. Before, such a disk could stay pending forever with no error. A clone that would have landed on a node already holding a replica of the golden image is refused as well; use the golden image's StorageClass or the copy clone strategy. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented image-backed disks from being rendered when CDI’s selected clone strategy cannot safely clone across differing storage classes, including applicable LINSTOR remote-access and snapshot-class checks. * Existing DataVolumes and same-namespace PVCs are not blocked by these checks. Disk creation can proceed when required information is unavailable or the selected strategy supports the operation. * **Documentation** * Clarified when image-backed disks may be refused and recommended using the golden image’s storage class or the copy strategy as alternatives. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 938abdc + d288342 commit fa9f09e

5 files changed

Lines changed: 1492 additions & 4 deletions

File tree

‎packages/apps/vm-disk/README.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ A Virtual Machine Disk
88

99
> `bindImmediately` is on by default, so a disk's volume is provisioned when the disk is created rather than deferred to the first consumer. That default exists because a VMDisk is a standalone object with a lifecycle of its own: on a `WaitForFirstConsumer` StorageClass a disk that no VM has attached yet would otherwise never populate at all. It costs something on a node-pinned class such as `local`, where the CDI worker pod becomes the first consumer and the volume lands where that pod was scheduled rather than where the VM will run, so a VM whose placement is constrained (a `nodeSelector`, a resource request only one node satisfies, or the Windows affinity applied by `_cluster.scheduling.dedicatedNodesForWindowsVMs`) can stay unschedulable against a node-affinity conflict, and `storageClass` is immutable so the only exit is deleting and recreating the disk. Turn `bindImmediately` off for a disk a VMInstance will consume on such a class: KubeVirt then renders a temporary launcher pod carrying the VM's own affinity and node selector, and the volume binds where the VM can actually run. The chart default `replicated` binds `Immediate` regardless, so neither setting changes anything there.
1010

11+
> A disk created from `source.image` should use the StorageClass its golden image lives in, `replicated` in the default layout. When CDI clones with the `csi-clone` strategy, the platform default, into a different StorageClass of the same CSI driver with the same volume mode and room to expand, it does not fall back to a host-assisted copy and leaves the clone to the driver. The `snapshot` strategy does the same when it gets a VolumeSnapshotClass of that driver: the class both StorageProfiles name, or, when they do not name the same one, any class of the 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 class, the clone may never bind. The chart refuses such a new disk when the golden's driver and the target class's provisioner are both LINSTOR and the target's `allowRemoteVolumeAccess` is not a value linstor-csi reads as true (`1`, `t` or `true`, in any of the cases Go's `strconv.ParseBool` accepts); any other value is a list of rules, and the chart counts it as access from anywhere only when one rule names no `fromSame` label. Without that parameter it reads the target's layer list the way linstor-csi does, which allows remote access when it has a DRBD or NVMe layer, and a class that sets no layer list has DRBD. Under `snapshot`, it also refuses a disk whose golden and target StorageProfiles name the same VolumeSnapshotClass when that class does not exist and the golden's driver is the target class's provisioner, whatever the driver: CDI then fails to plan the clone instead of falling back. Both refusals compare only what they can look up: without the golden image's PVC, the `cdi` resource or the target's StorageClass nothing is compared. The remote access refusal also needs the target's StorageProfile with a claim property set and the golden's driver, taken from its bound PV or else from its StorageClass; when any of them is missing, the disk renders. Neither applies to a disk whose DataVolume or PVC already exists. Use the golden image's StorageClass instead, or ask the platform administrator to set `cloneStrategyOverride: copy` in the `kubevirt-cdi` package, which clones through a host-assisted copy in every case. The remote access refusal leaves alone a target class that allows remote volume access, a driver other than LINSTOR, a target class of another driver, a different volume mode, a class without volume expansion, `snapshot` that gets no VolumeSnapshotClass of the driver, and the `copy` strategy. An empty `storageClass` is compared as the class CDI would pick for it: the class annotated `storageclass.kubevirt.io/is-default-virt-class`, otherwise the class annotated `storageclass.kubernetes.io/is-default-class`; when several classes carry the annotation that decides, nothing is compared.
12+
1113
## Parameters
1214

1315
### Common parameters

‎packages/apps/vm-disk/templates/dv.yaml‎

Lines changed: 122 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,133 @@
1919
namespace is visible: helm template has no cluster and every lookup
2020
comes back empty. An existing disk is exempt: its DataVolume spec is
2121
immutable, so failing the render cannot rescue it and would only block
22-
this release's upgrades. */}}
23-
{{- if not $existingDV }}
22+
this release's upgrades. Its PVC alone counts as existing, since CDI
23+
can garbage-collect a completed DataVolume and leave the PVC. */}}
24+
{{- if not (or $existingDV (lookup "v1" "PersistentVolumeClaim" .Release.Namespace .Release.Name)) }}
2425
{{- $golden := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" $desiredSource.pvc.name }}
26+
{{- $goldenPVC := lookup "v1" "PersistentVolumeClaim" "cozy-public" $desiredSource.pvc.name }}
2527
{{- if eq (dig "status" "phase" "" $golden | toString) "Failed" }}
2628
{{- 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) }}
27-
{{- else if and (not $golden) (not (lookup "v1" "PersistentVolumeClaim" "cozy-public" $desiredSource.pvc.name)) (lookup "v1" "Namespace" "" .Release.Namespace) }}
29+
{{- else if and (not $golden) (not $goldenPVC) (lookup "v1" "Namespace" "" .Release.Namespace) }}
2830
{{- fail (printf "source.image %q: golden image %q not found in cozy-public, so a clone of it would never start. Check the image name, or ask the platform administrator to add cozystack.vm-default-images to bundles.enabledPackages." .Values.source.image.name $desiredSource.pvc.name) }}
2931
{{- end }}
32+
{{- /* Refuse a clone into a class other than the golden's that CDI would
33+
attempt as an advanced clone (csi-clone, or snapshot with a usable
34+
snapshot class): nothing on that path compares class names,
35+
so a target class of the same provisioner gets no fallback. LINSTOR
36+
creates the clone where the source data is and then needs it
37+
reachable from the node the claim is placed on, which only a class
38+
allowing remote access from anywhere guarantees; otherwise the
39+
clone PVC may never bind. The golden PVC carries the class CDI
40+
resolved for it; without one (no cluster, or the PVC not created
41+
yet) nothing is compared. An empty storageClass is resolved the
42+
way CDI resolves it for a kubevirt DataVolume: the default virt
43+
class, else the Kubernetes default class. CDI breaks a tie between
44+
several defaults by creation time; that is not reproduced, so a
45+
tie compares nothing. */}}
46+
{{- $goldenSC := dig "spec" "storageClassName" "" ($goldenPVC | default dict) | toString }}
47+
{{- $targetSC := $requestedSC }}
48+
{{- if and $goldenSC (not $targetSC) }}
49+
{{- $classes := (lookup "storage.k8s.io/v1" "StorageClass" "" "").items | default list }}
50+
{{- $resolved := false }}
51+
{{- range $annotation := list "storageclass.kubevirt.io/is-default-virt-class" "storageclass.kubernetes.io/is-default-class" }}
52+
{{- if not $resolved }}
53+
{{- $defaults := list }}
54+
{{- range $classes }}
55+
{{- if eq (dig "metadata" "annotations" $annotation "" . | toString) "true" }}
56+
{{- $defaults = append $defaults .metadata.name }}
57+
{{- end }}
58+
{{- end }}
59+
{{- if $defaults }}
60+
{{- $resolved = true }}
61+
{{- if eq (len $defaults) 1 }}{{- $targetSC = first $defaults }}{{- end }}
62+
{{- end }}
63+
{{- end }}
64+
{{- end }}
65+
{{- end }}
66+
{{- if and $goldenSC $targetSC (ne $goldenSC $targetSC) }}
67+
{{- /* Mirror CDI's planner (pkg/controller/clone/planner.go at
68+
v1.66.1): the strategy is the CDI CR's override, else the target
69+
StorageProfile's, else snapshot. snapshot falls back to a
70+
host-assisted copy when no VolumeSnapshotClass serves the
71+
driver (the one both StorageProfiles name, else any), fails
72+
outright when the one both name does not exist, and otherwise
73+
goes on like csi-clone. csi-clone falls back on a driver or
74+
volume mode mismatch, or on a larger target in a class without
75+
expansion; sizes are not compared, so a class without expansion
76+
is left alone. A host-assisted copy completes, so every fallback
77+
renders. Anything else not found compares nothing. */}}
78+
{{- $cdi := lookup "cdi.kubevirt.io/v1beta1" "CDI" "" "cdi" }}
79+
{{- $profile := lookup "cdi.kubevirt.io/v1beta1" "StorageProfile" "" $targetSC | default dict }}
80+
{{- $goldenClass := lookup "storage.k8s.io/v1" "StorageClass" "" $goldenSC | default dict }}
81+
{{- $targetClass := lookup "storage.k8s.io/v1" "StorageClass" "" $targetSC | default dict }}
82+
{{- $strategy := "" }}
83+
{{- if $cdi }}
84+
{{- $strategy = dig "spec" "cloneStrategyOverride" "" $cdi | default (dig "status" "cloneStrategy" "snapshot" $profile) | toString }}
85+
{{- end }}
86+
{{- /* CDI reads the source's driver from its bound PV first. */}}
87+
{{- $goldenProvisioner := dig "provisioner" "" $goldenClass | toString }}
88+
{{- with dig "spec" "volumeName" "" $goldenPVC }}
89+
{{- with dig "spec" "csi" "driver" "" (lookup "v1" "PersistentVolume" "" . | default dict) }}
90+
{{- $goldenProvisioner = . | toString }}
91+
{{- end }}
92+
{{- end }}
93+
{{- $sameProvisioner := and $goldenProvisioner (eq $goldenProvisioner (dig "provisioner" "" $targetClass | toString)) }}
94+
{{- $advanced := eq $strategy "csi-clone" }}
95+
{{- /* Gated on the API being served: a lookup of a kind the cluster
96+
does not know fails the render instead of returning nothing. */}}
97+
{{- if and (eq $strategy "snapshot") (.Capabilities.APIVersions.Has "snapshot.storage.k8s.io/v1/VolumeSnapshotClass") }}
98+
{{- $goldenProfile := lookup "cdi.kubevirt.io/v1beta1" "StorageProfile" "" $goldenSC | default dict }}
99+
{{- $commonSnapshotClass := dig "status" "snapshotClass" "" $goldenProfile | toString }}
100+
{{- if ne $commonSnapshotClass (dig "status" "snapshotClass" "" $profile | toString) }}
101+
{{- $commonSnapshotClass = "" }}
102+
{{- end }}
103+
{{- if $commonSnapshotClass }}
104+
{{- $snapshotClass := lookup "snapshot.storage.k8s.io/v1" "VolumeSnapshotClass" "" $commonSnapshotClass }}
105+
{{- if and $sameProvisioner (not $snapshotClass) }}
106+
{{- fail (printf "source.image %q: VolumeSnapshotClass %q, named by the StorageProfiles of both %q and %q, does not exist, so CDI cannot plan a clone of golden image %q into StorageClass %q. Set storageClass to %q, or ask the platform administrator to fix the StorageProfiles' snapshotClass." .Values.source.image.name $commonSnapshotClass $goldenSC $targetSC $desiredSource.pvc.name $targetSC $goldenSC) }}
107+
{{- end }}
108+
{{- $advanced = eq (dig "driver" "" ($snapshotClass | default dict) | toString) $goldenProvisioner }}
109+
{{- else }}
110+
{{- range (lookup "snapshot.storage.k8s.io/v1" "VolumeSnapshotClass" "" "").items | default list }}
111+
{{- if eq (dig "driver" "" . | toString) $goldenProvisioner }}{{- $advanced = true }}{{- end }}
112+
{{- end }}
113+
{{- end }}
114+
{{- end }}
115+
{{- $propertySets := dig "status" "claimPropertySets" list $profile }}
116+
{{- $targetMode := "" }}
117+
{{- if $propertySets }}
118+
{{- $targetMode = dig "volumeMode" "Filesystem" (first $propertySets) | toString }}
119+
{{- end }}
120+
{{- /* linstor-csi takes the parameter with or without its prefix and
121+
the key in any case, parses the value with strconv.ParseBool, and
122+
without it allows remote access when the layer list (DRBD and
123+
storage by default) has a DRBD or NVMe layer. Any other value is
124+
a list of rules, and a rule naming no fromSame label allows
125+
access from anywhere. */}}
126+
{{- $remoteAccess := "" }}
127+
{{- $layerList := "drbd storage" }}
128+
{{- range $key, $value := dig "parameters" dict $targetClass }}
129+
{{- $parts := splitn "/" 2 $key }}
130+
{{- $name := $parts._0 }}
131+
{{- if $parts._1 }}
132+
{{- $name = ternary $parts._1 "" (eq $parts._0 "linstor.csi.linbit.com") }}
133+
{{- end }}
134+
{{- if eq (lower $name) "allowremotevolumeaccess" }}{{- $remoteAccess = $value | toString }}{{- end }}
135+
{{- if eq (lower $name) "layerlist" }}{{- $layerList = $value | toString }}{{- end }}
136+
{{- end }}
137+
{{- $layers := splitList " " (lower $layerList) }}
138+
{{- $remoteFromAnywhere := or (has "drbd" $layers) (has "nvme" $layers) }}
139+
{{- if $remoteAccess }}
140+
{{- $remoteFromAnywhere = has $remoteAccess (list "1" "t" "T" "TRUE" "true" "True") }}
141+
{{- range fromYamlArray $remoteAccess }}
142+
{{- if and (kindIs "map" .) (not (get . "fromSame")) }}{{- $remoteFromAnywhere = true }}{{- end }}
143+
{{- end }}
144+
{{- end }}
145+
{{- if and $advanced $sameProvisioner (eq $goldenProvisioner "linstor.csi.linbit.com") (not $remoteFromAnywhere) (eq $targetMode (dig "spec" "volumeMode" "Filesystem" $goldenPVC | toString)) (dig "allowVolumeExpansion" false $targetClass) }}
146+
{{- 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. LINSTOR creates the clone next to the source data, and StorageClass %q does not allow remote volume access from every node, so the clone 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 $targetSC $goldenSC) }}
147+
{{- end }}
148+
{{- end }}
30149
{{- end }}
31150
{{- else if hasKey .Values.source "http" }}
32151
{{- $desiredSource = dict "http" (dict "url" (required "A valid .Values.source.http.url entry required!" .Values.source.http.url)) }}

0 commit comments

Comments
 (0)