Some minor fixes for batchattach API (#3740)

Author: skogta

pkg/apis/cnsoperator/cnsnodevmbatchattachment/v1alpha1/cnsnodebatchvmattachment_types.go

@@ -138,7 +138,7 @@ type PersistentVolumeClaimStatus struct {
 // +kubebuilder:subresource:status
 // +kubebuilder:object:root=true
 // +kubebuilder:resource:shortName=batchattach
-// +kubebuilder:printcolumn:name="NodeUUID",type="string",JSONPath=".spec.nodeUUID"
+// +kubebuilder:printcolumn:name="InstanceUUID",type="string",JSONPath=".spec.instanceUUID"
 
 // CnsNodeVMBatchAttachment is the Schema for the cnsnodevmbatchattachments API
 type CnsNodeVMBatchAttachment struct {

pkg/apis/cnsoperator/config/cns.vmware.com_cnsnodevmbatchattachments.yaml

@@ -17,8 +17,8 @@ spec:
   scope: Namespaced
   versions:
   - additionalPrinterColumns:
-    - jsonPath: .spec.nodeUUID
-      name: NodeUUID
+    - jsonPath: .spec.instanceUUID
+      name: InstanceUUID
       type: string
     name: v1alpha1
     schema:

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

@@ -305,9 +305,8 @@ 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 reconcile.Result{RequeueAfter: timeout}, nil
+			return r.completeReconciliationWithError(batchAttachCtx, instance, request.NamespacedName, timeout, err)
 		}
 		log.Infof("Successfully removed finalizer %s from instance %s",
 			cnsoperatortypes.CNSFinalizer, request.NamespacedName.String())
@@ -316,10 +315,7 @@ func (r *Reconciler) Reconcile(ctx context.Context,
 		delete(backOffDuration, request.NamespacedName)
 		backOffDurationMapMutex.Unlock()
 
-		msg := fmt.Sprintf("ReconcileCnsNodeVMBatchAttachment: Successfully processed instance %s",
-			request.NamespacedName.String())
-		recordEvent(batchAttachCtx, r, instance, v1.EventTypeNormal, msg)
-		return reconcile.Result{}, nil
+		return r.completeReconciliationWithSuccess(batchAttachCtx, instance, request.NamespacedName, timeout)
 	}
 
 	// The CR is not being deleted, so call attach and detach for volumes.

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

@@ -252,6 +252,12 @@ func updatePvcStatusEntryName(ctx context.Context,
 		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
+		}
 		newVolumeName := instance.Status.VolumeStatus[i].Name + detachSuffix
 		instance.Status.VolumeStatus[i].Name = newVolumeName
 		log.Infof("Updating status name entry to %s for detaching PVC %s",

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

@@ -26,7 +26,6 @@ import (
 	"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"
@@ -233,55 +232,6 @@ 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) {
@@ -740,6 +690,96 @@ func TestIsSharedPvc(t *testing.T) {
 	}
 }
 
+func TestUpdatePvcStatusEntryName_AddsSuffix(t *testing.T) {
+	ctx := context.TODO()
+
+	instance := &v1alpha1.CnsNodeVMBatchAttachment{
+		Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
+			VolumeStatus: []v1alpha1.VolumeStatus{
+				{
+					Name: "vol1",
+					PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimStatus{
+						ClaimName: "pvc-1",
+					},
+				},
+			},
+		},
+	}
+
+	pvcsToDetach := map[string]string{
+		"pvc-1": "",
+	}
+
+	updatePvcStatusEntryName(ctx, instance, pvcsToDetach)
+
+	got := instance.Status.VolumeStatus[0].Name
+	want := "vol1" + detachSuffix
+
+	if got != want {
+		t.Fatalf("expected volume name %q, got %q", want, got)
+	}
+}
+
+func TestUpdatePvcStatusEntryName_SkipsIfAlreadyHasSuffix(t *testing.T) {
+	ctx := context.TODO()
+
+	instance := &v1alpha1.CnsNodeVMBatchAttachment{
+		Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
+			VolumeStatus: []v1alpha1.VolumeStatus{
+				{
+					Name: "vol1" + detachSuffix,
+					PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimStatus{
+						ClaimName: "pvc-1",
+					},
+				},
+			},
+		},
+	}
+
+	pvcsToDetach := map[string]string{
+		"pvc-1": "",
+	}
+
+	updatePvcStatusEntryName(ctx, instance, pvcsToDetach)
+
+	got := instance.Status.VolumeStatus[0].Name
+	want := "vol1" + detachSuffix // should remain unchanged
+
+	if got != want {
+		t.Fatalf("expected volume name to remain %q, got %q", want, got)
+	}
+}
+
+func TestUpdatePvcStatusEntryName_SkipsNonTargetPVC(t *testing.T) {
+	ctx := context.TODO()
+
+	instance := &v1alpha1.CnsNodeVMBatchAttachment{
+		Status: v1alpha1.CnsNodeVMBatchAttachmentStatus{
+			VolumeStatus: []v1alpha1.VolumeStatus{
+				{
+					Name: "vol1",
+					PersistentVolumeClaim: v1alpha1.PersistentVolumeClaimStatus{
+						ClaimName: "pvc-1",
+					},
+				},
+			},
+		},
+	}
+
+	pvcsToDetach := map[string]string{
+		"some-other-pvc": "",
+	}
+
+	updatePvcStatusEntryName(ctx, instance, pvcsToDetach)
+
+	got := instance.Status.VolumeStatus[0].Name
+	want := "vol1" // unchanged
+
+	if got != want {
+		t.Fatalf("expected volume name %q (unchanged), got %q", want, got)
+	}
+}
+
 func MockGetVMFromVcenter(ctx context.Context, nodeUUID string,
 	configInfo config.ConfigurationInfo) (*cnsvsphere.VirtualMachine, error) {
 	var vm *cnsvsphere.VirtualMachine