A Velero restore of a VMDisk or a VMInstance deletes the disk's
DataVolume and restores the PVC under its name, with no owner. The
restored release is in sync, so nothing recreates the DataVolume until
the next Helm upgrade, typically a platform update. That upgrade is
refused by CDI's admission webhook ("Destination PVC ... already
exists") and the release stays failed, followed by the VMInstance
release that looks the disk up.
CDI admits a DataVolume over an existing claim only when the
DataVolume carries cdi.kubevirt.io/allowClaimAdoption or the
DataVolumeClaimAdoption feature gate is on. The restore controller
puts the annotation on the restored PVC, where CDI never reads it, and
the CDI CR does not enable the gate. Setting it on the DataVolume
keeps the scope to vm-disk instead of every DataVolume in the cluster,
and does not depend on a gate CDI intends to remove. A claim CDI
created for this DataVolume carries its uid in createdForDataVolume
and is never treated as adoptable, so a fresh import is unaffected.
The VMDisk backup round-trip now forces an upgrade after the restore,
which is what reaches the failure, and checks that the DataVolume
adopted the restored PVC instead of replacing it.
Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Assisted-by: LLM
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 <f@lex.la>
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 <f@lex.la>
A disk with source.image clones a golden from cozy-public without
looking at it. When that golden's import has failed, CDI never
populates it, so the clone waits forever and the new disk sits pending
with nothing pointing at the image.
Fail the render instead when the golden's phase is Failed, naming the
image. A golden still importing is left alone because CDI holds the
clone until the source is populated. An existing disk is exempt: its
DataVolume spec is immutable, so failing the render cannot rescue it
and would only block its upgrades.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Assisted-by: LLM
Immediate binding for every source fixed a real bug: a standalone disk
on a WaitForFirstConsumer class has no consumer to wait for, so it never
populated. It also took away the path that is correct for the other case.
For a disk a VMInstance consumes, KubeVirt renders a temporary launcher
pod carrying the VM's own affinity and nodeSelector, so the volume binds
where the VM can actually run; with the annotation always present the CDI
worker becomes the first consumer instead and the volume lands where that
pod happened to be scheduled. On a node-pinned class the VM is then
unschedulable against a node-affinity conflict, and storageClass is
immutable, so the only exit is deleting the disk.
The two cases want opposite defaults and only the operator knows which
one a given disk is. bindImmediately defaults to true, so nothing moves
for existing disks or for anyone on `replicated`, which binds Immediate
either way. Both branches are pinned in tests, including the
existing-DataVolume path, which renders its own annotation block.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: LLM
Requesting immediate binding for every source, not just `upload`, is
deliberate: a VMDisk is a standalone object populated before any VM
consumes it, so on a WaitForFirstConsumer StorageClass a disk with no VM
attached would never populate at all.
What that costs was recorded nowhere an operator reads. On a node-pinned
class the CDI worker pod chooses the node, so the volume binds where the
importer landed rather than where the VM will run, and a VM constrained
elsewhere -- a nodeSelector, a request only one node satisfies, or the
Windows affinity from _cluster.scheduling.dedicatedNodesForWindowsVMs --
stays unschedulable against a node-affinity conflict. One disk is enough
for that; it is not a multi-disk-only case. storageClass is immutable,
so the exit is delete and recreate. The chart default `replicated` binds
Immediate regardless and does not move.
Also pinned in tests, in both directions the annotation is emitted: per
source type (blank, image, disk) so a regression to the old upload-only
condition reddens, and on the existing-DataVolume path, where `helm
upgrade` retrofits the annotation onto disks created before it was
unconditional -- inert for a volume that has already bound, and now a
recorded decision rather than an unasserted side effect.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: LLM
A VMDisk is a standalone object that is populated before any VM consumes
it, so on a WaitForFirstConsumer StorageClass CDI needs an explicit
signal to schedule its worker and start provisioning. Without it the
DataVolume sits in PendingPopulation indefinitely and the disk is never
filled -- and nothing says so.
This is a consistency fix rather than a new behaviour. The chart default
`replicated` is volumeBindingMode: Immediate, so a disk on it already
bound and populated at creation; `local`, the cluster's default class,
is WaitForFirstConsumer, so the same VMDisk silently did nothing there.
The annotation was previously emitted only for source: upload, which is
the one source that could not work without it, leaving http, image, disk
and blank to differ by class.
Two consequences worth knowing. CDI checks the annotation by presence
and ignores its value, so "" and "true" are equivalent and this is not a
behaviour change on the upload path. And on a node-pinned class each
disk now binds at its own creation rather than at VM scheduling, so a
multi-disk VM on `local` can find its disks on different nodes -- that
is the existing limitation of a node-pinned class, which is why the
chart default is not one, but it is reached earlier now.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Editing a VMDisk's storageClass or source after its DataVolume already
exists was silently ignored: dv.yaml reused the existing DataVolume.spec
verbatim (DataVolume.spec is immutable in CDI), so the edit rendered as a
no-op while the HelmRelease reported success and the CR diverged from the
cluster.
When a DataVolume already exists, fail with a clear message if the
requested storageClass or source differs from what the disk was created
with. Both compare the operator's input recorded in an annotation at
creation (vm-disk.cozystack.io/storage-class and .../source), written only
at creation and preserved verbatim afterwards — never overwritten with an
unapplied edit. Recording the input rather than reading the live object
matters on both axes: the image-clone PVC name carries a chart-version
prefix (vm-image- was renamed to vm-default-images-, commit bec35e3a) that
drifts across upgrades with no edit, and the live PVC storageClassName of an
adopted or foreign DataVolume may legitimately differ from the CR default
without being a change. The annotation key presence (not its value) marks a recorded class, so a disk
created with the cluster default "" is recorded and still caught if later
changed to a concrete class.
Disks with no annotation (created earlier, or out-of-band) are handled
conservatively so a working release never deadlocks: source is compared
against the stored spec.source only when that stored source is a shape the
chart itself produces, so an adopted DataVolume using spec.sourceRef or a
non-chart source (registry, snapshot, http+secretRef, ...) is left alone
rather than deadlocked. The image-clone exemption fires only when the stored
source is a cozy-public pvc AND the request is itself an image, so a change
away from an image source to another type is still caught. storageClass has
no safe live fallback, so an un-annotated disk is not guarded here (apiserver
CEL, #2657, is the eventual guard). An unrecognized source type
now fails explicitly. Storage growth is unaffected and still handled by
pvc-resize-hook.yaml.
Mark source as immutable in values.yaml so the generated schema carries
the same x-kubernetes-validations rule already present on storageClass,
and document the contract, the exemption, and the source-annotation schema
coupling in docs/storage-immutability.md and the chart README.
Add helm-unittest coverage for the immutability guard (rejection incl.
clearing storageClass to empty and changing a non-image disk to image,
no-op reuse with annotations preserved, legacy image-disk drift, annotation
non-poisoning, un-annotated storageClass skip, adopted default-class disk,
unknown source) and initial-creation source rendering.
Fixes#2985
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
The vm-disk pvc-resize, cozystack-api, seaweedfs and vm-instance update
hooks (whose alpine/k8s image this PR refreshes) ran as root with no
securityContext. Add a pod-level runAsNonRoot + runAsUser 65534 +
seccompProfile RuntimeDefault and a container-level
allowPrivilegeEscalation: false + capabilities drop ALL, mirroring the
dashboard adopt hook. kubectl needs no root, so this is behavior-neutral.
Each hook is lookup-gated; pin the securityContext in a per-chart
helm-unittest that mocks the gating object (DaemonSet / PVC / Service) via
kubernetesProvider so the guard renders without a cluster. vm-disk and
vm-instance had no Makefile test target, so add 'test: helm unittest .' to
both so CI runs the new suites.
Scope note: other kubectl hooks on clastix/kubectl (mariadb, keycloak,
kubernetes, tenant) are untouched here — they are not part of this
alpine/k8s refresh and belong to a separate hardening pass.
Also fix the vm-disk resize hook's backoffLimit: it was nested under the
pod spec (silently ignored, leaving the Job at the default 6 retries)
instead of Job.spec. Move it to Job.spec and pin it in the test.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Mattia Eleuteri