verify pvcs for ownerreferences pointing to VM before cleanup during DR #2345

pruthvitd · 2025-11-24T18:03:08Z

This PR introduces improvements to the VM cleanup process as part of DR action and addresses a critical issue where Secondary VRG reconciliation stopped prematurely, leaving PVCs stuck in a Terminating state.

Key Changes

Safe VM Deletion Validation

Added logic to validate whether VM resources can be safely deleted before initiating cleanup.
Prevents accidental deletion when PVC ownership conditions are not met.

Conditional Cleanup

If validation passes, VM resources are deleted.
If validation fails, the controller proceeds with the previous reconciliation logic to maintain consistency. That is cleanup will be expected by the user.

Fix for Secondary VRG Reconciliation

Resolved an issue where Secondary VRG stopped reconciling before deleting associated VolumeGroupReplication (VGR) and VolumeReplication (VR) resources.
This fix ensures proper cleanup of VGR and VR resources, preventing PVCs from remaining in a Terminating state. Secondary will continue to reconcile until VRG desired state is achieved.

asn1809 · 2025-11-26T07:45:06Z

internal/controller/util/vm_util.go

+		}
+
+		// Fetch the owner object
+		owner, err := dynamicClient.Resource(gvr).


sigs.k8s.io/controller-runtime Client can be used to get the resources(including CRs) and is readily available as part of VRG controller. Please check if that can used instead of dynamic client.

Thanks for the suggestion! Agreed—using sigs.k8s.io/controller-runtime’s typed Client is the better approach since it leverages the shared cache and provides strong typing. While the dynamic client works without scheme registration by using unstructured.Unstructured, it lacks type safety, caching benefits, and requires more manual handling. I’ll refactor this to use the controller-runtime client and ensure the necessary schemes are registered.
Additionally, for ownership checks where we only need metadata (like ownerReferences), I’ll consider using PartialObjectMetadata with the typed client. This will reduce overhead compared to fetching full objects while still benefiting from caching and typed interactions.

asn1809 · 2025-11-29T11:54:53Z

cmd/main.go

 	"go.uber.org/zap/zapcore"
 	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"


This file effectively has no changes. Better to do git restore and continue.

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

raaizik · 2025-12-04T13:40:34Z

internal/controller/vrg_volrep.go

+func (v *VRGInstance) ShouldCleanupVMForSecondary() bool {
+	if !v.IsDRActionInProgress() {
+		v.log.Info("Skip VM cleanup; reconcile as secondary")
+
+		return false
+	}
+
+	v.log.Info(
+		"DR action progressing",
+		"component", "VRGController",
+		"action", "reconcile",
+		"vrgName", v.instance.GetName(),
+		"namespace", v.instance.GetNamespace(),
+		"desiredState", v.instance.Spec.ReplicationState,
+		"currentState", v.instance.Status.State,
+	)
+
+	vmNamespaceList := v.instance.Spec.KubeObjectProtection.RecipeParameters[recipecore.ProtectedVMNamespace]
+	vmList := v.instance.Spec.KubeObjectProtection.RecipeParameters[recipecore.VMList]
+
+	if len(vmList) > 0 {
+		if rmnutil.IsVMDeletionInProgress(v.ctx, v.reconciler.Client, vmList, vmNamespaceList, v.log) {
+			v.log.Info("VM deletion is in progress, skipping ownerreferences check")
+
+			return true
+		}
+
+		foundVMs, err := rmnutil.ListVMsByVMNamespace(v.ctx, v.reconciler.APIReader,
+			v.log, vmNamespaceList, vmList)
+		if len(foundVMs) == 0 || err != nil {
+			v.log.Info(
+				"No VirtualMachines found for cleanup; deletion appears complete",
+				"vmList", vmList,
+				"namespaceList", vmNamespaceList,
+			)
+
+			return false
+		}
+	}
+
+	if v.IsAllProtectedPVCsOwnedByProtectedVMs() {
+		v.log.Info("all protected PVCs have ownerreferences to protected list of VMs")
+		// Cleanup VM resources
+		err := rmnutil.DeleteVMs(v.ctx, v.reconciler.Client, vmList, vmNamespaceList, v.log)
+		if err != nil {
+			v.log.Error(err, "Failed to delete VMs",
+				"vmList", vmList,
+			)
+
+			return false
+		}
+
+		return true
+	}
+
+	v.log.Info("not all protected PVCs have ownerReferences to the protected list of VMs")
+
+	return false


This could be broken down into smaller functions: checking deletion in progress, then listing VMs by namespace, then checking all PVCs’ ownerRefs. It will ensure early exits are obvious

raaizik · 2025-12-04T14:01:38Z

internal/controller/util/vm_util.go

+			log.Info("Failed to fetch owner", "error", err)
+
+			continue // skip if not found


Skipping silently avoids surfacing of issues which I'm not sure is the best practice.

Suggested change

log.Info("Failed to fetch owner", "error", err)

continue // skip if not found

if k8serrors.IsNotFound(err) {{

log.V(1).Info("Owner not found during traversal", "owner", key)

continue

}

log.Error(err, "Failed fetching owner during VM ownership resolution", "owner", key)

return "", fmt.Errorf("failed to fetch owner %s: %w", key, err)

raaizik · 2025-12-04T14:11:22Z

internal/controller/util/vm_util.go

+// 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
+		}
+
+		// Fetch only metadata of the owner
+		ownerMeta := &metav1.PartialObjectMetadata{}
+		ownerMeta.SetGroupVersionKind(schema.FromAPIVersionAndKind(owner.APIVersion, owner.Kind))
+
+		if err := c.Get(ctx, client.ObjectKey{Namespace: obj.GetNamespace(), Name: owner.Name}, ownerMeta); err != nil {
+			log.Info("Failed to fetch owner", "error", err)
+
+			continue // skip if not found
+		}
+
+		// Continue traversal with the owner
+		// Recursively check its ownerReferences
+		obj = ownerMeta
+
+		nestedOwners := obj.GetOwnerReferences()
+		if len(nestedOwners) > 0 {
+			vmName, err := IsOwnedByVM(ctx, c, obj, nestedOwners, log)
+			if err == nil {
+				return vmName, nil
+			}
+		}
+	}
+
+	return "", fmt.Errorf("no VM owner found")
+}


Personally I would implement this as an iterative function. OwnerReferences in k8s are structured as flat lists that are singly linked, so recursion doesn’t add much here and can introduce avoidable issues. K8s controllers typically traverse owner chains iteratively. It avoids stack depth issues and makes cycle detection straightforward.

raaizik · 2025-12-04T14:26:36Z

internal/controller/volumereplicationgroup_controller.go

+// +kubebuilder:rbac:groups="kubevirt.io",resources=virtualmachines,verbs=get;list;watch;delete
+// +kubebuilder:rbac:groups="cdi.kubevirt.io",resources=datavolumes,verbs=get;list;watch


Best to keep these to the bare minimum. I think list verb for data volumes isn't required, as I don't see you're listing those via the k8s API.

list and watch are required because the controller-runtime client uses a cache backed by informers, even for Get calls on PartialObjectMetadata. Without list, the cache cannot initialize, and the client fails. For the calls used here, these RBACs are required.

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

pruthvitd force-pushed the verify-vm-ownership branch 4 times, most recently from 9735efb to 9506f80 Compare November 25, 2025 18:24

pruthvitd requested a review from raghavendra-talur November 25, 2025 18:25

pruthvitd marked this pull request as ready for review November 25, 2025 18:25

pruthvitd requested review from BenamarMk and ShyamsundarR as code owners November 25, 2025 18:25

pruthvitd force-pushed the verify-vm-ownership branch from 9506f80 to dae9610 Compare November 25, 2025 18:45

pruthvitd changed the title ~~verify pvcs for ownerreferences pointing to VM~~ verify pvcs for ownerreferences pointing to VM before cleanup during DR Nov 25, 2025

pruthvitd force-pushed the verify-vm-ownership branch from dae9610 to c8fd4fd Compare November 25, 2025 18:56

asn1809 reviewed Nov 26, 2025

View reviewed changes

pruthvitd force-pushed the verify-vm-ownership branch from c8fd4fd to 3e93726 Compare November 28, 2025 06:34

pruthvitd requested a review from asn1809 November 28, 2025 10:50

asn1809 reviewed Nov 29, 2025

View reviewed changes

pruthvitd force-pushed the verify-vm-ownership branch 3 times, most recently from 3aeb241 to 36a82ac Compare December 1, 2025 15:39

verify pvcs for ownerreferences pointing to VM

2d314d2

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

pruthvitd force-pushed the verify-vm-ownership branch from 36a82ac to 2d314d2 Compare December 2, 2025 12:56

raaizik reviewed Dec 4, 2025

View reviewed changes

VM cleanup validation and cleanup

100ac46

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

pruthvitd force-pushed the verify-vm-ownership branch from 56d611e to 100ac46 Compare December 5, 2025 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

verify pvcs for ownerreferences pointing to VM before cleanup during DR #2345

verify pvcs for ownerreferences pointing to VM before cleanup during DR #2345

Uh oh!

pruthvitd commented Nov 24, 2025 •

edited

Loading

Uh oh!

asn1809 Nov 26, 2025

Uh oh!

pruthvitd Nov 28, 2025

Uh oh!

asn1809 Nov 29, 2025

Uh oh!

raaizik Dec 4, 2025

Uh oh!

raaizik Dec 4, 2025

Uh oh!

raaizik Dec 4, 2025

Uh oh!

raaizik Dec 4, 2025

Uh oh!

pruthvitd Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		log.Info("Failed to fetch owner", "error", err)

		continue // skip if not found

-			log.Info("Failed to fetch owner", "error", err)
-			continue // skip if not found
+  if k8serrors.IsNotFound(err) {{
+      log.V(1).Info("Owner not found during traversal", "owner", key)
+      continue
+  }
+  log.Error(err, "Failed fetching owner during VM ownership resolution", "owner", key)
+  return "", fmt.Errorf("failed to fetch owner %s: %w", key, err)

		// +kubebuilder:rbac:groups="kubevirt.io",resources=virtualmachines,verbs=get;list;watch;delete
		// +kubebuilder:rbac:groups="cdi.kubevirt.io",resources=datavolumes,verbs=get;list;watch

verify pvcs for ownerreferences pointing to VM before cleanup during DR #2345

Are you sure you want to change the base?

verify pvcs for ownerreferences pointing to VM before cleanup during DR #2345

Uh oh!

Conversation

pruthvitd commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Changes

Safe VM Deletion Validation

Conditional Cleanup

Fix for Secondary VRG Reconciliation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pruthvitd commented Nov 24, 2025 •

edited

Loading