diff --git a/packages/system/vm-adoption-controller/images/controller/controller_test.go b/packages/system/vm-adoption-controller/images/controller/controller_test.go index 574a152b4..cb01a4b98 100644 --- a/packages/system/vm-adoption-controller/images/controller/controller_test.go +++ b/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) + } +} diff --git a/packages/system/vm-adoption-controller/images/controller/main.go b/packages/system/vm-adoption-controller/images/controller/main.go index ae5ef4a69..c7c43ca6e 100644 --- a/packages/system/vm-adoption-controller/images/controller/main.go +++ b/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--`). +// +// 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) { diff --git a/packages/system/vm-adoption-controller/templates/clusterrole.yaml b/packages/system/vm-adoption-controller/templates/clusterrole.yaml index 961675ea7..e0be6c55b 100644 --- a/packages/system/vm-adoption-controller/templates/clusterrole.yaml +++ b/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