diff --git a/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller.go b/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller.go index f4b5c0e897..3a25a06ba6 100644 --- a/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller.go +++ b/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller.go @@ -33,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" apitypes "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes/scheme" typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/tools/record" @@ -534,6 +535,40 @@ func (r *ReconcileCnsRegisterVolume) Reconcile(ctx context.Context, return reconcile.Result{RequeueAfter: timeout}, nil } + // If existing PVC has DataSourceRef and volumeMode set, handle volumeMode validation and inheritance + volumeModeInherited := false + if pvc != nil && pvc.Spec.DataSourceRef != nil && pvc.Spec.VolumeMode != nil { + if instance.Spec.VolumeMode == "" { + // Inherit the volumeMode from the existing PVC + log.Infof("Existing PVC %s in namespace %s has DataSourceRef and volumeMode set to %s. "+ + "CnsRegisterVolume does not have volumeMode set. Inheriting volumeMode from existing PVC.", + pvc.Name, pvc.Namespace, *pvc.Spec.VolumeMode) + instance.Spec.VolumeMode = *pvc.Spec.VolumeMode + volumeModeInherited = true + } else if instance.Spec.VolumeMode != *pvc.Spec.VolumeMode { + // Both are set but don't match - this is an error + msg := fmt.Sprintf("VolumeMode mismatch: existing PVC %s in namespace %s has volumeMode %s, "+ + "but CnsRegisterVolume specifies volumeMode %s", + pvc.Name, pvc.Namespace, *pvc.Spec.VolumeMode, instance.Spec.VolumeMode) + log.Error(msg) + setInstanceError(ctx, r, instance, msg) + return reconcile.Result{RequeueAfter: timeout}, nil + } + } + + // Persist the inherited volumeMode to the CnsRegisterVolume CR + if volumeModeInherited { + err = updateCnsRegisterVolume(ctx, r.client, instance) + if err != nil { + msg := fmt.Sprintf("Failed to update CnsRegisterVolume with inherited volumeMode. Error: %+v", err) + log.Error(msg) + setInstanceError(ctx, r, instance, msg) + return reconcile.Result{RequeueAfter: timeout}, nil + } + log.Infof("Successfully updated CnsRegisterVolume %s with inherited volumeMode: %s", + instance.Name, instance.Spec.VolumeMode) + } + // Do this check before creating a PV. Otherwise, PVC will be bound to PV after PV // is created even if validation fails if pvc != nil { @@ -618,6 +653,13 @@ func (r *ReconcileCnsRegisterVolume) Reconcile(ctx context.Context, setInstanceError(ctx, r, instance, msg) return reconcile.Result{RequeueAfter: timeout}, nil } + } else { + // PV exists - check if volumeMode needs correction + pv, err = validateAndFixPVVolumeMode(ctx, k8sclient, r, instance, pv, pvName, volumeID, + capacityInMb, accessMode, storageClassName, pvNodeAffinity, timeout) + if err != nil { + return reconcile.Result{RequeueAfter: timeout}, nil + } } // If PV is already bound to a different PVC at this point, then its a // duplicate request. @@ -1029,6 +1071,103 @@ func isBlockVolumeRegisterRequest(ctx context.Context, instance *cnsregistervolu return false } +// validateAndFixPVVolumeMode checks if an existing PV has the correct volumeMode. +// If the volumeMode doesn't match what's expected, it untags the CNS volume, deletes +// and recreates the PV with the correct volumeMode since volumeMode is immutable on PVs. +func validateAndFixPVVolumeMode(ctx context.Context, k8sclient clientset.Interface, + r *ReconcileCnsRegisterVolume, instance *cnsregistervolumev1alpha1.CnsRegisterVolume, + pv *v1.PersistentVolume, pvName, volumeID string, capacityInMb int64, + accessMode v1.PersistentVolumeAccessMode, storageClassName string, + pvNodeAffinity *v1.VolumeNodeAffinity, timeout time.Duration) (*v1.PersistentVolume, error) { + log := logger.GetLogger(ctx) + + // Determine expected volumeMode + expectedVolumeMode := instance.Spec.VolumeMode + if expectedVolumeMode == "" { + expectedVolumeMode = v1.PersistentVolumeFilesystem + } + + // Get actual volumeMode from PV + pvVolumeMode := v1.PersistentVolumeFilesystem + if pv.Spec.VolumeMode != nil { + pvVolumeMode = *pv.Spec.VolumeMode + } + + // Check if volumeMode matches + if expectedVolumeMode != pvVolumeMode { + log.Warnf("PV %s exists but has incorrect volumeMode. Expected: %s, Actual: %s. "+ + "Untagging CNS volume and recreating PV with correct volumeMode.", pvName, expectedVolumeMode, pvVolumeMode) + + // Untag the CNS volume before deleting PV to prevent underlying volume deletion. + // deleteDisk=false ensures the underlying vSphere disk is preserved. + log.Infof("Untagging CNS volume %s to preserve underlying disk before PV deletion", volumeID) + _, err := common.DeleteVolumeUtil(ctx, r.volumeManager, volumeID, false) + if err != nil { + msg := fmt.Sprintf("Failed to untag CNS volume %s. Error: %+v", volumeID, err) + log.Error(msg) + setInstanceError(ctx, r, instance, msg) + return nil, err + } + log.Infof("Successfully untagged CNS volume %s", volumeID) + + // Delete the existing PV (underlying volume is safe due to CNS untag with deleteDisk=false) + err = k8sclient.CoreV1().PersistentVolumes().Delete(ctx, pvName, *metav1.NewDeleteOptions(0)) + if err != nil { + msg := fmt.Sprintf("Failed to delete PV %s with incorrect volumeMode. Error: %+v", pvName, err) + log.Error(msg) + setInstanceError(ctx, r, instance, msg) + return nil, err + } + log.Infof("Successfully deleted PV %s with incorrect volumeMode", pvName) + + // Wait for PV to be fully deleted before recreating + log.Infof("Waiting for PV %s to be fully deleted", pvName) + waitErr := wait.PollUntilContextTimeout(ctx, time.Second, timeout, true, func(ctx context.Context) (bool, error) { + _, err := k8sclient.CoreV1().PersistentVolumes().Get(ctx, pvName, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + // PV is fully deleted + return true, nil + } + // Unexpected error + log.Warnf("Error checking PV deletion status: %+v", err) + return false, err + } + // PV still exists, continue waiting + return false, nil + }) + if waitErr != nil { + msg := fmt.Sprintf("Timeout waiting for PV %s to be deleted. Error: %+v", pvName, waitErr) + log.Error(msg) + setInstanceError(ctx, r, instance, msg) + return nil, waitErr + } + log.Infof("PV %s has been fully deleted", pvName) + + // Recreate PV with correct volumeMode + claimRef := &v1.ObjectReference{ + Kind: "PersistentVolumeClaim", + APIVersion: "v1", + Namespace: instance.Namespace, + Name: instance.Spec.PvcName, + } + pvSpec := getPersistentVolumeSpec(pvName, volumeID, capacityInMb, + accessMode, instance.Spec.VolumeMode, storageClassName, claimRef) + pvSpec.Spec.NodeAffinity = pvNodeAffinity + log.Debugf("Recreating PV with spec: %+v", pvSpec) + pv, err = k8sclient.CoreV1().PersistentVolumes().Create(ctx, pvSpec, metav1.CreateOptions{}) + if err != nil { + log.Errorf("Failed to recreate PV with spec: %+v. Error: %+v", pvSpec, err) + setInstanceError(ctx, r, instance, + fmt.Sprintf("Failed to recreate PV: %s with correct volumeMode. Error: %+v", pvName, err)) + return nil, err + } + log.Infof("Successfully recreated PV %s with correct volumeMode: %s", pvName, expectedVolumeMode) + } + + return pv, nil +} + // setInstanceError sets error and records an event on the CnsRegisterVolume // instance. func setInstanceError(ctx context.Context, r *ReconcileCnsRegisterVolume, diff --git a/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go b/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go index 4240944767..52c0bc838a 100644 --- a/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go +++ b/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go @@ -33,6 +33,7 @@ import ( vim25types "github.com/vmware/govmomi/vim25/types" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -796,6 +797,39 @@ var _ = Describe("checkExistingPVCDataSourceRef", func() { Expect(pvc).To(BeNil()) }) }) + + Context("when PVC exists with DataSourceRef and volumeMode set", func() { + BeforeEach(func() { + apiGroup := "vmoperator.vmware.com" + volumeMode := corev1.PersistentVolumeBlock + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: "VirtualMachine", + Name: "test-vm", + }, + VolumeMode: &volumeMode, + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + Expect(err).To(BeNil()) + }) + + It("should return the PVC with volumeMode set", func() { + pvc, err := checkExistingPVCDataSourceRef(ctx, k8sclient, pvcName, namespace) + Expect(err).To(BeNil()) + Expect(pvc).ToNot(BeNil()) + Expect(pvc.Name).To(Equal(pvcName)) + Expect(pvc.Spec.DataSourceRef).ToNot(BeNil()) + Expect(pvc.Spec.VolumeMode).ToNot(BeNil()) + Expect(*pvc.Spec.VolumeMode).To(Equal(corev1.PersistentVolumeBlock)) + }) + }) }) var _ = Describe("validatePVCTopologyCompatibility", func() { @@ -1360,3 +1394,593 @@ func TestGetPersistentVolumeSpecWithVolumeMode(t *testing.T) { int64(capacity), accessMode, volumeMode, scName, nil) assert.Equal(t, volumeMode, *pv.Spec.VolumeMode) } + +func TestGetPersistentVolumeSpecWhenVolumeModeIsEmptyWithoutSharedDisk(t *testing.T) { + var ( + volumeName = "vol-1" + volumeID = "123456" + capacity = 256 + accessMode = corev1.ReadWriteOnce + scName = "testsc" + ) + + isSharedDiskEnabled = false + pv := getPersistentVolumeSpec(volumeName, volumeID, int64(capacity), accessMode, "", scName, nil) + // volumeMode should be set to Filesystem even when isSharedDiskEnabled is false + assert.NotNil(t, pv.Spec.VolumeMode, "VolumeMode should be set even when isSharedDiskEnabled is false") + assert.Equal(t, corev1.PersistentVolumeFilesystem, *pv.Spec.VolumeMode) +} + +func TestGetPersistentVolumeSpecWithVolumeModeWithoutSharedDisk(t *testing.T) { + var ( + volumeName = "vol-1" + volumeID = "123456" + capacity = 256 + accessMode = corev1.ReadWriteOnce + scName = "testsc" + volumeMode = corev1.PersistentVolumeBlock + ) + + isSharedDiskEnabled = false + pv := getPersistentVolumeSpec(volumeName, volumeID, + int64(capacity), accessMode, volumeMode, scName, nil) + // volumeMode should be set to Block even when isSharedDiskEnabled is false + assert.NotNil(t, pv.Spec.VolumeMode, "VolumeMode should be set even when isSharedDiskEnabled is false") + assert.Equal(t, volumeMode, *pv.Spec.VolumeMode) +} + +func TestVolumeModeInheritanceFromExistingPVCWithDataSourceRef(t *testing.T) { + ctx := context.Background() + k8sclient := k8sfake.NewSimpleClientset() + namespace := "test-namespace" + pvcName := "test-pvc" + + // Create a PVC with DataSourceRef and volumeMode set to Block + apiGroup := "vmoperator.vmware.com" + volumeMode := corev1.PersistentVolumeBlock + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: "VirtualMachine", + Name: "test-vm", + }, + VolumeMode: &volumeMode, + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Test case 1: CnsRegisterVolume without volumeMode should inherit from PVC + instance := &cnsregistervolumev1alpha1.CnsRegisterVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "register-vol", + Namespace: namespace, + }, + Spec: cnsregistervolumev1alpha1.CnsRegisterVolumeSpec{ + PvcName: pvcName, + VolumeID: "123456", + AccessMode: corev1.ReadWriteOnce, + // VolumeMode is not set + }, + } + + // Simulate the logic from the Reconcile function + existingPVC, err := checkExistingPVCDataSourceRef(ctx, k8sclient, instance.Spec.PvcName, instance.Namespace) + assert.NoError(t, err) + assert.NotNil(t, existingPVC) + + // Apply the volumeMode validation and inheritance logic + mismatchDetected := false + if existingPVC != nil && existingPVC.Spec.DataSourceRef != nil && existingPVC.Spec.VolumeMode != nil { + if instance.Spec.VolumeMode == "" { + instance.Spec.VolumeMode = *existingPVC.Spec.VolumeMode + } else if instance.Spec.VolumeMode != *existingPVC.Spec.VolumeMode { + mismatchDetected = true + } + } + + // Verify that volumeMode was inherited and no mismatch detected + assert.False(t, mismatchDetected) + assert.Equal(t, corev1.PersistentVolumeBlock, instance.Spec.VolumeMode) +} + +func TestVolumeModeNotInheritedWhenAlreadySet(t *testing.T) { + ctx := context.Background() + k8sclient := k8sfake.NewSimpleClientset() + namespace := "test-namespace" + pvcName := "test-pvc" + + // Create a PVC with DataSourceRef and volumeMode set to Block + apiGroup := "vmoperator.vmware.com" + volumeMode := corev1.PersistentVolumeBlock + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: "VirtualMachine", + Name: "test-vm", + }, + VolumeMode: &volumeMode, + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Test case 2: CnsRegisterVolume with volumeMode already set should NOT be overridden + instance := &cnsregistervolumev1alpha1.CnsRegisterVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "register-vol", + Namespace: namespace, + }, + Spec: cnsregistervolumev1alpha1.CnsRegisterVolumeSpec{ + PvcName: pvcName, + VolumeID: "123456", + AccessMode: corev1.ReadWriteOnce, + VolumeMode: corev1.PersistentVolumeFilesystem, // Already set to Filesystem + }, + } + + originalVolumeMode := instance.Spec.VolumeMode + + // Simulate the logic from the Reconcile function + existingPVC, err := checkExistingPVCDataSourceRef(ctx, k8sclient, instance.Spec.PvcName, instance.Namespace) + assert.NoError(t, err) + assert.NotNil(t, existingPVC) + + // Apply the volumeMode validation and inheritance logic + mismatchDetected := false + if existingPVC != nil && existingPVC.Spec.DataSourceRef != nil && existingPVC.Spec.VolumeMode != nil { + if instance.Spec.VolumeMode == "" { + instance.Spec.VolumeMode = *existingPVC.Spec.VolumeMode + } else if instance.Spec.VolumeMode != *existingPVC.Spec.VolumeMode { + mismatchDetected = true + } + } + + // Verify that mismatch was detected since PVC has Block but instance has Filesystem + assert.True(t, mismatchDetected) + // Verify that volumeMode was NOT changed (still has original Filesystem) + assert.Equal(t, originalVolumeMode, instance.Spec.VolumeMode) + assert.Equal(t, corev1.PersistentVolumeFilesystem, instance.Spec.VolumeMode) +} + +func TestVolumeModeNotInheritedWhenNoDataSourceRef(t *testing.T) { + ctx := context.Background() + k8sclient := k8sfake.NewSimpleClientset() + namespace := "test-namespace" + pvcName := "test-pvc" + + // Create a PVC without DataSourceRef but with volumeMode set + volumeMode := corev1.PersistentVolumeBlock + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: nil, // No DataSourceRef + VolumeMode: &volumeMode, + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Test case 3: CnsRegisterVolume should NOT inherit volumeMode when PVC has no DataSourceRef + instance := &cnsregistervolumev1alpha1.CnsRegisterVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "register-vol", + Namespace: namespace, + }, + Spec: cnsregistervolumev1alpha1.CnsRegisterVolumeSpec{ + PvcName: pvcName, + VolumeID: "123456", + AccessMode: corev1.ReadWriteOnce, + // VolumeMode is not set + }, + } + + // Simulate the logic from the Reconcile function + existingPVC, err := checkExistingPVCDataSourceRef(ctx, k8sclient, instance.Spec.PvcName, instance.Namespace) + assert.NoError(t, err) + assert.NotNil(t, existingPVC) + + // Apply the volumeMode validation and inheritance logic + mismatchDetected := false + if existingPVC != nil && existingPVC.Spec.DataSourceRef != nil && existingPVC.Spec.VolumeMode != nil { + if instance.Spec.VolumeMode == "" { + instance.Spec.VolumeMode = *existingPVC.Spec.VolumeMode + } else if instance.Spec.VolumeMode != *existingPVC.Spec.VolumeMode { + mismatchDetected = true + } + } + + // Verify that no mismatch detected and volumeMode was NOT inherited (should remain empty) + // because PVC has no DataSourceRef + assert.False(t, mismatchDetected) + assert.Equal(t, corev1.PersistentVolumeMode(""), instance.Spec.VolumeMode) +} + +func TestVolumeModeNotInheritedWhenPVCVolumeModeIsNil(t *testing.T) { + ctx := context.Background() + k8sclient := k8sfake.NewSimpleClientset() + namespace := "test-namespace" + pvcName := "test-pvc" + + // Create a PVC with DataSourceRef but VolumeMode is nil (not set) + apiGroup := "vmoperator.vmware.com" + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: "VirtualMachine", + Name: "test-vm", + }, + VolumeMode: nil, // VolumeMode is nil + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Test case: CnsRegisterVolume should NOT inherit when PVC's volumeMode is nil + instance := &cnsregistervolumev1alpha1.CnsRegisterVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "register-vol", + Namespace: namespace, + }, + Spec: cnsregistervolumev1alpha1.CnsRegisterVolumeSpec{ + PvcName: pvcName, + VolumeID: "123456", + AccessMode: corev1.ReadWriteOnce, + // VolumeMode is not set + }, + } + + // Simulate the logic from the Reconcile function + existingPVC, err := checkExistingPVCDataSourceRef(ctx, k8sclient, instance.Spec.PvcName, instance.Namespace) + assert.NoError(t, err) + assert.NotNil(t, existingPVC) + + // Apply the volumeMode validation and inheritance logic + mismatchDetected := false + if existingPVC != nil && existingPVC.Spec.DataSourceRef != nil && existingPVC.Spec.VolumeMode != nil { + if instance.Spec.VolumeMode == "" { + instance.Spec.VolumeMode = *existingPVC.Spec.VolumeMode + } else if instance.Spec.VolumeMode != *existingPVC.Spec.VolumeMode { + mismatchDetected = true + } + } + + // Verify that no mismatch detected and volumeMode was NOT inherited (should remain empty) + // because PVC's volumeMode is nil + assert.False(t, mismatchDetected) + assert.Equal(t, corev1.PersistentVolumeMode(""), instance.Spec.VolumeMode) +} + +func TestVolumeModeMatchesExistingPVC(t *testing.T) { + ctx := context.Background() + k8sclient := k8sfake.NewSimpleClientset() + namespace := "test-namespace" + pvcName := "test-pvc" + + // Create a PVC with DataSourceRef and volumeMode set to Block + apiGroup := "vmoperator.vmware.com" + volumeMode := corev1.PersistentVolumeBlock + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: "VirtualMachine", + Name: "test-vm", + }, + VolumeMode: &volumeMode, + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Test case: Both have volumeMode set to Block - should succeed + instance := &cnsregistervolumev1alpha1.CnsRegisterVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "register-vol", + Namespace: namespace, + }, + Spec: cnsregistervolumev1alpha1.CnsRegisterVolumeSpec{ + PvcName: pvcName, + VolumeID: "123456", + AccessMode: corev1.ReadWriteOnce, + VolumeMode: corev1.PersistentVolumeBlock, // Matches PVC + }, + } + + // Simulate the logic from the Reconcile function + existingPVC, err := checkExistingPVCDataSourceRef(ctx, k8sclient, instance.Spec.PvcName, instance.Namespace) + assert.NoError(t, err) + assert.NotNil(t, existingPVC) + + // Apply the volumeMode validation logic + mismatchDetected := false + if existingPVC != nil && existingPVC.Spec.DataSourceRef != nil && existingPVC.Spec.VolumeMode != nil { + if instance.Spec.VolumeMode == "" { + instance.Spec.VolumeMode = *existingPVC.Spec.VolumeMode + } else if instance.Spec.VolumeMode != *existingPVC.Spec.VolumeMode { + mismatchDetected = true + } + } + + // Verify that no mismatch was detected + assert.False(t, mismatchDetected) + assert.Equal(t, corev1.PersistentVolumeBlock, instance.Spec.VolumeMode) +} + +func TestPVRecreationWhenVolumeModeIncorrect(t *testing.T) { + ctx := context.Background() + k8sclient := k8sfake.NewSimpleClientset() + namespace := "test-namespace" + pvcName := "test-pvc" + pvName := "pvc-12345678-1234-1234-1234-123456789012" + volumeID := "test-volume-id" + storageClassName := "test-sc" + + // Enable shared disk feature to ensure volumeMode is set in PV spec + isSharedDiskEnabled = true + defer func() { + isSharedDiskEnabled = false + }() + + // Create a PVC with DataSourceRef and volumeMode set to Block + apiGroup := "vmoperator.vmware.com" + volumeMode := corev1.PersistentVolumeBlock + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + UID: "12345678-1234-1234-1234-123456789012", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: "VirtualMachine", + Name: "test-vm", + }, + VolumeMode: &volumeMode, + StorageClassName: &storageClassName, + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Create CnsRegisterVolume with volumeMode set to Block (inherited or explicitly set) + instance := &cnsregistervolumev1alpha1.CnsRegisterVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "register-vol", + Namespace: namespace, + }, + Spec: cnsregistervolumev1alpha1.CnsRegisterVolumeSpec{ + PvcName: pvcName, + VolumeID: volumeID, + AccessMode: corev1.ReadWriteOnce, + VolumeMode: corev1.PersistentVolumeBlock, // Expect Block + }, + } + + // Create an existing PV with INCORRECT volumeMode (Filesystem instead of Block) + incorrectVolumeMode := corev1.PersistentVolumeFilesystem + existingPV := &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + }, + Spec: corev1.PersistentVolumeSpec{ + Capacity: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Gi"), + }, + PersistentVolumeSource: corev1.PersistentVolumeSource{ + CSI: &corev1.CSIPersistentVolumeSource{ + Driver: "csi.vsphere.vmware.com", + VolumeHandle: volumeID, + }, + }, + AccessModes: []corev1.PersistentVolumeAccessMode{ + corev1.ReadWriteOnce, + }, + VolumeMode: &incorrectVolumeMode, // Wrong: Filesystem instead of Block + StorageClassName: storageClassName, + ClaimRef: &corev1.ObjectReference{ + Kind: "PersistentVolumeClaim", + APIVersion: "v1", + Namespace: namespace, + Name: pvcName, + }, + }, + } + _, err = k8sclient.CoreV1().PersistentVolumes().Create(ctx, existingPV, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Verify PV exists with incorrect volumeMode + pvBefore, err := k8sclient.CoreV1().PersistentVolumes().Get(ctx, pvName, metav1.GetOptions{}) + assert.NoError(t, err) + assert.NotNil(t, pvBefore.Spec.VolumeMode) + assert.Equal(t, corev1.PersistentVolumeFilesystem, *pvBefore.Spec.VolumeMode) + + // Mock volumeManager for CNS untag operation + patches := gomonkey.NewPatches() + defer patches.Reset() + + // Mock DeleteVolumeUtil to simulate CNS untag (deleteDisk=false) + patches.ApplyFunc(common.DeleteVolumeUtil, + func(ctx context.Context, volumeManager cnsvolume.Manager, volumeID string, deleteDisk bool) (string, error) { + // Verify deleteDisk is false (to preserve underlying disk) + assert.False(t, deleteDisk, "deleteDisk should be false to preserve underlying volume") + return "", nil + }) + + // Create a minimal reconciler with mocked volumeManager + reconciler := &ReconcileCnsRegisterVolume{ + volumeManager: nil, // Will be mocked + } + + // Call validateAndFixPVVolumeMode + capacityInMb := int64(1024) + accessMode := corev1.ReadWriteOnce + timeout := time.Second * 10 + + pvAfter, err := validateAndFixPVVolumeMode(ctx, k8sclient, reconciler, instance, + pvBefore, pvName, volumeID, capacityInMb, accessMode, storageClassName, nil, timeout) + + // Verify the function succeeded + assert.NoError(t, err) + assert.NotNil(t, pvAfter) + + // Verify the PV was recreated with correct volumeMode + assert.NotNil(t, pvAfter.Spec.VolumeMode) + assert.Equal(t, corev1.PersistentVolumeBlock, *pvAfter.Spec.VolumeMode, + "PV should have been recreated with Block volumeMode") + + // Verify the old PV with incorrect volumeMode no longer exists in the cluster + // (by checking that the new PV returned has the correct volumeMode) + pvFinal, err := k8sclient.CoreV1().PersistentVolumes().Get(ctx, pvName, metav1.GetOptions{}) + assert.NoError(t, err) + assert.NotNil(t, pvFinal.Spec.VolumeMode) + assert.Equal(t, corev1.PersistentVolumeBlock, *pvFinal.Spec.VolumeMode) + + // Verify other PV properties are preserved + assert.Equal(t, volumeID, pvFinal.Spec.CSI.VolumeHandle) + assert.Equal(t, storageClassName, pvFinal.Spec.StorageClassName) + assert.Equal(t, corev1.ReadWriteOnce, pvFinal.Spec.AccessModes[0]) +} + +func TestPVRecreationWithoutSharedDiskEnabled(t *testing.T) { + ctx := context.Background() + k8sclient := k8sfake.NewSimpleClientset() + namespace := "test-namespace" + pvcName := "test-pvc" + pvName := "pvc-12345678-1234-1234-1234-123456789012" + volumeID := "test-volume-id" + storageClassName := "test-sc" + + // Explicitly disable shared disk feature to test that volumeMode still works + isSharedDiskEnabled = false + + // Create a PVC with DataSourceRef and volumeMode set to Block + apiGroup := "vmoperator.vmware.com" + volumeMode := corev1.PersistentVolumeBlock + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvcName, + Namespace: namespace, + UID: "12345678-1234-1234-1234-123456789012", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + DataSourceRef: &corev1.TypedObjectReference{ + APIGroup: &apiGroup, + Kind: "VirtualMachine", + Name: "test-vm", + }, + VolumeMode: &volumeMode, + StorageClassName: &storageClassName, + }, + } + _, err := k8sclient.CoreV1().PersistentVolumeClaims(namespace).Create(ctx, pvc, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Create CnsRegisterVolume with volumeMode set to Block + instance := &cnsregistervolumev1alpha1.CnsRegisterVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "register-vol", + Namespace: namespace, + }, + Spec: cnsregistervolumev1alpha1.CnsRegisterVolumeSpec{ + PvcName: pvcName, + VolumeID: volumeID, + AccessMode: corev1.ReadWriteOnce, + VolumeMode: corev1.PersistentVolumeBlock, // Expect Block + }, + } + + // Create an existing PV with INCORRECT volumeMode (Filesystem instead of Block) + incorrectVolumeMode := corev1.PersistentVolumeFilesystem + existingPV := &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + }, + Spec: corev1.PersistentVolumeSpec{ + Capacity: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Gi"), + }, + PersistentVolumeSource: corev1.PersistentVolumeSource{ + CSI: &corev1.CSIPersistentVolumeSource{ + Driver: "csi.vsphere.vmware.com", + VolumeHandle: volumeID, + }, + }, + AccessModes: []corev1.PersistentVolumeAccessMode{ + corev1.ReadWriteOnce, + }, + VolumeMode: &incorrectVolumeMode, // Wrong: Filesystem instead of Block + StorageClassName: storageClassName, + ClaimRef: &corev1.ObjectReference{ + Kind: "PersistentVolumeClaim", + APIVersion: "v1", + Namespace: namespace, + Name: pvcName, + }, + }, + } + _, err = k8sclient.CoreV1().PersistentVolumes().Create(ctx, existingPV, metav1.CreateOptions{}) + assert.NoError(t, err) + + // Mock volumeManager for CNS untag operation + patches := gomonkey.NewPatches() + defer patches.Reset() + + // Mock DeleteVolumeUtil to simulate CNS untag (deleteDisk=false) + patches.ApplyFunc(common.DeleteVolumeUtil, + func(ctx context.Context, volumeManager cnsvolume.Manager, volumeID string, deleteDisk bool) (string, error) { + assert.False(t, deleteDisk, "deleteDisk should be false to preserve underlying volume") + return "", nil + }) + + // Create a minimal reconciler with mocked volumeManager + reconciler := &ReconcileCnsRegisterVolume{ + volumeManager: nil, // Will be mocked + } + + // Call validateAndFixPVVolumeMode + capacityInMb := int64(1024) + accessMode := corev1.ReadWriteOnce + timeout := time.Second * 10 + + pvAfter, err := validateAndFixPVVolumeMode(ctx, k8sclient, reconciler, instance, + existingPV, pvName, volumeID, capacityInMb, accessMode, storageClassName, nil, timeout) + + // Verify the function succeeded + assert.NoError(t, err) + assert.NotNil(t, pvAfter) + + // Verify the PV was recreated with correct volumeMode even without shared disk enabled + assert.NotNil(t, pvAfter.Spec.VolumeMode) + assert.Equal(t, corev1.PersistentVolumeBlock, *pvAfter.Spec.VolumeMode, + "PV should have been recreated with Block volumeMode even when shared disk is disabled") + + // Verify the PV in cluster has correct volumeMode + pvFinal, err := k8sclient.CoreV1().PersistentVolumes().Get(ctx, pvName, metav1.GetOptions{}) + assert.NoError(t, err) + assert.NotNil(t, pvFinal.Spec.VolumeMode) + assert.Equal(t, corev1.PersistentVolumeBlock, *pvFinal.Spec.VolumeMode) +} diff --git a/pkg/syncer/cnsoperator/controller/cnsregistervolume/util.go b/pkg/syncer/cnsoperator/controller/cnsregistervolume/util.go index d57568c241..9a414c0a39 100644 --- a/pkg/syncer/cnsoperator/controller/cnsregistervolume/util.go +++ b/pkg/syncer/cnsoperator/controller/cnsregistervolume/util.go @@ -267,13 +267,12 @@ func getPersistentVolumeSpec(volumeName string, volumeID string, capacity int64, Status: v1.PersistentVolumeStatus{}, } - if isSharedDiskEnabled { - if volumeMode == "" { - // For both RWO and RWX volumes, default volumeMode is Filesystem. - volumeMode = v1.PersistentVolumeFilesystem - } - pv.Spec.VolumeMode = &volumeMode + // Set volumeMode if specified or default to Filesystem + if volumeMode == "" { + // For both RWO and RWX volumes, default volumeMode is Filesystem. + volumeMode = v1.PersistentVolumeFilesystem } + pv.Spec.VolumeMode = &volumeMode annotations := make(map[string]string) annotations["pv.kubernetes.io/provisioned-by"] = cnsoperatortypes.VSphereCSIDriverName