fix(kubevirt-cdi): drop global cloneStrategyOverride by IvanHunters · Pull Request #2921 · cozystack/cozystack

IvanHunters · 2026-06-15T09:41:31Z

What this PR does

packages/system/kubevirt-cdi/templates/cdi-cr.yaml pinned cloneStrategyOverride: csi-clone on the CDI CR globally (introduced with the golden-disks feature, switched from snapshot to csi-clone in 42778cf).

That override bypassed CDI's same-StorageClass guard: CDI's snapshot-based smart-clone path checks that source and target PVCs share a StorageClass and falls back to host-assisted copy otherwise, but the csi-clone path has no such guard — Kubernetes allows cross-StorageClass PVC cloning at the API level and delegates satisfiability to the CSI driver. Forced csi-clone was therefore issued as-is for every clone, including cross-class ones.

The concrete failure (#2908): a vm-disk created from a golden image (which lives in a replicated StorageClass) into a local (node-local, placement count 1) StorageClass — CDI emits a tmp-pvc-… in cozy-public with the target StorageClass and a prep-… pod, but local cannot materialise the clone on a node that does not already hold the source data. The PVC stays unbound forever, LINSTOR CSI logs context deadline exceeded, and no error is surfaced to the user.

This PR removes the global override so CDI picks the clone strategy per StorageProfile. The same-class path still uses CSI-snapshot clones (fast); the cross-class path falls back to host-assisted copy (slow but correct).

Verification

$ helm template kubevirt-cdi packages/system/kubevirt-cdi --namespace cozy-cdi \
    --set '_cluster.root-host=example.cozy.local' \
    | grep -A2 'kind: CDI'
kind: CDI
metadata:
  name: cdi
  # No cloneStrategyOverride (removed)

The CRD schema in packages/system/kubevirt-cdi-operator/templates/cdi-operator.yaml still documents the cloneStrategyOverride field — that is the CRD definition, not its value, and remains unchanged.

Performance and behavioural implications

Same-StorageClass clones (the common case for VMs with a disk in the same SC as their golden image): unchanged user-visible behaviour. CDI will still pick snapshot for any StorageProfile whose CSI driver advertises VolumeSnapshot support (e.g. LINSTOR).
Cross-StorageClass clones (the broken case): falls back to host-assisted copy. Slower than csi-clone but actually completes; without this fix it never did.
If a particular StorageClass benefits from csi-clone in the same-class path and is being defaulted to snapshot, a StorageProfile CR with cloneStrategy: csi-clone for that class would restore the previous fast path without re-introducing the cross-class bug. None are shipped today and none are required by this PR.

Release note

fix(kubevirt-cdi): remove the global cloneStrategyOverride on the CDI CR. The override forced csi-clone for every CDI clone, including cross-StorageClass ones, which has no same-class guard — vm-disks cloned from a golden image into a different StorageClass (typically a node-local one) stayed unbound forever. Without the override CDI picks the clone strategy per StorageProfile: same-StorageClass clones still use CSI snapshots (fast), cross-StorageClass clones fall back to host-assisted copy (slower but correct).

Closes #2908

The CDI CR pinned cloneStrategyOverride to csi-clone globally (introduced with the golden-disks feature, switched from snapshot to csi-clone afterwards). That override bypassed CDI's same-StorageClass guard, so a cross-StorageClass clone (e.g. a vm-disk created from a golden image in a replicated StorageClass into a node-local one) was issued as csi-clone and could never satisfy on the target driver: the temporary clone PVC stayed unbound forever and no error surfaced. Drop the override and let CDI pick the clone strategy per StorageProfile. The same-class path still goes through CSI-snapshot clones (fast); the cross-class path falls back to host-assisted copy (slow but correct). Closes #2908 Signed-off-by: IvanHunters <[email protected]>

coderabbitai · 2026-06-15T09:41:40Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 11b2332e-fa98-427a-ba7d-517af1d18f19

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/vm-disk-cross-storageclass-clone

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Aleksei Sviridkin (lexfrei)

NOT LGTM (REQUEST_CHANGES)

Business context: this drops the global cloneStrategyOverride: csi-clone on the CDI CR so the clone strategy is chosen per StorageProfile, aiming to fix vm-disks that stay unbound when a golden image is cloned into a different StorageClass (#2908).

Dropping a global override that forces one strategy for every backend is a reasonable direction. The problem is that the stated mechanism is wrong for the storage backend this repo ships, and the change is not shown to fix the reported case.

Blocker

Removing the override does not make cross-StorageClass clones fall back to host-assisted copy for the reported LINSTOR case, so the fix is unverified against #2908.

The PR body, the release note, and the new code comment (cdi-cr.yaml:6-12) all state that without the override, cross-StorageClass clones "fall back to host-assisted copy". That is not what CDI v1.64.0 does — the version this chart deploys (quay.io/kubevirt/cdi-controller:v1.64.0) — when source and target StorageClasses share one CSI driver, which is exactly the reported scenario: replicated and local are both linstor.csi.linbit.com.

What CDI actually does with the override gone:

The strategy is read from the target StorageProfile's Status.CloneStrategy. LINSTOR is not in CDI's clone-strategy advisory table, so CDI defaults the profile to snapshot because this repo ships a default VolumeSnapshotClass for the driver. Evidence: pkg/controller/clone/planner.go computeStrategyForSourcePVC (strategy default CloneStrategySnapshot, then taken from the StorageProfile); pkg/storagecapabilities/storagecapabilities.go CloneStrategyByProvisionerKey has no linstor.csi.linbit.com entry; packages/system/linstor/templates/volumesnapshotclass.yaml (linstor-snapshots, is-default-class: true).
The planner only falls back to host-assisted when source and target do not share a CSI driver, have incompatible volume modes, lack a compatible VolumeSnapshotClass, or the target cannot expand. It never compares StorageClass names. For replicated->local (same driver, same snapshot class, same volume mode) none of these fire. Evidence: pkg/controller/clone/planner.go validateAdvancedClonePVC; pkg/controller/clone/common.go GetCommonDriver and GetCompatibleVolumeSnapshotClass (both compare provisioner, not StorageClass name).

So removing the override changes the cross-class strategy from csi-clone to snapshot, not to host-assisted copy. Whether a snapshot-based clone of a golden image into a node-local (placement count 1) StorageClass actually binds is not established — it can stay unbound for the same root reason as csi-clone (the target volume is provisioned on a node that does not hold the source data). The "Verification" section only runs helm template to confirm the field is gone; it does not exercise a real replicated->local clone. Evidence: PR description "Verification" block.

Please verify the actual failing scenario with the override removed (create a vm-disk from a golden image into local and confirm it binds), and correct the description, the code comment, and the release note to match the strategy CDI actually selects. If the snapshot path also stays unbound, override removal alone does not fix #2908 and a different approach is needed — e.g. an explicit per-StorageClass cloneStrategy, or a fail-fast / warning in vm-disk when the target StorageClass differs from the golden image's (the second direction suggested in #2908).

Non-blocking

Description history is inaccurate: the body says the override was "switched from snapshot to csi-clone in 42778cf", but it was introduced as copy (d38c8aa) and 42778cf switched copy->csi-clone. snapshot was never the global value. Evidence: git show 42778cf4b and git show d38c8aa5a on packages/system/kubevirt-cdi/templates/cdi-cr.yaml.
Nothing pins the intent. This chart ships no helm-unittest, and make update re-fetches templates/cdi-cr.yaml from upstream, which would silently restore an override if upstream ever set one. A test asserting spec.cloneStrategyOverride is absent would lock the change. Evidence: packages/system/kubevirt-cdi has no tests/ directory; packages/system/kubevirt-cdi/Makefile wget ... -O templates/cdi-cr.yaml.

Aleksei Sviridkin (lexfrei) · 2026-06-24T10:48:01Z

+  # replicated StorageClass into a node-local one) were issued as csi-clone
+  # and could never satisfy on the target driver — the PVC stayed unbound.
+  # Per-StorageProfile selection lets CSI-snapshot clones stay fast for the
+  # same-class case while falling back to host-assisted copy across classes.


This is the claim that does not hold for the reported LINSTOR case. With the override gone, replicated->local (both linstor.csi.linbit.com) is resolved as snapshot, not host-assisted copy: the planner only falls back when the two StorageClasses do not share a CSI driver / snapshot class / volume mode — it never compares StorageClass names. Evidence: pkg/controller/clone/planner.go validateAdvancedClonePVC and pkg/controller/clone/common.go GetCommonDriver (provisioner-only); linstor.csi.linbit.com is absent from CloneStrategyByProvisionerKey, so the profile defaults to snapshot via the shipped default VolumeSnapshotClass. See the review summary.

github-actions · 2026-08-24T04:55:58Z

This PR has had no activity for 60 days and was marked lifecycle/stale.
It will be closed in 14 days unless commented or labelled lifecycle/frozen.

github-actions Bot added size/XS This PR changes 0-9 lines, ignoring generated files area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/bug Categorizes issue or PR as related to a bug labels Jun 15, 2026

Aleksei Sviridkin (lexfrei) requested changes Jun 24, 2026

View reviewed changes

github-actions Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale label Aug 24, 2026

Aleksei Sviridkin (lexfrei) mentioned this pull request Sep 1, 2026

fix(cdi): update CDI to v1.66.1 #3920

Merged

1 of 11 tasks

IvanHunters closed this Sep 7, 2026

Aleksei Sviridkin (lexfrei) mentioned this pull request Sep 7, 2026

vm-disk from golden image stays unbound forever when target StorageClass differs from the image's #2908

Open

Navigation Menu

Repository navigation

fix(kubevirt-cdi): drop global cloneStrategyOverride - #2921

fix(kubevirt-cdi): drop global cloneStrategyOverride#2921
IvanHunters wants to merge 1 commit into
mainfrom
fix/vm-disk-cross-storageclass-clone

IvanHunters commented Jun 15, 2026

Uh oh!

coderabbitai Bot commented Jun 15, 2026

Review skipped

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Uh oh!

Aleksei Sviridkin (lexfrei) Jun 24, 2026

Uh oh!

github-actions Bot commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

IvanHunters commented Jun 15, 2026

What this PR does

Verification

Performance and behavioural implications

Release note

Uh oh!

coderabbitai Bot commented Jun 15, 2026

Review skipped

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Blocker

Non-blocking

Uh oh!

Aleksei Sviridkin (lexfrei) Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants