Revert "Add ":detaching" suffix to batch attach CR only if it isn't already present." (#3736)

This reverts commit dc07f12.

Author: skogta

pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_controller.go

@@ -305,8 +305,9 @@ func (r *Reconciler) Reconcile(ctx context.Context,
 
 		patchErr := removeFinalizerFromCRDInstance(batchAttachCtx, instance, r.client)
 		if patchErr != nil {
+			recordEvent(batchAttachCtx, r, instance, v1.EventTypeWarning, patchErr.Error())
 			log.Errorf("failed to update CnsNodeVMBatchAttachment %s. Err: +%v", instance.Name, patchErr)
-			return r.completeReconciliationWithError(batchAttachCtx, instance, request.NamespacedName, timeout, err)
+			return reconcile.Result{RequeueAfter: timeout}, nil
 		}
 		log.Infof("Successfully removed finalizer %s from instance %s",
 			cnsoperatortypes.CNSFinalizer, request.NamespacedName.String())
@@ -315,7 +316,10 @@ func (r *Reconciler) Reconcile(ctx context.Context,
 		delete(backOffDuration, request.NamespacedName)
 		backOffDurationMapMutex.Unlock()
 
-		return r.completeReconciliationWithSuccess(batchAttachCtx, instance, request.NamespacedName, timeout)
+		msg := fmt.Sprintf("ReconcileCnsNodeVMBatchAttachment: Successfully processed instance %s",
+			request.NamespacedName.String())
+		recordEvent(batchAttachCtx, r, instance, v1.EventTypeNormal, msg)
+		return reconcile.Result{}, nil
 	}
 
 	// The CR is not being deleted, so call attach and detach for volumes.

pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go

@@ -22,7 +22,6 @@ import (
 	"errors"
 	"fmt"
 	"slices"
-	"strconv"
 	"strings"
 	"sync"
 
@@ -249,53 +248,18 @@ func updatePvcStatusEntryName(ctx context.Context,
 	instance *v1alpha1.CnsNodeVMBatchAttachment, pvcsToDetach map[string]string) {
 	log := logger.GetLogger(ctx)
 
-	allVolumeNamesInStatus := getVolumeNamesInStatus(instance)
-
 	for i, volume := range instance.Status.VolumeStatus {
 		if _, ok := pvcsToDetach[volume.PersistentVolumeClaim.ClaimName]; !ok {
 			continue
 		}
-		if strings.HasSuffix(instance.Status.VolumeStatus[i].Name, detachSuffix) {
-			log.Infof("VolumeName %s for PVC %s already contains suffix %s. Skipping.",
-				instance.Status.VolumeStatus[i].Name, volume.PersistentVolumeClaim.ClaimName,
-				detachSuffix)
-			continue
-		}
-		// First ensure that the volumeName with the suffix is unique.
-		newVolumeName := getUniqueVolumeName(instance.Status.VolumeStatus[i].Name,
-			allVolumeNamesInStatus)
+		newVolumeName := instance.Status.VolumeStatus[i].Name + detachSuffix
 		instance.Status.VolumeStatus[i].Name = newVolumeName
-		// Add the new volumename to allVolumeNamesInStatus
-		allVolumeNamesInStatus[newVolumeName] = true
 		log.Infof("Updating status name entry to %s for detaching PVC %s",
 			newVolumeName,
 			volume.PersistentVolumeClaim.ClaimName)
 	}
 }
 
-// getUniqueVolumeName finds a unque name for volume status entry.
-func getUniqueVolumeName(currentName string, allVolumeNamesInStatus map[string]bool) string {
-	detachingSuffixIndex := 1
-	newVolumeName := currentName + "-" + strconv.Itoa(detachingSuffixIndex) + detachSuffix
-	for {
-		// Unique name found.
-		if _, exists := allVolumeNamesInStatus[newVolumeName]; !exists {
-			return newVolumeName
-		}
-		detachingSuffixIndex++
-		newVolumeName = currentName + "-" + strconv.Itoa(detachingSuffixIndex) + detachSuffix
-	}
-}
-
-// getVolumeNamesInStatus returns all the names of the volume in the instance.
-func getVolumeNamesInStatus(instance *v1alpha1.CnsNodeVMBatchAttachment) map[string]bool {
-	volumeNamesInStatus := make(map[string]bool)
-	for _, volumeStatus := range instance.Status.VolumeStatus {
-		volumeNamesInStatus[volumeStatus.Name] = true
-	}
-	return volumeNamesInStatus
-}
-
 // updateInstanceStatus updates the given nodevmbatchattachment instance's status.
 func updateInstanceStatus(ctx context.Context, cnsoperatorclient client.Client,
 	instance *v1alpha1.CnsNodeVMBatchAttachment) error {

pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_test.go

@@ -19,15 +19,14 @@ package cnsnodevmbatchattachment
 import (
 	"context"
 	"fmt"
-	"reflect"
-	"strconv"
 	"sync"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/vmware/govmomi/object"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -234,6 +233,55 @@ func TestCnsNodeVMBatchAttachmentWhenVmOnVcenterReturnsError(t *testing.T) {
 	})
 }
 
+func TestCnsNodeVMBatchAttachmentWhenVmOnVcenterReturnsNotFoundError(t *testing.T) {
+
+	t.Run("TestCnsNodeVMBatchAttachmentWhenVmOnVcenterReturnsNotFoundError", func(t *testing.T) {
+		testCnsNodeVMBatchAttachment := setupTestCnsNodeVMBatchAttachment()
+		testCnsNodeVMBatchAttachment.Spec.InstanceUUID = "test-3"
+
+		r := setTestEnvironment(&testCnsNodeVMBatchAttachment, true)
+
+		req := reconcile.Request{
+			NamespacedName: types.NamespacedName{
+				Name:      testCnsNodeVMBatchAttachmentName,
+				Namespace: testNamespace,
+			},
+		}
+
+		GetVMFromVcenter = MockGetVMFromVcenter
+		commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
+
+		// Override with fake client
+		newClientFunc = func(ctx context.Context) (kubernetes.Interface, error) {
+			fakeK8sClient := getClientSetWithPvc()
+			return fakeK8sClient, nil
+		}
+
+		res, err := r.Reconcile(context.TODO(), req)
+		if err != nil {
+			t.Fatal("Unexpected reconcile error")
+		}
+		expectedReconcileResult := reconcile.Result{}
+		var expectedReconcileError error
+
+		assert.Equal(t, expectedReconcileResult, res)
+		assert.Equal(t, expectedReconcileError, err)
+
+		updatedCnsNodeVMBatchAttachment := &v1alpha1.CnsNodeVMBatchAttachment{}
+		err = r.client.Get(context.TODO(), req.NamespacedName, updatedCnsNodeVMBatchAttachment)
+		if err == nil {
+			t.Fatalf("failed to get cnsnodevmbatchattachemnt instance")
+		}
+
+		// Error should be not found
+		if statusErr, ok := err.(*errors.StatusError); ok {
+			assert.Equal(t, metav1.StatusReasonNotFound, statusErr.Status().Reason)
+		} else {
+			t.Fatalf("Unable to verify CnsNodeVMBatchAttachment error")
+		}
+	})
+}
+
 func TestCnsNodeVMBatchAttachmentWhenVmOnVcenterReturnsNotFoundErrorAndInstanceIsNotDeleted(t *testing.T) {
 	t.Run("TestCnsNodeVMBatchAttachmentWhenVmOnVcenterReturnsNotFoundErrorAndInstanceIsNotDeleted",
 		func(t *testing.T) {
@@ -692,143 +740,6 @@ func TestIsSharedPvc(t *testing.T) {
 	}
 }
 
-func TestGetUniqueVolumeName(t *testing.T) {
-	tests := []struct {
-		name                   string
-		currentName            string
-		allVolumeNamesInStatus map[string]bool
-		expected               string
-	}{
-		{
-			name:                   "No existing volumes — first name available",
-			currentName:            "vol1",
-			allVolumeNamesInStatus: map[string]bool{},
-			expected:               "vol1-1" + detachSuffix,
-		},
-		{
-			name:        "First detaching name already exists — use next index",
-			currentName: "vol1",
-			allVolumeNamesInStatus: map[string]bool{
-				"vol1-1:detaching": true,
-			},
-			expected: "vol1-2" + detachSuffix,
-		},
-		{
-			name:        "Several existing detaching names — find next available",
-			currentName: "vol1",
-			allVolumeNamesInStatus: map[string]bool{
-				"vol1-1:detaching": true,
-				"vol1-2:detaching": true,
-				"vol1-3:detaching": true,
-			},
-			expected: "vol1-4" + detachSuffix,
-		},
-		{
-			name:        "Different volume names in status — should not affect result",
-			currentName: "volX",
-			allVolumeNamesInStatus: map[string]bool{
-				"volA-1:detaching": true,
-				"volB-2:detaching": true,
-			},
-			expected: "volX-1" + detachSuffix,
-		},
-		{
-			name:        "Handles long existing name list gracefully",
-			currentName: "vol99",
-			allVolumeNamesInStatus: func() map[string]bool {
-				m := make(map[string]bool)
-				for i := 1; i <= 50; i++ {
-					m["vol99-"+strconv.Itoa(i)+":detaching"] = true
-				}
-				return m
-			}(),
-			expected: "vol99-51" + detachSuffix,
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			got := getUniqueVolumeName(tc.currentName, tc.allVolumeNamesInStatus)
-			if got != tc.expected {
-				t.Errorf("expected %s, got %s", tc.expected, got)
-			}
-		})
-	}
-}
-
-func TestGetVolumeNamesInStatus(t *testing.T) {
-	tests := []struct {
-		name     string
-		instance v1alpha1.CnsNodeVMBatchAttachment
-		expected map[string]bool
-	}{
-		{
-			name: "No volumes in status",
-			instance: v1alpha1.CnsNodeVMBatchAttachment{
-				Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
-					VolumeStatus: []v1alpha1.VolumeStatus{},
-				},
-			},
-			expected: map[string]bool{},
-		},
-		{
-			name: "Single volume in status",
-			instance: v1alpha1.CnsNodeVMBatchAttachment{
-				Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
-					VolumeStatus: []v1alpha1.VolumeStatus{
-						{Name: "vol1"},
-					},
-				},
-			},
-			expected: map[string]bool{
-				"vol1": true,
-			},
-		},
-		{
-			name: "Multiple unique volumes",
-			instance: v1alpha1.CnsNodeVMBatchAttachment{
-				Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
-					VolumeStatus: []v1alpha1.VolumeStatus{
-						{Name: "vol1"},
-						{Name: "vol2"},
-						{Name: "vol3"},
-					},
-				},
-			},
-			expected: map[string]bool{
-				"vol1": true,
-				"vol2": true,
-				"vol3": true,
-			},
-		},
-		{
-			name: "Duplicate volume names should not create duplicates in map",
-			instance: v1alpha1.CnsNodeVMBatchAttachment{
-				Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
-					VolumeStatus: []v1alpha1.VolumeStatus{
-						{Name: "vol1"},
-						{Name: "vol1"},
-						{Name: "vol2"},
-					},
-				},
-			},
-			expected: map[string]bool{
-				"vol1": true,
-				"vol2": true,
-			},
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			got := getVolumeNamesInStatus(&tc.instance)
-			if !reflect.DeepEqual(got, tc.expected) {
-				t.Errorf("expected %+v, got %+v", tc.expected, got)
-			}
-		})
-	}
-}
-
 func MockGetVMFromVcenter(ctx context.Context, nodeUUID string,
 	configInfo config.ConfigurationInfo) (*cnsvsphere.VirtualMachine, error) {
 	var vm *cnsvsphere.VirtualMachine