fix(migration): restore the class reclaim policy on imported volumes (#4480)

## What this PR does The VM import handoff sets the transferred `PersistentVolume` to `Retain` so the data survives the swap from the Forklift-populated claim to the VMDisk's claim, and then never gives the policy back. Every imported disk therefore ends up with `persistentVolumeReclaimPolicy: Retain`, while its StorageClass says `Delete` and every VMDisk created directly carries `Delete`. When the tenant later deletes an imported VMDisk through the normal path, its DataVolume and claim go away, but the volume stays `Released` with its backing storage: an orphaned, billed volume that no tenant can see or remove. #### How it was measured On a lab cluster with a LINSTOR-backed `replicated` class (`reclaimPolicy: Delete`): a 16 GiB VM imported from vSphere produced a VMDisk whose PV was `Retain`, while every non-imported VMDisk's PV in the cluster was `Delete`. Deleting the VMInstance and then the VMDisk removed the DataVolume and the claim and left the PV `Released`, still `Retain`, with its LINSTOR resource in place. Patching that PV back to `Delete` made the CSI provisioner delete both the PV and the LINSTOR resource, which confirms the reclaim policy is the only thing keeping it. #### The fix `Retain` is now held only for the window it protects. A new step, `settleReclaimPolicy`, restores the policy the volume's StorageClass declares (`Delete` when the class names none or no longer exists) once the handoff is provably over, and until then reports "not yet" and touches nothing: - the replacement claim is `Bound` and the PV is `Bound` with a `claimRef` naming that claim by UID, as the PV controller reports it rather than as the controller wrote it, so no release is in flight; - the transferred claim is gone; - the claim's one and only owner is the VMDisk's DataVolume, as a controller reference whose UID matches the live DataVolume (CDI's adoption), so the only deletion that can reach the volume is the VMDisk's own; - the VMDisk exists and carries this task's output markers. `fulfill` runs this step after adopting every disk and before creating the VMInstance and deleting the scaffolding VM, so a VM is reported `Succeeded` only once its volumes carry their class's policy, and an import that fails after this point still leaves disks that clean up like any other. The step is a compare-and-set, so repeated passes and restarts are no-ops once settled. It also runs on the path that recovers outputs whose status was never written, for tasks caught mid-handoff by an upgrade. The comment that called permanent `Retain` "part of the contract" and the matching paragraph in `packages/system/migration-controller/README.md` are updated; the README also says how to fix volumes left on `Retain` by imports completed before this change, since those tasks are terminal and are not reconciled again. In practice a VM now stays in `Creating` for one more requeue (about 20 s) after the swap, while the PV controller rebinds and CDI adopts the claim. #### Tests - `TestSettleRestoresTheClassReclaimPolicy`: a finished handoff gets the class's policy back: `Delete`, `Retain` when the class asks for it, and `Delete` when the class names none or is gone. - `TestSettleIsIdempotent`: a second pass over a settled volume reports settled and does not rewrite the PV. - `TestSettleKeepsRetainWhileTheHandoffIsOpen`: thirteen cases, each breaking one condition (claim not created by this import, claim not bound, PV still `Released`, `claimRef` still on the old claim, `claimRef` on the new claim with a stale UID, transferred claim still present, claim not yet adopted by CDI, an extra owner, a DataVolume owner that is not the controller, an owner reference to a previous DataVolume, DataVolume missing, VMDisk missing, VMDisk not created by this import). Every one keeps `Retain`. - `TestFulfillRestoresTheReclaimPolicyBeforeFinishing`: behavioural regression over `fulfill`. The first pass swaps the volume, keeps `Retain`, creates no VMInstance and keeps the scaffolding VM. After simulating the PV controller and CDI, the second pass restores `Delete`, creates the VMInstance and removes the scaffolding VM. With the new step turned into a no-op, this test fails. - `TestFulfillSettlesReclaimedOutputs`: the recovery path for outputs whose status was never written. With the handoff still open the volume keeps `Retain`; once it is finished the policy goes back to `Delete`. - The existing `adoptVolume` tests still assert `Retain` during the swap. `go test ./internal/migrationcontroller/...` and `make test-controllers` pass, `go vet` is clean, and `make generate` leaves the tree unchanged. ### Screenshots  ### 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: The diff touches only `internal/migrationcontroller/`, `packages/system/migration-controller/README.md` and a comment in `packages/system/migration-controller/templates/rbac.yaml`, which match no entry of the trigger map; the website's VMware migration page does not describe the reclaim policy, so it needs no change. ### Release note  ```release-note fix(migration): disks imported from VMware no longer keep a Retain reclaim policy after the import. The volume gets its StorageClass policy back once it is bound to its VMDisk, so deleting an imported VMDisk deletes its volume instead of leaving an orphaned Released PersistentVolume. Volumes imported before this fix keep Retain and have to be patched back by hand. ```  ## Summary by CodeRabbit * **Bug Fixes** * Imported volumes are protected during claim transfers, then returned to their StorageClass reclaim policy after the replacement claim and volume are safely adopted. If the class or policy is unavailable, the policy defaults to `Delete`. * VM imports wait for volume adoption and reclaim-policy settlement before completing, helping prevent premature volume deletion and incomplete imports. Previously unrecorded handoff outputs are also settled before being returned. * **Documentation** * Clarified reclaim-policy behavior during imports and how to restore cleanup behavior for volumes from older imports.
2026-10-23 03:27:50 +09:00 · 2026-09-29 14:41:04 +03:00
commit d9ed566326
@@ -9,6 +9,7 @@ import (
 	"sort"

 	corev1 "k8s.io/api/core/v1"
+	storagev1 "k8s.io/api/storage/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -94,6 +95,12 @@ func (r *VMImportTaskReconciler) fulfill(
 			return nil, err
 		}
 		if reclaimed != nil {
+			// A controller predating settleReclaimPolicy deleted the VM before
+			// settling anything, so these volumes may still be on Retain.
+			settled, err := r.settleReclaimPolicies(ctx, task, req.ID, reclaimed.Disks, nil)
+			if err != nil || !settled {
+				return nil, err
+			}
 			logger.Info("reclaimed outputs recorded by no status", "vm", req.ID, "instance", reclaimed.VMInstance)
 			return reclaimed, nil
 		}
@@ -124,9 +131,15 @@ func (r *VMImportTaskReconciler) fulfill(
 			return nil, err
 		}
 	}
+	settled, err := r.settleReclaimPolicies(ctx, task, req.ID, diskNames, claims)
+	if err != nil || !settled {
+		return nil, err
+	}

-	// The VMInstance is created only once every disk is in place, so a tenant
-	// never sees an instance referencing a disk that does not exist yet.
+	// The VMInstance is created only once every disk is in place, its volume
+	// policy included, so a tenant never sees an instance referencing a disk
+	// that does not exist yet, and an import failing past this point leaves
+	// disks that clean up after themselves like any other.
 	if err := r.createVMInstance(ctx, task, req, name, vm, diskNames); err != nil {
 		return nil, err
 	}
@@ -387,10 +400,11 @@ func (r *VMImportTaskReconciler) adoptVolume(
 		return err
 	}

-	// Retain is what protects the data during the swap, and it stays on
-	// permanently: CDI takes a controller owner reference on an adopted claim,
-	// so deleting the DataVolume garbage-collects the claim, and only the
-	// reclaim policy keeps the disk. That policy is part of the contract.
+	// Retain carries the data through the swap: from the moment the
+	// transferred claim is deleted until claimRef names its replacement, the
+	// volume is Released, and under Delete the provisioner reclaims it on the
+	// spot. It is held for that window only: settleReclaimPolicy hands back
+	// the class's own policy once the volume answers to its VMDisk alone.
 	if pv.Spec.PersistentVolumeReclaimPolicy != corev1.PersistentVolumeReclaimRetain {
 		pv.Spec.PersistentVolumeReclaimPolicy = corev1.PersistentVolumeReclaimRetain
 		if err := r.Update(ctx, pv); err != nil {
@@ -470,6 +484,157 @@ func (r *VMImportTaskReconciler) adoptVolume(
 	return r.ensureVMDisk(ctx, task, vmID, diskName, bound)
 }

+// settleReclaimPolicies settles every disk of one source VM, and reports
+// whether all of them are done. sourceClaims, when known, is index-aligned
+// with diskNames.
+func (r *VMImportTaskReconciler) settleReclaimPolicies(
+	ctx context.Context,
+	task *migrationv1alpha1.VMImportTask,
+	vmID string,
+	diskNames []string,
+	sourceClaims []string,
+) (bool, error) {
+	all := true
+	for i, diskName := range diskNames {
+		source := ""
+		if i < len(sourceClaims) {
+			source = sourceClaims[i]
+		}
+		settled, err := r.settleReclaimPolicy(ctx, task, vmID, diskName, source)
+		if err != nil {
+			return false, err
+		}
+		all = all && settled
+	}
+	return all, nil
+}
+
+// settleReclaimPolicy gives an adopted volume back the reclaim policy its
+// StorageClass declares, once nothing but its VMDisk can release it.
+//
+// Past the swap, Retain protects nothing and leaks a volume per imported disk:
+// deleting the VMDisk removes its DataVolume, garbage collection takes the
+// claim through the owner reference CDI set when it adopted it, and the volume
+// is left Released with its backing storage, where no tenant can see or delete
+// it. Every disk a tenant creates directly carries its class's policy, and an
+// imported one has to end up the same.
+//
+// Until the handoff is provably over this reports false and touches nothing.
+// Over means: the claim and the volume are each Bound to the other, as the PV
+// controller reports it rather than as claimRef was written, so no release is
+// still in flight; the transferred claim is gone; and the claim's one owner is
+// the DataVolume of the VMDisk release, so the only deletion that can reach
+// the volume is the VMDisk's own.
+func (r *VMImportTaskReconciler) settleReclaimPolicy(
+	ctx context.Context,
+	task *migrationv1alpha1.VMImportTask,
+	vmID string,
+	diskName string,
+	sourceClaim string,
+) (bool, error) {
+	target := vmDiskReleasePrefix + diskName
+	waiting := func(what string) (bool, error) {
+		log.FromContext(ctx).V(1).Info("imported volume not settled yet", "disk", diskName, "waitingFor", what)
+		return false, nil
+	}
+
+	claim := &corev1.PersistentVolumeClaim{}
+	if err := r.Get(ctx, types.NamespacedName{Namespace: task.Namespace, Name: target}, claim); err != nil {
+		if apierrors.IsNotFound(err) {
+			return waiting("the replacement claim")
+		}
+		return false, err
+	}
+	if !isOwnOutput(claim, task, vmID) {
+		return waiting("a claim created by this import")
+	}
+	if claim.Status.Phase != corev1.ClaimBound || claim.Spec.VolumeName == "" {
+		return waiting("the replacement claim to bind")
+	}
+
+	pv := &corev1.PersistentVolume{}
+	if err := r.Get(ctx, types.NamespacedName{Name: claim.Spec.VolumeName}, pv); err != nil {
+		if apierrors.IsNotFound(err) {
+			return waiting("the volume")
+		}
+		return false, err
+	}
+	ref := pv.Spec.ClaimRef
+	if pv.Status.Phase != corev1.VolumeBound || ref == nil ||
+		ref.UID != claim.UID || ref.Namespace != claim.Namespace || ref.Name != claim.Name {
+		return waiting("the volume to bind to the replacement claim")
+	}
+
+	if sourceClaim != "" && sourceClaim != target {
+		old := &corev1.PersistentVolumeClaim{}
+		err := r.Get(ctx, types.NamespacedName{Namespace: task.Namespace, Name: sourceClaim}, old)
+		if err == nil {
+			return waiting("the transferred claim to be deleted")
+		}
+		if !apierrors.IsNotFound(err) {
+			return false, err
+		}
+	}
+
+	dv := newObject(dataVolumeGVK)
+	if err := r.Get(ctx, types.NamespacedName{Namespace: task.Namespace, Name: target}, dv); err != nil {
+		if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) {
+			return waiting("the VMDisk DataVolume")
+		}
+		return false, err
+	}
+	owners := claim.OwnerReferences
+	if len(owners) != 1 || owners[0].Kind != dataVolumeGVK.Kind || owners[0].Name != target ||
+		owners[0].UID != dv.GetUID() || owners[0].Controller == nil || !*owners[0].Controller {
+		return waiting("CDI to adopt the claim into the VMDisk DataVolume")
+	}
+
+	disk := newObject(vmDiskGVK)
+	if err := r.Get(ctx, types.NamespacedName{Namespace: task.Namespace, Name: diskName}, disk); err != nil {
+		if apierrors.IsNotFound(err) {
+			return waiting("the VMDisk")
+		}
+		return false, err
+	}
+	if !isOwnOutput(disk, task, vmID) {
+		return waiting("a VMDisk created by this import")
+	}
+
+	policy, err := r.classReclaimPolicy(ctx, pv.Spec.StorageClassName)
+	if err != nil {
+		return false, err
+	}
+	if pv.Spec.PersistentVolumeReclaimPolicy != policy {
+		pv.Spec.PersistentVolumeReclaimPolicy = policy
+		if err := r.Update(ctx, pv); err != nil {
+			return false, err
+		}
+		log.FromContext(ctx).Info("restored the reclaim policy of an imported volume",
+			"disk", diskName, "volume", pv.Name, "policy", policy)
+	}
+	return true, nil
+}
+
+// classReclaimPolicy is the policy a volume of this class would have been
+// provisioned with. A class that is gone, or that names none, gets Delete:
+// the API's own default for a dynamically provisioned volume.
+func (r *VMImportTaskReconciler) classReclaimPolicy(ctx context.Context, className string) (corev1.PersistentVolumeReclaimPolicy, error) {
+	if className == "" {
+		return corev1.PersistentVolumeReclaimDelete, nil
+	}
+	sc := &storagev1.StorageClass{}
+	if err := r.Get(ctx, types.NamespacedName{Name: className}, sc); err != nil {
+		if apierrors.IsNotFound(err) {
+			return corev1.PersistentVolumeReclaimDelete, nil
+		}
+		return "", err
+	}
+	if sc.ReclaimPolicy == nil || *sc.ReclaimPolicy == "" {
+		return corev1.PersistentVolumeReclaimDelete, nil
+	}
+	return *sc.ReclaimPolicy, nil
+}
+
 // deleteOwningDataVolume removes the DataVolume that owns a claim, and waits
 // for it to actually be gone before the caller touches the claim.
 func (r *VMImportTaskReconciler) deleteOwningDataVolume(ctx context.Context, namespace, claim string) error {
@@ -0,0 +1,505 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package migrationcontroller
+
+import (
+	"context"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	storagev1 "k8s.io/api/storage/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	migrationv1alpha1 "github.com/cozystack/cozystack/api/migration/v1alpha1"
+)
+
+const (
+	settleNS     = "tenant-foo"
+	settleDisk   = "web-01-disk-0"
+	settleTarget = vmDiskReleasePrefix + settleDisk
+	settlePV     = "pv-1"
+	settleDVUID  = types.UID("vmdisk-dv-uid")
+	settleClaimU = types.UID("replacement-claim-uid")
+)
+
+func classWithPolicy(name string, policy *corev1.PersistentVolumeReclaimPolicy) *storagev1.StorageClass {
+	sc := storageClass(name, storagev1.VolumeBindingImmediate, false)
+	sc.ReclaimPolicy = policy
+	return sc
+}
+
+// settledHandoff is the cluster once the swap is over: the replacement claim
+// and the volume are bound to each other, CDI has adopted the claim into the
+// VMDisk's DataVolume, and the VMDisk exists. Only the reclaim policy is still
+// the one the swap needed.
+func settledHandoff(tk *migrationv1alpha1.VMImportTask) (*corev1.PersistentVolume, *corev1.PersistentVolumeClaim, *unstructured.Unstructured, *unstructured.Unstructured) {
+	controller := true
+	claim := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      settleTarget,
+			Namespace: settleNS,
+			UID:       settleClaimU,
+			Labels:    outputMarkers(tk, "vm-1"),
+			OwnerReferences: []metav1.OwnerReference{{
+				APIVersion: "cdi.kubevirt.io/v1beta1",
+				Kind:       "DataVolume",
+				Name:       settleTarget,
+				UID:        settleDVUID,
+				Controller: &controller,
+			}},
+		},
+		Spec:   corev1.PersistentVolumeClaimSpec{VolumeName: settlePV},
+		Status: corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound},
+	}
+	pv := &corev1.PersistentVolume{
+		ObjectMeta: metav1.ObjectMeta{Name: settlePV},
+		Spec: corev1.PersistentVolumeSpec{
+			StorageClassName:              "replicated",
+			PersistentVolumeReclaimPolicy: corev1.PersistentVolumeReclaimRetain,
+			ClaimRef: &corev1.ObjectReference{
+				Kind: "PersistentVolumeClaim", APIVersion: "v1",
+				Namespace: settleNS, Name: settleTarget, UID: settleClaimU,
+			},
+		},
+		Status: corev1.PersistentVolumeStatus{Phase: corev1.VolumeBound},
+	}
+	dv := newObject(dataVolumeGVK)
+	dv.SetName(settleTarget)
+	dv.SetNamespace(settleNS)
+	dv.SetUID(settleDVUID)
+
+	disk := newObject(vmDiskGVK)
+	disk.SetName(settleDisk)
+	disk.SetNamespace(settleNS)
+	stampOutput(disk, tk, "vm-1")
+	return pv, claim, dv, disk
+}
+
+func reclaimPolicyOf(t *testing.T, c client.Client) corev1.PersistentVolumeReclaimPolicy {
+	t.Helper()
+	pv := &corev1.PersistentVolume{}
+	if err := c.Get(context.Background(), types.NamespacedName{Name: settlePV}, pv); err != nil {
+		t.Fatalf("get pv: %v", err)
+	}
+	return pv.Spec.PersistentVolumeReclaimPolicy
+}
+
+// The leak this closes: an imported disk kept Retain for good, so deleting its
+// VMDisk left the volume Released with its backing storage, billed and
+// invisible to the tenant. Once the handoff is over the volume must carry what
+// its class says, like any disk created directly.
+func TestSettleRestoresTheClassReclaimPolicy(t *testing.T) {
+	deletePolicy := corev1.PersistentVolumeReclaimDelete
+	retainPolicy := corev1.PersistentVolumeReclaimRetain
+	cases := []struct {
+		name  string
+		class *storagev1.StorageClass
+		want  corev1.PersistentVolumeReclaimPolicy
+	}{
+		{"class says Delete", classWithPolicy("replicated", &deletePolicy), corev1.PersistentVolumeReclaimDelete},
+		// A class that asks for Retain keeps it: the policy restored is the
+		// class's, not an opinion of this controller.
+		{"class says Retain", classWithPolicy("replicated", &retainPolicy), corev1.PersistentVolumeReclaimRetain},
+		{"class names no policy", classWithPolicy("replicated", nil), corev1.PersistentVolumeReclaimDelete},
+		{"class is gone", nil, corev1.PersistentVolumeReclaimDelete},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			s := testScheme(t)
+			tk := task("import", settleNS, "vcenter", "replicated")
+			pv, claim, dv, disk := settledHandoff(tk)
+			objs := []client.Object{tk, pv, claim, dv, disk}
+			if tc.class != nil {
+				objs = append(objs, tc.class)
+			}
+			c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(objs...).Build()
+			r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
+
+			settled, err := r.settleReclaimPolicy(context.Background(), tk, "vm-1", settleDisk, "forklift-claim")
+			if err != nil {
+				t.Fatalf("settleReclaimPolicy: %v", err)
+			}
+			if !settled {
+				t.Fatal("a finished handoff was reported as unsettled; the task would never complete")
+			}
+			if got := reclaimPolicyOf(t, c); got != tc.want {
+				t.Errorf("reclaim policy = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+// The controller is re-entered on every requeue and after every restart; a
+// settled volume must not be rewritten on each pass.
+func TestSettleIsIdempotent(t *testing.T) {
+	s := testScheme(t)
+	tk := task("import", settleNS, "vcenter", "replicated")
+	pv, claim, dv, disk := settledHandoff(tk)
+	deletePolicy := corev1.PersistentVolumeReclaimDelete
+	c := clientfake.NewClientBuilder().WithScheme(s).
+		WithObjects(tk, pv, claim, dv, disk, classWithPolicy("replicated", &deletePolicy)).Build()
+	r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
+
+	if _, err := r.settleReclaimPolicy(context.Background(), tk, "vm-1", settleDisk, ""); err != nil {
+		t.Fatalf("first pass: %v", err)
+	}
+	first := &corev1.PersistentVolume{}
+	if err := c.Get(context.Background(), types.NamespacedName{Name: settlePV}, first); err != nil {
+		t.Fatalf("get pv: %v", err)
+	}
+
+	settled, err := r.settleReclaimPolicy(context.Background(), tk, "vm-1", settleDisk, "")
+	if err != nil {
+		t.Fatalf("second pass: %v", err)
+	}
+	if !settled {
+		t.Error("a volume already settled was reported as unsettled on the next pass")
+	}
+	second := &corev1.PersistentVolume{}
+	if err := c.Get(context.Background(), types.NamespacedName{Name: settlePV}, second); err != nil {
+		t.Fatalf("get pv: %v", err)
+	}
+	if second.ResourceVersion != first.ResourceVersion {
+		t.Error("the second pass rewrote a volume that was already settled")
+	}
+}
+
+// Every state in which something other than the VMDisk could still release the
+// volume must keep Retain. Each case breaks exactly one condition of a finished
+// handoff; restoring Delete in any of them is the data loss Retain exists for.
+func TestSettleKeepsRetainWhileTheHandoffIsOpen(t *testing.T) {
+	cases := []struct {
+		name       string
+		mutate     func(pv *corev1.PersistentVolume, claim *corev1.PersistentVolumeClaim) []client.Object
+		mutateDisk func(disk *unstructured.Unstructured)
+		skipDV     bool
+		skipDisk   bool
+	}{
+		{
+			// A claim of the right name that this import did not create is a
+			// tenant's disk; its volume's policy is not this controller's to
+			// rewrite, whatever state the rest of the handoff is in.
+			name: "replacement claim not created by this import",
+			mutate: func(_ *corev1.PersistentVolume, claim *corev1.PersistentVolumeClaim) []client.Object {
+				claim.Labels = outputMarkers(task("other", settleNS, "vcenter", "replicated"), "vm-1")
+				return nil
+			},
+		},
+		{
+			name: "replacement claim not bound yet",
+			mutate: func(_ *corev1.PersistentVolume, claim *corev1.PersistentVolumeClaim) []client.Object {
+				claim.Status.Phase = corev1.ClaimPending
+				return nil
+			},
+		},
+		{
+			// claimRef was written, but the PV controller has not caught up:
+			// a stale view of the claim is exactly when it would reclaim.
+			name: "volume still Released",
+			mutate: func(pv *corev1.PersistentVolume, _ *corev1.PersistentVolumeClaim) []client.Object {
+				pv.Status.Phase = corev1.VolumeReleased
+				return nil
+			},
+		},
+		{
+			name: "claimRef still names the transferred claim",
+			mutate: func(pv *corev1.PersistentVolume, _ *corev1.PersistentVolumeClaim) []client.Object {
+				pv.Spec.ClaimRef.Name = "forklift-claim"
+				pv.Spec.ClaimRef.UID = "forklift-claim-uid"
+				return nil
+			},
+		},
+		{
+			// The name alone proves nothing: a claim deleted and recreated
+			// under the same name is a different claim to the PV controller.
+			name: "claimRef names the replacement claim with a stale UID",
+			mutate: func(pv *corev1.PersistentVolume, _ *corev1.PersistentVolumeClaim) []client.Object {
+				pv.Spec.ClaimRef.UID = "a-previous-claim-uid"
+				return nil
+			},
+		},
+		{
+			name: "transferred claim still exists",
+			mutate: func(_ *corev1.PersistentVolume, _ *corev1.PersistentVolumeClaim) []client.Object {
+				return []client.Object{&corev1.PersistentVolumeClaim{
+					ObjectMeta: metav1.ObjectMeta{Name: "forklift-claim", Namespace: settleNS},
+					Spec:       corev1.PersistentVolumeClaimSpec{VolumeName: settlePV},
+				}}
+			},
+		},
+		{
+			name: "CDI has not adopted the claim yet",
+			mutate: func(_ *corev1.PersistentVolume, claim *corev1.PersistentVolumeClaim) []client.Object {
+				claim.OwnerReferences = nil
+				return nil
+			},
+		},
+		{
+			// An owner besides the VMDisk's DataVolume is a deletion that is
+			// not the tenant's — the scaffolding VM being the obvious one.
+			name: "claim has another owner",
+			mutate: func(_ *corev1.PersistentVolume, claim *corev1.PersistentVolumeClaim) []client.Object {
+				claim.OwnerReferences = append(claim.OwnerReferences, metav1.OwnerReference{
+					APIVersion: "kubevirt.io/v1", Kind: "VirtualMachine", Name: "test-vm", UID: "vm-uid",
+				})
+				return nil
+			},
+		},
+		{
+			// A plain owner reference does not make the DataVolume the claim's
+			// controller, so CDI has not adopted it.
+			name: "DataVolume owns the claim without controlling it",
+			mutate: func(_ *corev1.PersistentVolume, claim *corev1.PersistentVolumeClaim) []client.Object {
+				notController := false
+				claim.OwnerReferences[0].Controller = &notController
+				return nil
+			},
+		},
+		{
+			name: "owner reference points at a previous DataVolume",
+			mutate: func(_ *corev1.PersistentVolume, claim *corev1.PersistentVolumeClaim) []client.Object {
+				claim.OwnerReferences[0].UID = "an-older-dv-uid"
+				return nil
+			},
+		},
+		{
+			name:   "VMDisk DataVolume missing",
+			mutate: func(*corev1.PersistentVolume, *corev1.PersistentVolumeClaim) []client.Object { return nil },
+			skipDV: true,
+		},
+		{
+			name:     "VMDisk missing",
+			mutate:   func(*corev1.PersistentVolume, *corev1.PersistentVolumeClaim) []client.Object { return nil },
+			skipDisk: true,
+		},
+		{
+			// A VMDisk of the right name that another import or the tenant
+			// created is not the release this volume was handed to.
+			name:   "VMDisk not created by this import",
+			mutate: func(*corev1.PersistentVolume, *corev1.PersistentVolumeClaim) []client.Object { return nil },
+			mutateDisk: func(disk *unstructured.Unstructured) {
+				disk.SetLabels(outputMarkers(task("other", settleNS, "vcenter", "replicated"), "vm-1"))
+			},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			s := testScheme(t)
+			tk := task("import", settleNS, "vcenter", "replicated")
+			pv, claim, dv, disk := settledHandoff(tk)
+			extra := tc.mutate(pv, claim)
+			if tc.mutateDisk != nil {
+				tc.mutateDisk(disk)
+			}
+			deletePolicy := corev1.PersistentVolumeReclaimDelete
+			objs := []client.Object{tk, pv, claim, classWithPolicy("replicated", &deletePolicy)}
+			if !tc.skipDV {
+				objs = append(objs, dv)
+			}
+			if !tc.skipDisk {
+				objs = append(objs, disk)
+			}
+			objs = append(objs, extra...)
+			c := clientfake.NewClientBuilder().WithScheme(s).WithObjects(objs...).Build()
+			r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
+
+			settled, err := r.settleReclaimPolicy(context.Background(), tk, "vm-1", settleDisk, "forklift-claim")
+			if err != nil {
+				t.Fatalf("settleReclaimPolicy: %v", err)
+			}
+			if settled {
+				t.Error("reported settled while the handoff is still open")
+			}
+			if got := reclaimPolicyOf(t, c); got != corev1.PersistentVolumeReclaimRetain {
+				t.Errorf("reclaim policy = %q while the handoff is open, want Retain", got)
+			}
+		})
+	}
+}
+
+// End to end over fulfill: the first pass performs the swap and must leave the
+// volume on Retain, with no instance yet and the task still Creating. Once the
+// PV controller and CDI have done their part, the next pass restores the class
+// policy and only then finishes the import, so a VMDisk deleted afterwards
+// takes its volume with it.
+func TestFulfillRestoresTheReclaimPolicyBeforeFinishing(t *testing.T) {
+	ctx := context.Background()
+	s := testScheme(t)
+	req := migrationv1alpha1.VMImportRequest{ID: "vm-1", Name: "web-01"}
+	tk := task("import", settleNS, "vcenter", "replicated", req)
+
+	plan := newObject(planGVK)
+	plan.SetName(planName("import", "vm-1"))
+	plan.SetNamespace(settleNS)
+	plan.SetUID("plan-uid")
+
+	vm := newObject(virtualMachineGVK)
+	vm.SetName("test-vm")
+	vm.SetNamespace(settleNS)
+	vm.SetLabels(map[string]string{"plan": "plan-uid"})
+	if err := unstructured.SetNestedSlice(vm.Object, []interface{}{
+		map[string]interface{}{"name": "disk0", "dataVolume": map[string]interface{}{"name": "forklift-claim"}},
+	}, "spec", "template", "spec", "volumes"); err != nil {
+		t.Fatalf("build vm: %v", err)
+	}
+
+	transferred := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{Name: "forklift-claim", Namespace: settleNS},
+		Spec: corev1.PersistentVolumeClaimSpec{
+			VolumeName: settlePV,
+			Resources: corev1.VolumeResourceRequirements{
+				Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("16Gi")},
+			},
+		},
+		Status: corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound},
+	}
+	pv := &corev1.PersistentVolume{
+		ObjectMeta: metav1.ObjectMeta{Name: settlePV},
+		Spec: corev1.PersistentVolumeSpec{
+			StorageClassName:              "replicated",
+			PersistentVolumeReclaimPolicy: corev1.PersistentVolumeReclaimDelete,
+			Capacity:                      corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("16Gi")},
+			ClaimRef: &corev1.ObjectReference{
+				Kind: "PersistentVolumeClaim", APIVersion: "v1",
+				Namespace: settleNS, Name: "forklift-claim",
+			},
+		},
+		Status: corev1.PersistentVolumeStatus{Phase: corev1.VolumeBound},
+	}
+	deletePolicy := corev1.PersistentVolumeReclaimDelete
+	c := clientfake.NewClientBuilder().WithScheme(s).
+		WithObjects(tk, plan, vm, transferred, pv, classWithPolicy("replicated", &deletePolicy)).Build()
+	r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
+
+	out, err := r.fulfill(ctx, tk, &req)
+	if err != nil {
+		t.Fatalf("first pass: %v", err)
+	}
+	if out != nil {
+		t.Fatal("the first pass finished the import with the volume still on the swap's Retain")
+	}
+	if got := reclaimPolicyOf(t, c); got != corev1.PersistentVolumeReclaimRetain {
+		t.Fatalf("reclaim policy during the swap = %q, want Retain", got)
+	}
+	if err := c.Get(ctx, types.NamespacedName{Namespace: settleNS, Name: "web-01"}, newObject(vmInstanceGVK)); !apierrors.IsNotFound(err) {
+		t.Errorf("a VMInstance exists over a disk that is not settled (err %v)", err)
+	}
+	if err := c.Get(ctx, types.NamespacedName{Namespace: settleNS, Name: "test-vm"}, newObject(virtualMachineGVK)); err != nil {
+		t.Errorf("the scaffolding VM was removed before the disks settled: %v", err)
+	}
+
+	// What the PV controller and CDI do between the two passes.
+	claim := &corev1.PersistentVolumeClaim{}
+	if err := c.Get(ctx, types.NamespacedName{Namespace: settleNS, Name: settleTarget}, claim); err != nil {
+		t.Fatalf("get replacement claim: %v", err)
+	}
+	dv := newObject(dataVolumeGVK)
+	if err := c.Get(ctx, types.NamespacedName{Namespace: settleNS, Name: settleTarget}, dv); err != nil {
+		t.Fatalf("get VMDisk DataVolume: %v", err)
+	}
+	controller := true
+	claim.OwnerReferences = []metav1.OwnerReference{{
+		APIVersion: "cdi.kubevirt.io/v1beta1", Kind: "DataVolume",
+		Name: settleTarget, UID: dv.GetUID(), Controller: &controller,
+	}}
+	if err := c.Update(ctx, claim); err != nil {
+		t.Fatalf("adopt replacement claim: %v", err)
+	}
+	claim.Status.Phase = corev1.ClaimBound
+	if err := c.Status().Update(ctx, claim); err != nil {
+		t.Fatalf("bind replacement claim: %v", err)
+	}
+	gotPV := &corev1.PersistentVolume{}
+	if err := c.Get(ctx, types.NamespacedName{Name: settlePV}, gotPV); err != nil {
+		t.Fatalf("get pv: %v", err)
+	}
+	gotPV.Status.Phase = corev1.VolumeBound
+	if err := c.Status().Update(ctx, gotPV); err != nil {
+		t.Fatalf("bind pv: %v", err)
+	}
+
+	out, err = r.fulfill(ctx, tk, &req)
+	if err != nil {
+		t.Fatalf("second pass: %v", err)
+	}
+	if out == nil {
+		t.Fatal("the second pass did not finish an import whose handoff is over")
+	}
+	if got := reclaimPolicyOf(t, c); got != corev1.PersistentVolumeReclaimDelete {
+		t.Errorf("reclaim policy after the import = %q, want the class's Delete — deleting the VMDisk would leak the volume", got)
+	}
+	if err := c.Get(ctx, types.NamespacedName{Namespace: settleNS, Name: "web-01"}, newObject(vmInstanceGVK)); err != nil {
+		t.Errorf("VMInstance was not created once the disks settled: %v", err)
+	}
+	if err := c.Get(ctx, types.NamespacedName{Namespace: settleNS, Name: "test-vm"}, newObject(virtualMachineGVK)); !apierrors.IsNotFound(err) {
+		t.Errorf("the scaffolding VM survived a finished import (err %v)", err)
+	}
+}
+
+// The recovery path: a controller predating settlement deleted the scaffolding
+// VM and died before writing the status, so fulfill finds no VM and no status
+// entry, only this task's marked VMInstance and VMDisk. Those volumes may still
+// be on the swap's Retain, and recovering the outputs must settle them first,
+// or the import is recorded as finished with the leak it was meant to close.
+func TestFulfillSettlesReclaimedOutputs(t *testing.T) {
+	cases := []struct {
+		name       string
+		open       bool
+		wantOut    bool
+		wantPolicy corev1.PersistentVolumeReclaimPolicy
+	}{
+		{"handoff still open", true, false, corev1.PersistentVolumeReclaimRetain},
+		{"handoff finished", false, true, corev1.PersistentVolumeReclaimDelete},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := context.Background()
+			s := testScheme(t)
+			req := migrationv1alpha1.VMImportRequest{ID: "vm-1", Name: "web-01"}
+			tk := task("import", settleNS, "vcenter", "replicated", req)
+			// Status knows nothing and no Plan or VM is left to find.
+			tk.Status.VMs = nil
+
+			pv, claim, dv, disk := settledHandoff(tk)
+			if tc.open {
+				// CDI has not adopted the claim yet.
+				claim.OwnerReferences = nil
+			}
+			instance := newObject(vmInstanceGVK)
+			instance.SetName("web-01")
+			instance.SetNamespace(settleNS)
+			stampOutput(instance, tk, "vm-1")
+
+			deletePolicy := corev1.PersistentVolumeReclaimDelete
+			c := clientfake.NewClientBuilder().WithScheme(s).
+				WithObjects(tk, pv, claim, dv, disk, instance, classWithPolicy("replicated", &deletePolicy)).Build()
+			r := &VMImportTaskReconciler{Client: c, Scheme: s, Recorder: record.NewFakeRecorder(10)}
+
+			out, err := r.fulfill(ctx, tk, &req)
+			if err != nil {
+				t.Fatalf("fulfill: %v", err)
+			}
+			if !tc.wantOut && out != nil {
+				t.Errorf("recovered outputs %+v while their volume is still on the swap's Retain", out)
+			}
+			if tc.wantOut {
+				if out == nil {
+					t.Fatal("recovered nothing from a finished handoff; the task would never complete")
+				}
+				if out.VMInstance != "web-01" || len(out.Disks) != 1 || out.Disks[0] != settleDisk {
+					t.Errorf("outputs = %+v, want web-01 over %s", out, settleDisk)
+				}
+			}
+			if got := reclaimPolicyOf(t, c); got != tc.wantPolicy {
+				t.Errorf("reclaim policy = %q, want %q", got, tc.wantPolicy)
+			}
+		})
+	}
+}
@@ -258,9 +258,9 @@ func TestAdoptVolumeReplicatesVolumeModeAndAccessModes(t *testing.T) {
 			adopted.Annotations[populatedForAnnotation])
 	}

-	// Retain is not advice: it is the only thing standing between a routine
-	// DataVolume deletion and data loss, because CDI takes a controller owner
-	// reference on the claim it adopts.
+	// Retain is not advice during the swap: between deleting the transferred
+	// claim and re-pointing claimRef the volume is Released, and under Delete
+	// the provisioner would reclaim it. settleReclaimPolicy lifts it later.
 	gotPV := &corev1.PersistentVolume{}
 	if err := c.Get(context.Background(), types.NamespacedName{Name: "pv-1"}, gotPV); err != nil {
 		t.Fatalf("get pv: %v", err)
@@ -591,7 +591,7 @@ func TestAdoptVolumeOrphansAClaimWithNoDataVolume(t *testing.T) {
 		t.Fatalf("get pv: %v", err)
 	}
 	if gotPV.Spec.PersistentVolumeReclaimPolicy != corev1.PersistentVolumeReclaimRetain {
-		t.Error("the volume was not retained")
+		t.Error("the volume was not retained for the swap")
 	}
 }

@@ -146,7 +146,9 @@ Every disk of a task lands on `spec.storageClass`, or the cluster default when i

 A `WaitForFirstConsumer` class does not fail an import, it **hangs** it: nothing consumes the claim while CDI populates it, so the transfer waits forever. The controller checks the binding mode before creating anything and fails the task with a message naming the class. On a stock Cozystack install the default class is `local`, which is `WaitForFirstConsumer` — so a task that names no class fails immediately with that explanation. `replicated` qualifies.

-The transferred volume is handed to its `VMDisk` without being copied: the `PersistentVolume` is retained and re-bound into the claim the disk expects. That volume stays on `Retain` permanently and this is part of the contract, not an implementation detail — CDI takes a controller owner reference on an adopted claim, so deleting the DataVolume garbage-collects the claim, and only the reclaim policy keeps the data.
+The transferred volume is handed to its `VMDisk` without being copied: the `PersistentVolume` is retained and re-bound into the claim the disk expects. `Retain` is held for the swap only. Between deleting the transferred claim and re-pointing the volume's `claimRef`, the volume is `Released`, and under `Delete` the provisioner would reclaim it on the spot. Once the new claim and the volume both report `Bound` to each other, the transferred claim is gone, and CDI has adopted the claim into the `VMDisk`'s DataVolume as its sole owner, the controller sets the volume back to the reclaim policy its StorageClass declares (`Delete` when the class names none or no longer exists). Only then does it create the `VMInstance` and mark the VM `Succeeded`, so a task in `Creating` a little longer than the transfer itself is this step waiting on the PV controller and CDI. From then on an imported disk behaves like any other: deleting the `VMDisk` removes its DataVolume, garbage collection takes the claim, and the volume and its backing storage go with it.
+
+Imports finished by a controller older than this behaviour left their volumes on `Retain` for good, so deleting such a `VMDisk` leaves a `Released` volume that no tenant can see or remove. Setting `persistentVolumeReclaimPolicy` back to the class's policy on those volumes, while their `VMDisk` still exists, lets them clean up like the rest; on one already `Released`, the same patch makes the provisioner delete it.

 **Budget roughly twice each disk's size against the tenant quota while an import runs.** CDI allocates a prime volume of the target size *and* a scratch volume of about 106% of it, so a 230 GiB disk occupies around 474 GiB until the transfer finishes. On a tenant near its quota this shows up as the import stalling on `exceeded quota` rather than as anything resembling a storage error, and it clears on its own once the previous disk releases its transit volumes.

@@ -43,7 +43,8 @@ rules:
  resources: ["persistentvolumeclaims"]
  verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
 # Storage classes are read to reject a binding mode that would deadlock an
-# import before anything is created.
+# import before anything is created, and to give each imported volume its
+# class's reclaim policy back once the handoff is over.
 - apiGroups: ["storage.k8s.io"]
  resources: ["storageclasses"]
  verbs: ["get", "list", "watch"]