Cleanup PVC finalizers added by cnsnodevmattachment

pkg/syncer/cnsoperator/manager/cleanupcustomresources.go

@@ -27,6 +27,7 @@ import (
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	cnsoperatorv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
+	nodeattachv1a1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsnodevmattachment/v1alpha1"
 	batchattachv1a1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsnodevmbatchattachment/v1alpha1"
 	cnsregistervolumev1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsregistervolume/v1alpha1"
 	cnsunregistervolumev1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsunregistervolume/v1alpha1"
@@ -234,3 +235,99 @@ func cleanupOrphanedBatchAttachPVCs(ctx context.Context, config rest.Config) {
 		}
 	}
 }
+
+// cleanupOrphanedNodeAttachPVCs removes the `CNSPvcFinalizer` from the PVCs in cases
+// where the CnsNodeVmAttachment CR gets deleted before the PVCs.
+// This is EXTREMELY UNLIKELY to happen but in case of concurrent updates and deletes of the CR,
+// it was observed that the CRs could sometime lose track of the PVCs that could lead to this.
+// This routine is a SAFETY GUARDRAIL to avoid situations where namespaces could remain stuck in terminating state.
+// This routine usually runs every 10 minutes.
+func cleanupOrphanedNodeAttachPVCs(ctx context.Context, config rest.Config) {
+	log := logger.GetLogger(ctx)
+
+	pvcList := commonco.ContainerOrchestratorUtility.ListPVCs(ctx, "")
+	if len(pvcList) == 0 {
+		log.Debug("no PVCs found in cluster, skipping node attach cleanup cycle")
+		return
+	}
+
+	// map to hold all the PVCs that are orphaned by CnsNodeVmAttachment reconciler.
+	// structure of the map follows the logical grouping kubernetes uses.
+	// namespace -> pvc -> struct{}{}
+	orphanPVCs := make(map[string]map[string]struct{})
+
+	for _, pvc := range pvcList {
+		if pvc.DeletionTimestamp == nil {
+			// PVC is not being deleted. Can be ignored.
+			continue
+		}
+
+		if !controllerutil.ContainsFinalizer(pvc, cnsoperatortypes.CNSPvcFinalizer) {
+			// not a PVC that is attached to or being attached to a VM. Can be ignored.
+			continue
+		}
+
+		// For CnsNodeVmAttachment, we need to check if there's a corresponding CR
+		// that references this PVC. Unlike batch attach, node attach doesn't use
+		// specific annotations, so we'll identify orphaned PVCs by checking if
+		// any CnsNodeVmAttachment CR exists that could be managing this PVC.
+
+		if _, ok := orphanPVCs[pvc.Namespace]; !ok {
+			orphanPVCs[pvc.Namespace] = map[string]struct{}{}
+		}
+		orphanPVCs[pvc.Namespace][pvc.Name] = struct{}{}
+	}
+
+	if len(orphanPVCs) == 0 {
+		log.Debug("no orphan PVCs found in cluster, skipping node attach cleanup cycle")
+		return
+	}
+
+	cnsClient, err := newClientForGroup(ctx, &config, cnsoperatorv1alpha1.GroupName)
+	if err != nil {
+		log.Error("failed to create cns operator client")
+		return
+	}
+
+	// Iterate over namespaces that have candidate orphan PVCs
+	for namespace := range orphanPVCs {
+		nodeAttachList := nodeattachv1a1.CnsNodeVmAttachmentList{}
+		err = cnsClient.List(ctx, &nodeAttachList, ctrlclient.InNamespace(namespace))
+		if err != nil {
+			log.With("kind", nodeAttachList.Kind).With("namespace", namespace).Error("listing failed")
+			continue
+		}
+
+		// If there are any CnsNodeVmAttachment CRs in the namespace, we need to be careful
+		// about which PVCs to clean up. For now, we'll take a conservative approach:
+		// Only clean up PVCs if there are NO CnsNodeVmAttachment CRs in the namespace.
+		// This ensures we don't accidentally remove finalizers from PVCs that might
+		// still be managed by existing CRs.
+		if len(nodeAttachList.Items) > 0 {
+			log.With("namespace", namespace).With("nodeAttachCRCount", len(nodeAttachList.Items)).
+				Debug("skipping PVC cleanup in namespace with existing CnsNodeVmAttachment CRs")
+			delete(orphanPVCs, namespace)
+		}
+	}
+
+	c, err := newForConfig(&config)
+	if err != nil {
+		log.Error("failed to create core API client")
+		return
+	}
+
+	// All the PVCs that are remaining in the map are eligible orphans whose finalizer can be removed.
+	for namespace, pvcs := range orphanPVCs {
+		for name := range pvcs {
+			err := k8s.RemoveFinalizerFromPVC(ctx, c, name, namespace, cnsoperatortypes.CNSPvcFinalizer)
+			if err != nil {
+				log.With("name", name).With("namespace", namespace).
+					Error("failed to remove the finalizer")
+				// safe to continue as failure to remove finalizer of a pvc doesn't influence others
+				continue
+			}
+			log.With("name", name).With("namespace", namespace).
+				Info("successfully removed CNS PVC finalizer from orphaned PVC")
+		}
+	}
+}

pkg/syncer/cnsoperator/manager/cleanupcustomresources_test.go

@@ -32,6 +32,7 @@ import (
 	runtimefake "sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	cnsoperatorapis "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
+	nodeattachv1a1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsnodevmattachment/v1alpha1"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsnodevmbatchattachment/v1alpha1"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/unittestcommon"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco"
@@ -499,3 +500,199 @@ func TestCleanupPVCs(t *testing.T) {
 		})
 	})
 }
+
+// createTestNodeAttachCR creates a test CnsNodeVmAttachment CR
+func createTestNodeAttachCR(name, namespace, volumeName string) *nodeattachv1a1.CnsNodeVmAttachment {
+	return &nodeattachv1a1.CnsNodeVmAttachment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+		Spec: nodeattachv1a1.CnsNodeVmAttachmentSpec{
+			NodeUUID:   "test-node-uuid",
+			VolumeName: volumeName,
+		},
+	}
+}
+
+func TestCleanupNodeAttachPVCs(t *testing.T) {
+	// Save original function variables
+	coUtilOrig := commonco.ContainerOrchestratorUtility
+	newClientForGroupOrig := newClientForGroup
+	newForConfigOrig := newForConfig
+
+	// Restore original functions after test
+	defer func() {
+		commonco.ContainerOrchestratorUtility = coUtilOrig
+		newClientForGroup = newClientForGroupOrig
+		newForConfig = newForConfigOrig
+	}()
+
+	setupNodeAttach := func(tt *testing.T, pvcs []*corev1.PersistentVolumeClaim,
+		nodeAttachCRs []*nodeattachv1a1.CnsNodeVmAttachment) kubernetes.Interface {
+		// Setup fake k8s client
+		k8sClient := k8sfake.NewClientset()
+		for _, pvc := range pvcs {
+			_, err := k8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(
+				context.Background(), pvc, metav1.CreateOptions{})
+			assert.NoError(tt, err)
+		}
+
+		// Setup fake controller-runtime client
+		scheme := runtime.NewScheme()
+		err := cnsoperatorapis.AddToScheme(scheme)
+		assert.NoError(tt, err)
+
+		var objects []client.Object
+		for _, cr := range nodeAttachCRs {
+			objects = append(objects, cr)
+		}
+		runtimeClient := runtimefake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
+
+		// Mock functions
+		fakeOrch := &unittestcommon.FakeK8SOrchestrator{}
+		fakeOrch.SetPVCs(pvcs)
+		commonco.ContainerOrchestratorUtility = fakeOrch
+
+		newClientForGroup = func(ctx context.Context, config *rest.Config, groupName string) (client.Client, error) {
+			return runtimeClient, nil
+		}
+
+		newForConfig = func(config *rest.Config) (kubernetes.Interface, error) {
+			return k8sClient, nil
+		}
+
+		return k8sClient
+	}
+
+	t.Run("WhenNoPVCsExist", func(tt *testing.T) {
+		k8sClient := setupNodeAttach(tt, []*corev1.PersistentVolumeClaim{}, []*nodeattachv1a1.CnsNodeVmAttachment{})
+
+		// Execute
+		cleanupOrphanedNodeAttachPVCs(context.Background(), rest.Config{})
+
+		// Assert - no PVCs should exist
+		pvcs, err := k8sClient.CoreV1().PersistentVolumeClaims("").List(context.Background(), metav1.ListOptions{})
+		assert.NoError(tt, err)
+		assert.Empty(tt, pvcs.Items)
+	})
+
+	t.Run("WhenAllPVCsWithoutDeletionTimestamp", func(tt *testing.T) {
+		pvcs := []*corev1.PersistentVolumeClaim{
+			createTestPVC("pvc-1", "ns-1", true),
+			createTestPVC("pvc-2", "ns-1", true),
+		}
+		k8sClient := setupNodeAttach(tt, pvcs, []*nodeattachv1a1.CnsNodeVmAttachment{})
+
+		// Execute
+		cleanupOrphanedNodeAttachPVCs(context.Background(), rest.Config{})
+
+		// Assert - all PVCs should still have finalizers (no cleanup should happen)
+		for _, pvc := range pvcs {
+			updatedPVC, err := k8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Get(
+				context.Background(), pvc.Name, metav1.GetOptions{})
+			assert.NoError(tt, err)
+			assert.True(tt, controllerutil.ContainsFinalizer(updatedPVC, cnsoperatortypes.CNSPvcFinalizer))
+		}
+	})
+
+	t.Run("WhenPVCsWithoutFinalizer", func(tt *testing.T) {
+		pvcs := []*corev1.PersistentVolumeClaim{
+			createTestPVCWithDeletion("pvc-1", "ns-1", false, false),
+			createTestPVCWithDeletion("pvc-2", "ns-1", false, false),
+		}
+		k8sClient := setupNodeAttach(tt, pvcs, []*nodeattachv1a1.CnsNodeVmAttachment{})
+
+		// Execute
+		cleanupOrphanedNodeAttachPVCs(context.Background(), rest.Config{})
+
+		// Assert - PVCs without finalizers should be ignored
+		for _, pvc := range pvcs {
+			updatedPVC, err := k8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Get(
+				context.Background(), pvc.Name, metav1.GetOptions{})
+			assert.NoError(tt, err)
+			assert.False(tt, controllerutil.ContainsFinalizer(updatedPVC, cnsoperatortypes.CNSPvcFinalizer))
+		}
+	})
+
+	t.Run("WhenOrphanedNodeAttachPVCsExist", func(tt *testing.T) {
+		pvcs := []*corev1.PersistentVolumeClaim{
+			createTestPVCWithDeletion("pvc-1", "ns-1", true, false),
+			createTestPVCWithDeletion("pvc-2", "ns-1", true, false),
+			createTestPVCWithDeletion("pvc-3", "ns-2", true, false),
+		}
+		k8sClient := setupNodeAttach(tt, pvcs, []*nodeattachv1a1.CnsNodeVmAttachment{})
+
+		// Execute
+		cleanupOrphanedNodeAttachPVCs(context.Background(), rest.Config{})
+
+		// Assert - all orphaned PVCs should have finalizers removed
+		for _, pvc := range pvcs {
+			updatedPVC, err := k8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Get(
+				context.Background(), pvc.Name, metav1.GetOptions{})
+			assert.NoError(tt, err)
+			assert.False(tt, controllerutil.ContainsFinalizer(updatedPVC, cnsoperatortypes.CNSPvcFinalizer))
+		}
+	})
+
+	t.Run("WhenNodeAttachCRsExistInNamespace", func(tt *testing.T) {
+		pvcs := []*corev1.PersistentVolumeClaim{
+			createTestPVCWithDeletion("pvc-1", "ns-1", true, false),
+			createTestPVCWithDeletion("pvc-2", "ns-2", true, false),
+		}
+		nodeAttachCRs := []*nodeattachv1a1.CnsNodeVmAttachment{
+			createTestNodeAttachCR("attach-1", "ns-1", "volume-1"),
+		}
+		k8sClient := setupNodeAttach(tt, pvcs, nodeAttachCRs)
+
+		// Execute
+		cleanupOrphanedNodeAttachPVCs(context.Background(), rest.Config{})
+
+		// Assert
+		// PVC in ns-1 should keep finalizer (CR exists in namespace)
+		pvc1, err := k8sClient.CoreV1().PersistentVolumeClaims("ns-1").Get(
+			context.Background(), "pvc-1", metav1.GetOptions{})
+		assert.NoError(tt, err)
+		assert.True(tt, controllerutil.ContainsFinalizer(pvc1, cnsoperatortypes.CNSPvcFinalizer))
+
+		// PVC in ns-2 should have finalizer removed (no CR in namespace)
+		pvc2, err := k8sClient.CoreV1().PersistentVolumeClaims("ns-2").Get(
+			context.Background(), "pvc-2", metav1.GetOptions{})
+		assert.NoError(tt, err)
+		assert.False(tt, controllerutil.ContainsFinalizer(pvc2, cnsoperatortypes.CNSPvcFinalizer))
+	})
+
+	t.Run("WhenMultipleNamespacesWithMixedStates", func(tt *testing.T) {
+		pvcs := []*corev1.PersistentVolumeClaim{
+			createTestPVCWithDeletion("pvc-1", "ns-1", true, false),
+			createTestPVCWithDeletion("pvc-2", "ns-2", true, false),
+			createTestPVCWithDeletion("pvc-3", "ns-3", true, false),
+		}
+		nodeAttachCRs := []*nodeattachv1a1.CnsNodeVmAttachment{
+			createTestNodeAttachCR("attach-1", "ns-2", "volume-1"),
+		}
+		k8sClient := setupNodeAttach(tt, pvcs, nodeAttachCRs)
+
+		// Execute
+		cleanupOrphanedNodeAttachPVCs(context.Background(), rest.Config{})
+
+		// Assert
+		// PVC in ns-1 should have finalizer removed (no CR in namespace)
+		pvc1, err := k8sClient.CoreV1().PersistentVolumeClaims("ns-1").Get(
+			context.Background(), "pvc-1", metav1.GetOptions{})
+		assert.NoError(tt, err)
+		assert.False(tt, controllerutil.ContainsFinalizer(pvc1, cnsoperatortypes.CNSPvcFinalizer))
+
+		// PVC in ns-2 should keep finalizer (CR exists in namespace)
+		pvc2, err := k8sClient.CoreV1().PersistentVolumeClaims("ns-2").Get(
+			context.Background(), "pvc-2", metav1.GetOptions{})
+		assert.NoError(tt, err)
+		assert.True(tt, controllerutil.ContainsFinalizer(pvc2, cnsoperatortypes.CNSPvcFinalizer))
+
+		// PVC in ns-3 should have finalizer removed (no CR in namespace)
+		pvc3, err := k8sClient.CoreV1().PersistentVolumeClaims("ns-3").Get(
+			context.Background(), "pvc-3", metav1.GetOptions{})
+		assert.NoError(tt, err)
+		assert.False(tt, controllerutil.ContainsFinalizer(pvc3, cnsoperatortypes.CNSPvcFinalizer))
+	})
+}

pkg/syncer/cnsoperator/manager/init.go

@@ -162,6 +162,22 @@ func InitCnsOperator(ctx context.Context, clusterFlavor cnstypes.CnsClusterFlavo
 			}()
 		}
 
+		// Start cleanup routine to remove orphaned PVC finalizers from CnsNodeVmAttachment.
+		// This handles cases where CnsNodeVmAttachment CRs are deleted before
+		// removing finalizers from their associated PVCs.
+		log.Info("Starting go routine to cleanup orphaned PVC finalizers from node attach.")
+		go func() {
+			for {
+				ctx, log = logger.GetNewContextWithLogger()
+				log.Info("Triggering node attach PVC finalizer cleanup routine")
+				cleanupOrphanedNodeAttachPVCs(ctx, *restConfig)
+				log.Info("Completed node attach PVC finalizer cleanup")
+				for i := 1; i <= cnsOperator.configInfo.Cfg.Global.CnsPVCProtectionCleanupIntervalInMin; i++ {
+					time.Sleep(1 * time.Minute)
+				}
+			}
+		}()
+
 		// Create CnsVolumeMetadata CRD
 		err = k8s.CreateCustomResourceDefinitionFromManifest(ctx, cnsoperatorconfig.EmbedCnsVolumeMetadataCRFile,
 			cnsoperatorconfig.EmbedCnsVolumeMetadataCRFileName)