VM cleanup validation and cleanup

Signed-off-by: pruthvitd <[email protected]>

Author: pruthvitd

config/rbac/role.yaml

@@ -235,6 +235,14 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - kubevirt.io
+  resources:
+  - virtualmachineinstances
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - kubevirt.io
   resources:
@@ -243,6 +251,7 @@ rules:
   - delete
   - get
   - list
+  - patch
   - watch
 - apiGroups:
   - multicluster.x-k8s.io

internal/controller/util/vm_util.go

@@ -3,9 +3,9 @@ package util
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/go-logr/logr"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
@@ -16,8 +16,8 @@ import (
 )
 
 const (
-	KindVirtualMachine = "VirtualMachine"
-	KubeVirtAPIVersion = "kubevirt.io/v1"
+	KindVirtualMachine       = "VirtualMachine"
+	KubeVirtAPIVersionPrefix = "kubevirt.io/" // prefix match on group; version follows (e.g., v1)
 )
 
 func ListVMsByLabelSelector(
@@ -64,37 +64,26 @@ func ListVMsByVMNamespace(
 	log logr.Logger,
 	vmNamespaceList []string,
 	vmList []string,
-) ([]string, error) {
-	var foundVMList []string
-
-	var notFoundErr error
-
+) []virtv1.VirtualMachine {
 	foundVM := &virtv1.VirtualMachine{}
 
+	foundVMList := make([]virtv1.VirtualMachine, 0, len(vmList))
 	for _, ns := range vmNamespaceList {
 		for _, vm := range vmList {
 			vmLookUp := types.NamespacedName{Namespace: ns, Name: vm}
 			if err := apiReader.Get(ctx, vmLookUp, foundVM); err != nil {
-				if !k8serrors.IsNotFound(err) {
-					return nil, err
-				}
-
-				if notFoundErr == nil {
-					notFoundErr = err
-				}
-
 				continue
 			}
 
-			foundVMList = append(foundVMList, foundVM.Name)
+			foundVMList = append(foundVMList, *foundVM)
 		}
 	}
 
 	if len(foundVMList) > 0 {
-		return foundVMList, nil
+		return foundVMList
 	}
 
-	return nil, notFoundErr
+	return nil
 }
 
 // IsVMDeletionInProgress returns true if any listed KubeVirt VM within the given protected NS is in deletion state.
@@ -121,6 +110,8 @@ func IsVMDeletionInProgress(ctx context.Context,
 
 			if !foundVM.GetDeletionTimestamp().IsZero() {
 				// Deletion of vm has been requested
+				log.Info("VM deletion is in progress", "VM", vm)
+
 				return true
 			}
 		}
@@ -134,65 +125,108 @@ func IsVMDeletionInProgress(ctx context.Context,
 func DeleteVMs(
 	ctx context.Context,
 	k8sclient client.Client,
+	foundVMs []virtv1.VirtualMachine,
 	vmList []string,
 	vmNamespaceList []string,
 	log logr.Logger,
 ) error {
-	for _, ns := range vmNamespaceList {
-		for _, vmName := range vmList {
-			vm := &virtv1.VirtualMachine{}
-			key := client.ObjectKey{Name: vmName, Namespace: ns}
+	for _, vm := range foundVMs {
+		ns := vm.GetNamespace()
 
-			if err := k8sclient.Get(ctx, key, vm); err != nil {
-				log.Error(err, "Failed to get VM", "namespace", ns, "name", vmName)
+		vmName := vm.GetName()
 
-				return fmt.Errorf("failed to get VM %s/%s: %w", ns, vmName, err)
-			}
-
-			if err := k8sclient.Delete(ctx, vm); err != nil {
-				log.Error(err, "Failed to delete VM", "namespace", ns, "name", vmName)
+		// Foreground deletion option
+		deleteOpts := &client.DeleteOptions{
+			GracePeriodSeconds: nil,
+			PropagationPolicy: func() *metav1.DeletionPropagation {
+				p := metav1.DeletePropagationForeground
 
-				return fmt.Errorf("failed to delete VM %s/%s: %w", ns, vmName, err)
-			}
+				return &p
+			}(),
+		}
+		if err := k8sclient.Delete(ctx, &vm, deleteOpts); err != nil {
+			log.Error(err, "Failed to delete VM", "namespace", ns, "name", vmName)
 
-			log.Info("Deleted VM successfully", "namespace", ns, "name", vmName)
+			return fmt.Errorf("failed to delete VM %s/%s: %w", ns, vmName, err)
 		}
+
+		log.Info("Deleted VMs successfully", "from namespace", ns, "VM name", vmName)
 	}
 
 	return nil
 }
 
-// IsOwnedByVM recursively traverses ownerReferences until it finds a VirtualMachine.
-func IsOwnedByVM(ctx context.Context, c client.Client, obj client.Object,
-	owners []metav1.OwnerReference, log logr.Logger,
-) (string, error) {
-	for _, owner := range owners {
-		if owner.Kind == KindVirtualMachine && owner.APIVersion == KubeVirtAPIVersion {
-			return owner.Name, nil // Found VM root
-		}
+// IsOwnedByVM walks the owner chain and returns the VM metadata object if found.
+// It prefers KubeVirt VM owners (kind=VirtualMachine, apigroup starts with kubevirt.io/)
+// Assuming all the owners are from same namespace
+// Typical KubeVirt ownership depth (PVC→DV→VM or PVC→VMI→VM or virt-launcher-pod->VMI->VM)
+func IsOwnedByVM(
+	ctx context.Context,
+	c client.Client,
+	obj client.Object,
+	log logr.Logger,
+) (client.Object, error) {
+	owners := obj.GetOwnerReferences()
+	// Breadth-first/flat traversal to reduce cognitive complexity
+	type queued struct {
+		ns    string
+		owner metav1.OwnerReference
+	}
+
+	q := make([]queued, 0, len(owners))
+	for _, o := range owners {
+		q = append(q, queued{ns: obj.GetNamespace(), owner: o})
+	}
 
-		// Fetch only metadata of the owner
-		ownerMeta := &metav1.PartialObjectMetadata{}
-		ownerMeta.SetGroupVersionKind(schema.FromAPIVersionAndKind(owner.APIVersion, owner.Kind))
+	for len(q) > 0 {
+		cur := q[0]
+		q = q[1:]
 
-		if err := c.Get(ctx, client.ObjectKey{Namespace: obj.GetNamespace(), Name: owner.Name}, ownerMeta); err != nil {
-			log.Info("Failed to fetch owner", "error", err)
+		// Try fetching only the owner's metadata
+		ownerMeta, err := fetchPartialMeta(ctx, c, cur.ns, cur.owner)
+		if err != nil {
+			log.Info("Failed to fetch owner", "gvk", cur.owner.APIVersion+"/"+cur.owner.Kind, "name", cur.owner.Name, "err", err)
 
-			continue // skip if not found
+			continue
 		}
 
-		// Continue traversal with the owner
-		// Recursively check its ownerReferences
-		obj = ownerMeta
+		if ownerMeta.GetUID() != cur.owner.UID {
+			// UID mismatch; skip
+			continue
+		}
 
-		nestedOwners := obj.GetOwnerReferences()
-		if len(nestedOwners) > 0 {
-			vmName, err := IsOwnedByVM(ctx, c, obj, nestedOwners, log)
-			if err == nil {
-				return vmName, nil
-			}
+		// If this owner is a KubeVirt VM, return it
+		if isKubeVirtVM(cur.owner) {
+			return ownerMeta, nil
+		}
+
+		// Otherwise, enqueue its parents (same namespace assumption for KubeVirt chain)
+		nestedOwners := ownerMeta.GetOwnerReferences()
+		for _, nestedOwner := range nestedOwners {
+			q = append(q, queued{ns: cur.ns, owner: nestedOwner})
 		}
 	}
 
-	return "", fmt.Errorf("no VM owner found")
+	return nil, fmt.Errorf("no VM owner found")
+}
+
+func isKubeVirtVM(o metav1.OwnerReference) bool {
+	return o.Kind == KindVirtualMachine && strings.HasPrefix(o.APIVersion, KubeVirtAPIVersionPrefix)
+}
+
+// Fetch only metadata of the owner
+func fetchPartialMeta(
+	ctx context.Context,
+	c client.Client,
+	ns string,
+	o metav1.OwnerReference,
+) (*metav1.PartialObjectMetadata, error) {
+	objMeta := &metav1.PartialObjectMetadata{}
+	objMeta.SetGroupVersionKind(schema.FromAPIVersionAndKind(o.APIVersion, o.Kind))
+
+	if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: o.Name}, objMeta); err != nil {
+		return nil, err
+	}
+
+	return objMeta, nil
 }

internal/controller/volumereplicationgroup_controller.go

@@ -10,6 +10,7 @@ import (
 	"maps"
 	"reflect"
 	"slices"
+	"strings"
 	"sync"
 	"time"
 
@@ -28,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/util/workqueue"
+	virtv1 "kubevirt.io/api/core/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -398,6 +400,7 @@ func filterPVC(reader client.Reader, pvc *corev1.PersistentVolumeClaim, log logr
 // +kubebuilder:rbac:groups="apiextensions.k8s.io",resources=customresourcedefinitions,verbs=get;list;watch
 // +kubebuilder:rbac:groups=core,resources=pods/exec,verbs=create
 // +kubebuilder:rbac:groups="kubevirt.io",resources=virtualmachines,verbs=get;list;watch;delete
+// +kubebuilder:rbac:groups="kubevirt.io",resources=virtualmachineinstances,verbs=get;list;watch
 // +kubebuilder:rbac:groups="cdi.kubevirt.io",resources=datavolumes,verbs=get;list;watch
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
@@ -1662,34 +1665,7 @@ func (v *VRGInstance) processAsSecondary() ctrl.Result {
 func (v *VRGInstance) reconcileAsSecondary() ctrl.Result {
 	result := ctrl.Result{}
 
-	if v.isVMRecipeProtection() {
-		v.log.Info("Checking VM cleanup and cross-cluster resource conflicts",
-			"name", v.instance.GetName(),
-			"namespace", v.instance.GetNamespace(),
-			"recipeName", "vm-recipe")
-
-		if err := v.validateVMsForStandaloneProtection(); err != nil {
-			result.Requeue = true
-		}
-
-		if !result.Requeue {
-			v.log.Info("VRG status observed",
-				"name", v.instance.GetName(),
-				"namespace", v.instance.GetNamespace(),
-				"replicationState", v.instance.Spec.ReplicationState,
-				"statusState", v.instance.Status.State,
-				"generation", v.instance.GetGeneration(),
-				"resourceVersion", v.instance.GetResourceVersion(),
-			)
-
-			if v.ShouldCleanupVMForSecondary() {
-				v.log.Info("Requeuing until VM cleanup is complete")
-
-				result.Requeue = true
-			}
-		}
-	}
-
+	result.Requeue = v.ShouldCleanupForSecondary()
 	result.Requeue = v.reconcileVolSyncAsSecondary() || result.Requeue
 	result.Requeue = v.reconcileVolRepsAsSecondary() || result.Requeue
 
@@ -2467,24 +2443,28 @@ func (v *VRGInstance) CheckForVMConflictOnSecondary() error {
 }
 
 func (v *VRGInstance) CheckForVMNameConflictOnSecondary(vmNamespaceList, vmList []string) error {
-	var foundVMs []string
-
-	var err error
-	if foundVMs, err = util.ListVMsByVMNamespace(v.ctx, v.reconciler.APIReader,
-		v.log, vmNamespaceList, vmList); err != nil {
-		if !k8serrors.IsNotFound(err) {
-			return fmt.Errorf("failed to lookup virtualmachine resources, check rbacs")
-		}
+	var foundVMs []virtv1.VirtualMachine
 
+	if foundVMs = util.ListVMsByVMNamespace(v.ctx, v.reconciler.APIReader,
+		v.log, vmNamespaceList, vmList); len(foundVMs) == 0 {
 		return nil
 	}
 
-	v.log.Info(fmt.Sprintf("found conflicting VM[%v] on secondary", foundVMs))
+	v.log.Info("found conflicting VMs on secondary", "foundVMs", vmNamesString(foundVMs))
 
 	return fmt.Errorf("protected VMs on the primary cluster share names with VMs on " +
 		"the secondary site, which may impact failover or recovery")
 }
 
+func vmNamesString(vms []virtv1.VirtualMachine) string {
+	names := make([]string, len(vms))
+	for i := range vms {
+		names[i] = vms[i].Name
+	}
+
+	return strings.Join(names, ", ")
+}
+
 func (v *VRGInstance) aggregateVRGNoClusterDataConflictCondition() *metav1.Condition {
 	var vmResourceConflict, pvcResourceConflict bool