fix(vm-adoption-controller): keep the imported disk when adopting in place

Forklift owns every migrated PVC by the VirtualMachine it creates -- EnsureVM
patches each one with a blockOwnerDeletion ownerReference, upstream comment
"so that they'll be cleaned up when the VirtualMachine is removed". Adoption
then deleted that VM to free the name the VMInstance takes, and the garbage
collector took the disk with it: the clone lost its source mid-flight, the
DataVolume sat in CloneWithoutSource and the VM in ErrorPvcNotFound, with a
completed transfer destroyed by the adoption meant to consume it.

The code assumed the opposite -- "KubeVirt does not delete PVCs with a VM" --
which holds for an ordinary VM but not for a Forklift import.

Two changes, both scoped to same-namespace adoption:

Detach before deleting. Only the volumes this adoption consumes, and only
ownerReferences pointing at this VM, so an import that never reaches adoption
keeps Forklift's cleanup of abandoned migrations.

Stop cloning. On the raw-copy path Forklift already wrote the disk into the
target namespace with the right size, StorageClass and content, so the clone
copied a whole disk to stand still and left two full copies behind for good.
The disks keep referencing the imported PVC; vm-instance already falls back to
a bare PersistentVolumeClaim when no DataVolume of that name exists. The
cross-namespace virt-v2v path is unchanged -- there the copy is the point.

Assisted-By: Claude
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>

packages/system/vm-adoption-controller/images/controller/controller_test.go

@@ -22,6 +22,9 @@ var (
 	migrationsGVR  = schema.GroupVersionResource{Group: "forklift.konveyor.io", Version: "v1beta1", Resource: "migrations"}
 	vmsGVR         = schema.GroupVersionResource{Group: "kubevirt.io", Version: "v1", Resource: "virtualmachines"}
 	vmInstancesGVR = schema.GroupVersionResource{Group: vmInstanceGroup, Version: vmInstanceVersion, Resource: "vminstances"}
+	vmDisksGVR     = schema.GroupVersionResource{Group: vmDiskGroup, Version: vmDiskVersion, Resource: "vmdisks"}
+	dataVolumesGVR = schema.GroupVersionResource{Group: "cdi.kubevirt.io", Version: "v1beta1", Resource: "datavolumes"}
+	pvcsGVR        = schema.GroupVersionResource{Version: "v1", Resource: "persistentvolumeclaims"}
 )
 
 func newForkliftObj(kind, namespace, name, uid string, ann map[string]string) *unstructured.Unstructured {
@@ -59,11 +62,37 @@ func fakeClient(objs ...runtime.Object) *dynamicfake.FakeDynamicClient {
 			migrationsGVR:  "MigrationList",
 			vmsGVR:         "VirtualMachineList",
 			vmInstancesGVR: "VMInstanceList",
+			vmDisksGVR:     "VMDiskList",
+			dataVolumesGVR: "DataVolumeList",
+			pvcsGVR:        "PersistentVolumeClaimList",
 		},
 		objs...,
 	)
 }
 
+// newPVC builds an imported disk the way Forklift leaves it: a bare PVC owned by
+// the VirtualMachine Forklift created, with blockOwnerDeletion set.
+func newPVC(namespace, name, ownerVM string) *unstructured.Unstructured {
+	u := &unstructured.Unstructured{Object: map[string]interface{}{}}
+	u.SetAPIVersion("v1")
+	u.SetKind("PersistentVolumeClaim")
+	u.SetNamespace(namespace)
+	u.SetName(name)
+	if ownerVM != "" {
+		block := true
+		u.SetOwnerReferences([]metav1.OwnerReference{{
+			APIVersion:         "kubevirt.io/v1",
+			Kind:               "VirtualMachine",
+			Name:               ownerVM,
+			UID:                types.UID("vm-uid"),
+			BlockOwnerDeletion: &block,
+		}})
+	}
+	_ = unstructured.SetNestedField(u.Object, "16Gi", "spec", "resources", "requests", "storage")
+	_ = unstructured.SetNestedField(u.Object, "replicated", "spec", "storageClassName")
+	return u
+}
+
 func newVM(namespace, name string, labels map[string]string) *unstructured.Unstructured {
 	u := &unstructured.Unstructured{Object: map[string]interface{}{}}
 	u.SetAPIVersion("kubevirt.io/v1")
@@ -309,3 +338,126 @@ func TestIsAdoptionEnabled(t *testing.T) {
 		})
 	}
 }
+
+// On the raw-copy path Forklift already wrote the disk into the target
+// namespace. Cloning it into a VMDisk there would copy the whole disk a second
+// time and leave two full copies behind for good, so the adoption must keep
+// referencing the imported PVC instead.
+func TestWrapDisksKeepsImportedPVCWhenAdoptingInPlace(t *testing.T) {
+	client := fakeClient(newPVC("tenant-a", "web-disk-0", "web"))
+	c := &AdoptionController{dynamicClient: client, recorder: record.NewFakeRecorder(10)}
+
+	disks := []interface{}{map[string]interface{}{"name": "imported-0", "dvName": "web-disk-0"}}
+	if err := c.wrapDisksAsVMDisks(context.Background(), "tenant-a", "tenant-a", "web", disks); err != nil {
+		t.Fatalf("wrapDisksAsVMDisks: %v", err)
+	}
+
+	if got := disks[0].(map[string]interface{})["dvName"]; got != "web-disk-0" {
+		t.Errorf("dvName = %v, want the imported PVC web-disk-0 (a clone was substituted)", got)
+	}
+	list, err := client.Resource(vmDisksGVR).Namespace("tenant-a").List(context.Background(), metav1.ListOptions{})
+	if err != nil {
+		t.Fatalf("listing VMDisks: %v", err)
+	}
+	if len(list.Items) != 0 {
+		t.Errorf("created %d VMDisk(s) for an in-place adoption, want none", len(list.Items))
+	}
+}
+
+// The virt-v2v path still has to move the disk: conversion runs in a privileged
+// namespace the tenant cannot read from, so there the clone is the point.
+func TestWrapDisksClonesWhenCrossingNamespaces(t *testing.T) {
+	client := fakeClient(newPVC("cozy-forklift", "web-disk-0", "web"))
+	c := &AdoptionController{dynamicClient: client, recorder: record.NewFakeRecorder(10)}
+
+	disks := []interface{}{map[string]interface{}{"name": "imported-0", "dvName": "web-disk-0"}}
+	if err := c.wrapDisksAsVMDisks(context.Background(), "cozy-forklift", "tenant-a", "web", disks); err != nil {
+		t.Fatalf("wrapDisksAsVMDisks: %v", err)
+	}
+
+	if got := disks[0].(map[string]interface{})["dvName"]; got != "vm-disk-web-imported-0" {
+		t.Errorf("dvName = %v, want the VMDisk DataVolume vm-disk-web-imported-0", got)
+	}
+	if _, err := client.Resource(vmDisksGVR).Namespace("tenant-a").Get(context.Background(), "web-imported-0", metav1.GetOptions{}); err != nil {
+		t.Errorf("VMDisk was not created for the cross-namespace adoption: %v", err)
+	}
+}
+
+// The imported PVC must be detached from the Forklift VM BEFORE that VM is
+// deleted. Forklift owns the PVC by the VM with blockOwnerDeletion precisely so
+// the two are collected together, so if the order slips the garbage collector
+// destroys a finished transfer -- the observed failure was a completed 16 GiB
+// disk removed 357ms into adoption. The fake client runs no garbage collector,
+// so the ordering itself is what this asserts.
+func TestReleaseSourceVMDetachesImportedPVCBeforeDeletingVM(t *testing.T) {
+	client := fakeClient(
+		newVM("tenant-a", "web", map[string]string{"plan": "uid-1"}),
+		newPVC("tenant-a", "web-disk-0", "web"),
+	)
+
+	c := &AdoptionController{dynamicClient: client, recorder: record.NewFakeRecorder(10)}
+	vm := kubevirtv1.VirtualMachine{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant-a", Name: "web"}}
+	disks := []interface{}{map[string]interface{}{"name": "imported-0", "dvName": "web-disk-0"}}
+
+	if err := c.releaseSourceVM(context.Background(), vm, "tenant-a", "web", disks); err != nil {
+		t.Fatalf("releaseSourceVM: %v", err)
+	}
+
+	detachedAt, deletedAt := -1, -1
+	for i, a := range client.Actions() {
+		if a.Matches("update", "persistentvolumeclaims") && detachedAt < 0 {
+			detachedAt = i
+		}
+		if a.Matches("delete", "virtualmachines") && deletedAt < 0 {
+			deletedAt = i
+		}
+	}
+	if detachedAt < 0 {
+		t.Fatal("imported PVC was never detached from the source VM")
+	}
+	if deletedAt < 0 {
+		t.Fatal("source VM was never deleted")
+	}
+	if detachedAt > deletedAt {
+		t.Errorf("PVC detached at action %d but VM deleted at %d: the garbage collector would take the migrated disk with the VM", detachedAt, deletedAt)
+	}
+
+	cur, err := client.Resource(pvcsGVR).Namespace("tenant-a").Get(context.Background(), "web-disk-0", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("imported PVC did not survive adoption: %v", err)
+	}
+	for _, ref := range cur.GetOwnerReferences() {
+		if ref.Kind == "VirtualMachine" {
+			t.Errorf("imported PVC still owned by VirtualMachine %s after adoption", ref.Name)
+		}
+	}
+}
+
+// Detaching is narrow on purpose: only ownerReferences pointing at the VM being
+// released are dropped, so any other owner keeps its claim on the volume.
+func TestReleaseImportedVolumesLeavesForeignOwnersAlone(t *testing.T) {
+	pvc := newPVC("tenant-a", "web-disk-0", "web")
+	pvc.SetOwnerReferences(append(pvc.GetOwnerReferences(), metav1.OwnerReference{
+		APIVersion: "cdi.kubevirt.io/v1beta1",
+		Kind:       "DataVolume",
+		Name:       "someone-else",
+		UID:        types.UID("dv-uid"),
+	}))
+	client := fakeClient(pvc)
+	c := &AdoptionController{dynamicClient: client, recorder: record.NewFakeRecorder(10)}
+
+	vm := kubevirtv1.VirtualMachine{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant-a", Name: "web"}}
+	disks := []interface{}{map[string]interface{}{"name": "imported-0", "dvName": "web-disk-0"}}
+	if err := c.releaseImportedVolumes(context.Background(), vm, disks); err != nil {
+		t.Fatalf("releaseImportedVolumes: %v", err)
+	}
+
+	cur, err := client.Resource(pvcsGVR).Namespace("tenant-a").Get(context.Background(), "web-disk-0", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("reading PVC: %v", err)
+	}
+	refs := cur.GetOwnerReferences()
+	if len(refs) != 1 || refs[0].Kind != "DataVolume" {
+		t.Errorf("ownerReferences = %+v, want only the unrelated DataVolume owner", refs)
+	}
+}

packages/system/vm-adoption-controller/images/controller/main.go

@@ -689,7 +689,21 @@ func (c *AdoptionController) ensureVMDisk(ctx context.Context, vmDiskNs, vmDiskN
 // wrapDisksAsVMDisks turns the raw imported PVC reference of each disk into a
 // managed Cozystack VMDisk (clone) in the target namespace, rewriting each
 // disk's dvName to the VMDisk's DataVolume (`vm-disk-<vmInstance>-<disk>`).
+//
+// This is only needed when the disk has to cross a namespace: on the virt-v2v
+// path Forklift converts in a privileged namespace and the data must be copied
+// into the tenant. On the raw-copy path Forklift wrote the disk straight into
+// the target namespace, with the right size, StorageClass and content, so there
+// is nothing to move. Cloning it there would copy the whole disk a second time
+// for no gain and leave two full copies behind permanently -- and, worse, race
+// the clone against releaseSourceVM deleting the VM that owns the source PVC.
+// The disks keep pointing at the imported PVC instead; vm-instance references a
+// bare PersistentVolumeClaim when no DataVolume of that name exists.
 func (c *AdoptionController) wrapDisksAsVMDisks(ctx context.Context, srcNs, targetNs, vmInstanceName string, disks []interface{}) error {
+	if srcNs == targetNs {
+		klog.V(2).Infof("Adoption target %s is the import namespace; referencing the imported PVCs directly instead of cloning", targetNs)
+		return nil
+	}
 	for _, d := range disks {
 		dm, ok := d.(map[string]interface{})
 		if !ok {
@@ -981,7 +995,7 @@ func (c *AdoptionController) adoptVMViaVMDisks(ctx context.Context, vm kubevirtv
 	// earlier pass and only the source VM is left to release.
 	if _, err := c.dynamicClient.Resource(vmInstanceGVR).Namespace(targetNamespace).Get(ctx, vmInstanceName, metav1.GetOptions{}); err == nil {
 		klog.Infof("VMInstance %s/%s already exists, ensuring source VM is handled", targetNamespace, vmInstanceName)
-		return c.releaseSourceVM(ctx, vm, targetNamespace, vmInstanceName)
+		return c.releaseSourceVM(ctx, vm, targetNamespace, vmInstanceName, disks)
 	}
 
 	spec := map[string]interface{}{
@@ -1030,20 +1044,97 @@ func (c *AdoptionController) adoptVMViaVMDisks(ctx context.Context, vm kubevirtv
 	c.recorder.Eventf(vmRef(&vm), corev1.EventTypeNormal, "Adopted",
 		"Imported VM adopted as Cozystack VMInstance %s/%s", targetNamespace, vmInstanceName)
 
-	return c.releaseSourceVM(ctx, vm, targetNamespace, vmInstanceName)
+	return c.releaseSourceVM(ctx, vm, targetNamespace, vmInstanceName, disks)
+}
+
+// releaseImportedVolumes detaches the volumes this adoption consumes from the
+// Forklift-created VirtualMachine, so deleting that VM does not take the
+// migrated data with it.
+//
+// Forklift deliberately owns every migrated PVC by the VM it creates: EnsureVM
+// (pkg/controller/plan/kubevirt.go) patches each PVC with an ownerReference
+// carrying blockOwnerDeletion, described upstream as being there "so that
+// they'll be cleaned up when the VirtualMachine is removed". That is the right
+// default for Forklift and exactly wrong once Cozystack adopts the import: the
+// VMInstance references those PVCs, so a cascading delete destroys a completed
+// transfer.
+//
+// The detach is deliberately narrow -- only the volumes named by this VM's
+// disks, only ownerReferences pointing at this VM. An import that never reaches
+// adoption keeps Forklift's ownership, and with it Forklift's cleanup of
+// abandoned migrations.
+func (c *AdoptionController) releaseImportedVolumes(ctx context.Context, vm kubevirtv1.VirtualMachine, disks []interface{}) error {
+	gvrs := []schema.GroupVersionResource{
+		{Group: "cdi.kubevirt.io", Version: "v1beta1", Resource: "datavolumes"},
+		{Version: "v1", Resource: "persistentvolumeclaims"},
+	}
+	for _, d := range disks {
+		dm, ok := d.(map[string]interface{})
+		if !ok {
+			continue
+		}
+		name, _ := dm["dvName"].(string)
+		if name == "" {
+			continue
+		}
+		for _, gvr := range gvrs {
+			if err := c.dropVMOwnerReference(ctx, gvr, vm, name); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+// dropVMOwnerReference removes the ownerReferences pointing at vm from a single
+// object, leaving any other owner intact. A missing object is not an error:
+// Forklift deletes the DataVolume once the transfer completes, so on a finished
+// import only the PVC is normally still there.
+func (c *AdoptionController) dropVMOwnerReference(ctx context.Context, gvr schema.GroupVersionResource,
+	vm kubevirtv1.VirtualMachine, name string) error {
+
+	obj, err := c.dynamicClient.Resource(gvr).Namespace(vm.Namespace).Get(ctx, name, metav1.GetOptions{})
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return fmt.Errorf("failed to read %s %s/%s: %w", gvr.Resource, vm.Namespace, name, err)
+	}
+
+	refs := obj.GetOwnerReferences()
+	kept := make([]metav1.OwnerReference, 0, len(refs))
+	for _, ref := range refs {
+		// Same namespace and same name make this the VM being released; the
+		// typed VM carries no UID here, so kind+name is the available identity.
+		if ref.Kind == "VirtualMachine" && ref.Name == vm.Name {
+			continue
+		}
+		kept = append(kept, ref)
+	}
+	if len(kept) == len(refs) {
+		return nil
+	}
+
+	obj.SetOwnerReferences(kept)
+	if _, err := c.dynamicClient.Resource(gvr).Namespace(vm.Namespace).Update(ctx, obj, metav1.UpdateOptions{}); err != nil {
+		return fmt.Errorf("failed to detach %s %s/%s from VM %s: %w", gvr.Resource, vm.Namespace, name, vm.Name, err)
+	}
+	klog.Infof("Detached %s %s/%s from Forklift VM %s/%s so it survives adoption", gvr.Resource, vm.Namespace, name, vm.Namespace, vm.Name)
+	return nil
 }
 
 // releaseSourceVM hands the Forklift-created VM over to the managed VMInstance,
 // once that VMInstance exists. In the same namespace the VMInstance renders a
 // VirtualMachine under the same name, so the Forklift VM has to go; the imported
-// PVC stays behind as the VMDisk clone source, since KubeVirt does not delete
-// PVCs with a VM. In a different namespace nothing collides, so the VM is only
-// labeled adopted to keep it out of the next reconcile.
+// PVCs it referenced are detached from it first (see releaseImportedVolumes) so
+// they survive the delete and back the adopted VMInstance. In a different
+// namespace nothing collides, so the VM is only labeled adopted to keep it out
+// of the next reconcile.
 //
 // Failing here is retryable: the VMInstance is already created, so the next loop
 // takes the idempotency path and calls this again.
 func (c *AdoptionController) releaseSourceVM(ctx context.Context, vm kubevirtv1.VirtualMachine,
-	targetNamespace, vmInstanceName string) error {
+	targetNamespace, vmInstanceName string, disks []interface{}) error {
 
 	helmReleaseName := "vm-instance-" + vmInstanceName
 	if targetNamespace != vm.Namespace {
@@ -1053,6 +1144,13 @@ func (c *AdoptionController) releaseSourceVM(ctx context.Context, vm kubevirtv1.
 		return nil
 	}
 
+	// Must happen before the delete, not after: the garbage collector acts on
+	// the ownerReference as soon as the VM goes, and a PVC it has already
+	// removed cannot be recovered.
+	if err := c.releaseImportedVolumes(ctx, vm, disks); err != nil {
+		return fmt.Errorf("failed to detach imported volumes from source VM %s/%s: %w", vm.Namespace, vm.Name, err)
+	}
+
 	vmGVR := schema.GroupVersionResource{Group: "kubevirt.io", Version: "v1", Resource: "virtualmachines"}
 	if err := c.dynamicClient.Resource(vmGVR).Namespace(vm.Namespace).Delete(ctx, vm.Name, metav1.DeleteOptions{}); err != nil {
 		if apierrors.IsNotFound(err) {

packages/system/vm-adoption-controller/templates/clusterrole.yaml

@@ -28,10 +28,13 @@ rules:
   resources: ["datavolumes/source"]
   verbs: ["create"]
 
-# Read source PVCs to size the cross-namespace clone
+# Read source PVCs to size the cross-namespace clone, and detach the imported
+# PVCs from the Forklift VM before it is deleted (Forklift owns them by that VM
+# with blockOwnerDeletion, so without this the migrated data is garbage-collected
+# along with the VM the adoption replaces)
 - apiGroups: [""]
   resources: ["persistentvolumeclaims"]
-  verbs: ["get", "list"]
+  verbs: ["get", "list", "update", "patch"]
 
 # Emit Events on imported VMs so adoption outcomes are visible via
 # `kubectl describe` / the dashboard, not only in the controller log