From e86b88a3f6c9d688a28c025991015f02765e97c3 Mon Sep 17 00:00:00 2001 From: Aparna Singh Date: Fri, 15 Dec 2023 20:51:21 +0530 Subject: [PATCH] Dynamically update SnapshotDir for ANF volume Extending tridentctl update volume command to ANF storage driver to allow dynamic modification of SnapshotDir of azure-netapp-files(ANF) volume/PV. Also, changing default value of SnapshotDir to true for a newly created ANF volume which uses NFSv4 protocol. --- storage/backend.go | 17 +- storage_drivers/azure/api/azure.go | 2 +- storage_drivers/azure/azure_anf.go | 117 ++++++- storage_drivers/azure/azure_anf_test.go | 322 +++++++++++++++++- storage_drivers/ontap/ontap_nas_qtree.go | 23 +- storage_drivers/ontap/ontap_nas_qtree_test.go | 75 ++-- 6 files changed, 499 insertions(+), 57 deletions(-) diff --git a/storage/backend.go b/storage/backend.go index b5d2be775..2aaa44dc1 100644 --- a/storage/backend.go +++ b/storage/backend.go @@ -124,13 +124,28 @@ type StorageBackend struct { nodeAccessUpToDate bool } -func (b *StorageBackend) UpdateVolume(ctx context.Context, volConfig *VolumeConfig, updateInfo *utils.VolumeUpdateInfo) (map[string]*Volume, error) { +func (b *StorageBackend) UpdateVolume( + ctx context.Context, volConfig *VolumeConfig, updateInfo *utils.VolumeUpdateInfo, +) (map[string]*Volume, error) { + Logc(ctx).WithFields(LogFields{ + "backend": b.name, + "volume": volConfig.Name, + "volumeInternal": volConfig.InternalName, + "updateInfo": updateInfo, + }).Debug("Attempting volume update.") + + // Ensure driver supports volume update operation volUpdateDriver, ok := b.driver.(VolumeUpdater) if !ok { return nil, errors.UnsupportedError( fmt.Sprintf("volume update is not supported for backend of type %v", b.driver.Name())) } + // Ensure volume is managed + if volConfig.ImportNotManaged { + return nil, errors.NotManagedError("source volume %s is not managed by Trident", volConfig.InternalName) + } + return volUpdateDriver.Update(ctx, volConfig, updateInfo, b.volumes) } diff --git a/storage_drivers/azure/api/azure.go b/storage_drivers/azure/api/azure.go index 01a4ba674..1210f2ce8 100644 --- a/storage_drivers/azure/api/azure.go +++ b/storage_drivers/azure/api/azure.go @@ -1077,7 +1077,7 @@ func (c Client) ModifyVolume( } // Modify the export-rule to restrict the kerberos protocol type - if len(anfVolume.Properties.ExportPolicy.Rules) > 0 && &exportRule != nil { + if len(anfVolume.Properties.ExportPolicy.Rules) > 0 && exportRule != nil { anfVolume.Properties.ExportPolicy.Rules[0].Nfsv41 = &exportRule.Nfsv41 anfVolume.Properties.ExportPolicy.Rules[0].Kerberos5ReadWrite = &exportRule.Kerberos5ReadWrite anfVolume.Properties.ExportPolicy.Rules[0].Kerberos5ReadOnly = &exportRule.Kerberos5ReadOnly diff --git a/storage_drivers/azure/azure_anf.go b/storage_drivers/azure/azure_anf.go index e20e5f1bd..badbdc9ee 100644 --- a/storage_drivers/azure/azure_anf.go +++ b/storage_drivers/azure/azure_anf.go @@ -862,6 +862,7 @@ func (d *NASStorageDriver) Create( case nfsVersion41: nfsV41Access = true protocolTypes = []string{api.ProtocolTypeNFSv41} + snapshotDirBool = true } apiExportRule = api.ExportRule{ @@ -910,7 +911,7 @@ func (d *NASStorageDriver) Create( // Update config to reflect values used to create volume volConfig.Size = strconv.FormatUint(sizeBytes, 10) volConfig.ServiceLevel = serviceLevel - volConfig.SnapshotDir = snapshotDir + volConfig.SnapshotDir = strconv.FormatBool(snapshotDirBool) volConfig.UnixPermissions = unixPermissions // Find a subnet @@ -2252,3 +2253,117 @@ func constructVolumeAccessPath( } return "" } + +func (d *NASStorageDriver) Update( + ctx context.Context, volConfig *storage.VolumeConfig, + updateInfo *utils.VolumeUpdateInfo, allVolumes map[string]*storage.Volume, +) (map[string]*storage.Volume, error) { + name := volConfig.Name + fields := LogFields{ + "Method": "Update", + "Type": "AzureNASStorageDriver", + "name": name, + "internalName": volConfig.InternalName, + "updateInfo": updateInfo, + } + Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Update") + defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Update") + + updateErrorMsg := fmt.Sprintf("Failed to update volume %v.", name) + + // Ensure there is something to update + if updateInfo == nil { + msg := fmt.Sprintf("nothing to update for volume %v", name) + err := errors.InvalidInputError(msg) + Logc(ctx).WithError(err).Error(updateErrorMsg) + return nil, err + } + + // Update resource cache as needed + if err := d.SDK.RefreshAzureResources(ctx); err != nil { + updateErr := fmt.Errorf("could not update ANF resource cache; %v", err) + Logc(ctx).WithError(updateErr).Error(updateErrorMsg) + return nil, updateErr + } + + // Get the volume + volume, err := d.SDK.Volume(ctx, volConfig) + if err != nil { + updateErr := fmt.Errorf("could not find volume %s; %v", name, err) + Logc(ctx).WithError(updateErr).Error(updateErrorMsg) + return nil, updateErr + } + + // Heal the ID on legacy volumes + if volConfig.InternalID == "" { + volConfig.InternalID = volume.ID + } + + // If the volume state isn't Available, return an error + if volume.ProvisioningState != api.StateAvailable { + updateErr := fmt.Errorf("volume %s state is %s, not %s", name, volume.ProvisioningState, api.StateAvailable) + Logc(ctx).WithError(updateErr).Error(updateErrorMsg) + return nil, updateErr + } + + var updatedVols map[string]*storage.Volume + var updateError error + + // Update snapshotDirectory for volume + if updateInfo.SnapshotDirectory != "" { + updatedVols, updateError = d.updateSnapshotDirectory(ctx, name, volume, updateInfo.SnapshotDirectory, allVolumes) + } + + return updatedVols, updateError +} + +func (d *NASStorageDriver) updateSnapshotDirectory( + ctx context.Context, name string, volume *api.FileSystem, + snapshotDir string, allVolumes map[string]*storage.Volume, +) (map[string]*storage.Volume, error) { + fields := LogFields{ + "Method": "updateSnapshotDirectory", + "Type": "AzureNASStorageDriver", + "name": name, + "snapshotDir": snapshotDir, + } + Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> updateSnapshotDirectory") + defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< updateSnapshotDirectory") + + genericLogError := fmt.Sprintf("Failed to update snapshot directory for volume %v.", name) + + // Ensure ACP is enabled + if err := acp.API().IsFeatureEnabled(ctx, acp.FeatureReadOnlyClone); err != nil { + Logc(ctx).WithFields(fields).WithError(err).Error(genericLogError) + return nil, err + } + + // Validate request + snapDirBool, err := strconv.ParseBool(snapshotDir) + if err != nil { + failureErr := errors.InvalidInputError(fmt.Sprintf("invalid value for snapshot directory %v; %v", snapshotDir, err)) + Logc(ctx).WithError(failureErr).Error(genericLogError) + return nil, failureErr + } + + // Modify volume snapshotDirectory if the current value is different from requested + if volume.SnapshotDirectory != snapDirBool { + if err = d.SDK.ModifyVolume(ctx, volume, nil, nil, &snapDirBool, nil); err != nil { + Logc(ctx).WithError(err).Error(genericLogError) + return nil, err + } + } + + // Update volConfig for the volume to ensure cache is appropriately updated + // Always do this, to ensure cache is always in consistent state with the requested value + // There could be a case where snapshotDirectory is modified directly in the backend and trident cache is not updated + var vol *storage.Volume + if vol = allVolumes[name]; vol == nil { + failureErr := fmt.Errorf("volume %v not found", name) + Logc(ctx).WithError(failureErr).Error(genericLogError) + return nil, failureErr + } + vol.Config.SnapshotDir = strconv.FormatBool(snapDirBool) + + return map[string]*storage.Volume{name: vol}, nil +} diff --git a/storage_drivers/azure/azure_anf_test.go b/storage_drivers/azure/azure_anf_test.go index 03328923b..15bf1a664 100644 --- a/storage_drivers/azure/azure_anf_test.go +++ b/storage_drivers/azure/azure_anf_test.go @@ -1534,6 +1534,7 @@ func TestCreate_NFSVolume_Kerberos_type5(t *testing.T) { createRequest.ExportPolicy.Rules[0].Nfsv3 = false createRequest.ExportPolicy.Rules[0].UnixReadWrite = false createRequest.ProtocolTypes = []string{api.ProtocolTypeNFSv41} + createRequest.SnapshotDirectory = true createRequest.UnixPermissions = "0777" createRequest.NetworkFeatures = api.NetworkFeaturesStandard @@ -1560,7 +1561,7 @@ func TestCreate_NFSVolume_Kerberos_type5(t *testing.T) { assert.Equal(t, filesystem.ID, volConfig.InternalID, "internal ID not set on volConfig") assert.Equal(t, strconv.FormatInt(createRequest.QuotaInBytes, 10), volConfig.Size, "request size mismatch") assert.Equal(t, api.ServiceLevelUltra, volConfig.ServiceLevel) - assert.Equal(t, "false", volConfig.SnapshotDir) + assert.Equal(t, "true", volConfig.SnapshotDir) assert.Equal(t, "0777", volConfig.UnixPermissions) } @@ -2330,6 +2331,7 @@ func TestCreate_NFSVolume_VolConfigMountOptions(t *testing.T) { createRequest.ProtocolTypes = []string{api.ProtocolTypeNFSv41} createRequest.ExportPolicy.Rules[0].Nfsv3 = false createRequest.ExportPolicy.Rules[0].Nfsv41 = true + createRequest.SnapshotDirectory = true mockAPI.EXPECT().RefreshAzureResources(ctx).Return(nil).Times(1) mockAPI.EXPECT().VolumeExists(ctx, volConfig).Return(false, nil, nil).Times(1) @@ -2345,6 +2347,7 @@ func TestCreate_NFSVolume_VolConfigMountOptions(t *testing.T) { assert.NoError(t, result, "create failed") assert.Equal(t, filesystem.ID, volConfig.InternalID, "internal ID not set on volConfig") + assert.Equal(t, "true", volConfig.SnapshotDir) } func TestCreate_NFSVolume_CreateFailed(t *testing.T) { @@ -6591,3 +6594,320 @@ func TestConstructVolumeAccessPath(t *testing.T) { }) } } + +func TestUpdate_Success(t *testing.T) { + defer acp.SetAPI(acp.API()) + + // Create mocks + mockCtrl := gomock.NewController(t) + mockAPI, driver := newMockANFDriver(t) + mockACP := mockacp.NewMockTridentACP(mockCtrl) + acp.SetAPI(mockACP) + + // Set up driver and populate defaults + driver.Config.BackendName = "anf" + driver.Config.ServiceLevel = api.ServiceLevelUltra + driver.Config.NetworkFeatures = api.NetworkFeaturesStandard + driver.Config.NASType = "nfs" + + driver.populateConfigurationDefaults(ctx, &driver.Config) + driver.initializeStoragePools(ctx) + driver.initializeTelemetry(ctx, BackendUUID) + + storagePool := driver.pools["anf_pool"] + + volConfig, _, _, _, filesystem := getStructsForCreateNFSVolume(ctx, driver, storagePool) + + // Set right expects + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(nil).AnyTimes() + mockAPI.EXPECT().RefreshAzureResources(ctx).Return(nil).AnyTimes() + mockAPI.EXPECT().Volume(ctx, volConfig).Return(filesystem, nil).AnyTimes() + mockAPI.EXPECT().ModifyVolume(ctx, filesystem, gomock.Any(), gomock.Any(), utils.Ptr(true), gomock.Any()).Return(nil).AnyTimes() + + updateInfo := &utils.VolumeUpdateInfo{SnapshotDirectory: "TRUE"} + allVolumes := map[string]*storage.Volume{volConfig.Name: { + Config: volConfig, + BackendUUID: BackendUUID, + Pool: "anf_pool", + Orphaned: false, + State: "Online", + }} + + result, err := driver.Update(ctx, volConfig, updateInfo, allVolumes) + + assert.Nil(t, err) + assert.NotNil(t, result) + assert.Equal(t, 1, len(result)) + for _, v := range result { + assert.Equal(t, "true", v.Config.SnapshotDir) + } +} + +func TestUpdate_NilVolumeUpdateInfo(t *testing.T) { + _, driver := newMockANFDriver(t) + + driver.Config.BackendName = "anf" + driver.Config.ServiceLevel = api.ServiceLevelUltra + driver.Config.NetworkFeatures = api.NetworkFeaturesStandard + driver.Config.NASType = "nfs" + + driver.populateConfigurationDefaults(ctx, &driver.Config) + driver.initializeStoragePools(ctx) + driver.initializeTelemetry(ctx, BackendUUID) + + storagePool := driver.pools["anf_pool"] + + volConfig, _, _, _, _ := getStructsForCreateNFSVolume(ctx, driver, storagePool) + + allVolumes := map[string]*storage.Volume{volConfig.InternalName: { + Config: volConfig, + BackendUUID: BackendUUID, + Pool: "anf_pool", + Orphaned: false, + State: "Online", + }} + + result, err := driver.Update(ctx, volConfig, nil, allVolumes) + + assert.Nil(t, result) + assert.NotNil(t, err) + assert.True(t, errors.IsInvalidInputError(err)) +} + +func TestUpdate_ErrorInAPIOperation(t *testing.T) { + defer acp.SetAPI(acp.API()) + + mockCtrl := gomock.NewController(t) + mockAPI, driver := newMockANFDriver(t) + mockACP := mockacp.NewMockTridentACP(mockCtrl) + acp.SetAPI(mockACP) + + driver.Config.BackendName = "anf" + driver.Config.ServiceLevel = api.ServiceLevelUltra + driver.Config.NetworkFeatures = api.NetworkFeaturesStandard + driver.Config.NASType = "nfs" + + driver.populateConfigurationDefaults(ctx, &driver.Config) + driver.initializeStoragePools(ctx) + driver.initializeTelemetry(ctx, BackendUUID) + + storagePool := driver.pools["anf_pool"] + + volConfig, _, _, _, _ := getStructsForCreateNFSVolume(ctx, driver, storagePool) + + mockError := errors.New("mock error") + + // CASE 1: Error in refreshing volume + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(nil).AnyTimes() + mockAPI.EXPECT().RefreshAzureResources(ctx).Return(mockError).AnyTimes() + + updateInfo := &utils.VolumeUpdateInfo{SnapshotDirectory: "TRUE"} + allVolumes := map[string]*storage.Volume{volConfig.InternalName: { + Config: volConfig, + BackendUUID: BackendUUID, + Pool: "anf_pool", + Orphaned: false, + State: "Online", + }} + + result, err := driver.Update(ctx, volConfig, updateInfo, allVolumes) + + assert.Nil(t, result) + assert.NotNil(t, err) + + // CASE 2: Error in getting volume + mockAPI, driver = newMockANFDriver(t) + mockAPI.EXPECT().RefreshAzureResources(ctx).Return(nil).AnyTimes() + mockAPI.EXPECT().Volume(ctx, volConfig).Return(nil, errors.New("error in getting volume")).AnyTimes() + + result, err = driver.Update(ctx, volConfig, updateInfo, allVolumes) + + assert.Nil(t, result) + assert.NotNil(t, err) +} + +func TestUpdate_VolumeStateNotAvailable(t *testing.T) { + mockAPI, driver := newMockANFDriver(t) + + driver.Config.BackendName = "anf" + driver.Config.ServiceLevel = api.ServiceLevelUltra + driver.Config.NetworkFeatures = api.NetworkFeaturesStandard + driver.Config.NASType = "nfs" + + driver.populateConfigurationDefaults(ctx, &driver.Config) + driver.initializeStoragePools(ctx) + driver.initializeTelemetry(ctx, BackendUUID) + + storagePool := driver.pools["anf_pool"] + + volConfig, _, _, _, filesystem := getStructsForCreateNFSVolume(ctx, driver, storagePool) + filesystem.ProvisioningState = api.StateError + + mockAPI.EXPECT().RefreshAzureResources(ctx).Return(nil).AnyTimes() + mockAPI.EXPECT().Volume(ctx, volConfig).Return(filesystem, nil).AnyTimes() + + updateInfo := &utils.VolumeUpdateInfo{SnapshotDirectory: "TRUE"} + allVolumes := map[string]*storage.Volume{volConfig.InternalName: { + Config: volConfig, + BackendUUID: BackendUUID, + Pool: "anf_pool", + Orphaned: false, + State: "Online", + }} + + result, err := driver.Update(ctx, volConfig, updateInfo, allVolumes) + + assert.Nil(t, result) + assert.NotNil(t, err) +} + +func TestUpdateSnapshotDirectory_DifferentSnapshotDir(t *testing.T) { + defer acp.SetAPI(acp.API()) + + tests := []struct { + name string + inputSnapDir string + outputSnapDir string + expectErr bool + }{ + { + name: "SnapshotDir to true", + inputSnapDir: "TRUE", + outputSnapDir: "true", + }, + { + name: "SnapshotDir to false", + inputSnapDir: "False", + outputSnapDir: "false", + }, + { + name: "Invalid SnapshotDir", + inputSnapDir: "tRUe", + expectErr: true, + }, + } + + // Create mocks + mockCtrl := gomock.NewController(t) + mockAPI, driver := newMockANFDriver(t) + mockACP := mockacp.NewMockTridentACP(mockCtrl) + acp.SetAPI(mockACP) + + // Set up driver and populate defaults + driver.Config.BackendName = "anf" + driver.Config.ServiceLevel = api.ServiceLevelUltra + driver.Config.NetworkFeatures = api.NetworkFeaturesStandard + driver.Config.NASType = "nfs" + + driver.populateConfigurationDefaults(ctx, &driver.Config) + driver.initializeStoragePools(ctx) + driver.initializeTelemetry(ctx, BackendUUID) + + storagePool := driver.pools["anf_pool"] + + volConfig, _, _, _, filesystem := getStructsForCreateNFSVolume(ctx, driver, storagePool) + + // Set right expects + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(nil).AnyTimes() + mockAPI.EXPECT().RefreshAzureResources(ctx).Return(nil).AnyTimes() + mockAPI.EXPECT().Volume(ctx, volConfig).Return(filesystem, nil).AnyTimes() + mockAPI.EXPECT().ModifyVolume(ctx, filesystem, gomock.Any(), gomock.Any(), utils.Ptr(true), gomock.Any()).Return(nil).AnyTimes() + + allVolumes := map[string]*storage.Volume{volConfig.InternalName: { + Config: volConfig, + BackendUUID: BackendUUID, + Pool: "anf_pool", + Orphaned: false, + State: "Online", + }} + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + result, err := driver.updateSnapshotDirectory( + ctx, volConfig.InternalName, filesystem, test.inputSnapDir, allVolumes) + + if test.expectErr { + assert.Nil(tt, result) + assert.NotNil(tt, err) + assert.True(tt, errors.IsInvalidInputError(err)) + } else { + assert.Nil(t, err) + assert.NotNil(tt, result) + assert.Equal(t, 1, len(result)) + for _, v := range result { + assert.Equal(t, test.outputSnapDir, v.Config.SnapshotDir) + } + } + }) + } +} + +func TestUpdateSnapshotDirectory_Failure(t *testing.T) { + defer acp.SetAPI(acp.API()) + + // Create mocks + mockCtrl := gomock.NewController(t) + mockAPI, driver := newMockANFDriver(t) + mockACP := mockacp.NewMockTridentACP(mockCtrl) + acp.SetAPI(mockACP) + + // Set up driver and populate defaults + driver.Config.BackendName = "anf" + driver.Config.ServiceLevel = api.ServiceLevelUltra + driver.Config.NetworkFeatures = api.NetworkFeaturesStandard + driver.Config.NASType = "nfs" + + driver.populateConfigurationDefaults(ctx, &driver.Config) + driver.initializeStoragePools(ctx) + driver.initializeTelemetry(ctx, BackendUUID) + + storagePool := driver.pools["anf_pool"] + + volConfig, _, _, _, filesystem := getStructsForCreateNFSVolume(ctx, driver, storagePool) + + // CASE: ACP is not enabled + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(errors.New("mock error")) + result, err := driver.updateSnapshotDirectory( + ctx, volConfig.InternalName, filesystem, "TRUE", map[string]*storage.Volume{}) + + assert.Nil(t, result) + assert.NotNil(t, err) + + // CASE: Error while modifying volume + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(nil).AnyTimes() + mockAPI.EXPECT().RefreshAzureResources(ctx).Return(nil).AnyTimes() + mockAPI.EXPECT().Volume(ctx, volConfig).Return(filesystem, nil).AnyTimes() + mockAPI.EXPECT().ModifyVolume( + ctx, filesystem, gomock.Any(), gomock.Any(), utils.Ptr(true), gomock.Any()).Return( + errors.New("mock error")).AnyTimes() + + allVolumes := map[string]*storage.Volume{volConfig.InternalName: { + Config: volConfig, + BackendUUID: BackendUUID, + Pool: "anf_pool", + Orphaned: false, + State: "Online", + }} + + result, err = driver.updateSnapshotDirectory( + ctx, volConfig.InternalName, filesystem, "TRUE", allVolumes) + + assert.Nil(t, result) + assert.NotNil(t, err) + + // CASE: Volume not found in backend cache + mockAPI, driver = newMockANFDriver(t) + mockAPI.EXPECT().RefreshAzureResources(ctx).Return(nil).AnyTimes() + mockAPI.EXPECT().Volume(ctx, volConfig).Return(filesystem, nil).AnyTimes() + mockAPI.EXPECT().ModifyVolume( + ctx, filesystem, gomock.Any(), gomock.Any(), utils.Ptr(true), gomock.Any()).Return(nil).AnyTimes() + + allVolumes = map[string]*storage.Volume{} + + filesystem.SnapshotDirectory = false + result, err = driver.updateSnapshotDirectory( + ctx, volConfig.InternalName, filesystem, "TRUE", allVolumes) + + assert.Nil(t, result) + assert.NotNil(t, err) +} diff --git a/storage_drivers/ontap/ontap_nas_qtree.go b/storage_drivers/ontap/ontap_nas_qtree.go index 181b23035..48fca4299 100644 --- a/storage_drivers/ontap/ontap_nas_qtree.go +++ b/storage_drivers/ontap/ontap_nas_qtree.go @@ -2299,11 +2299,6 @@ func (d *NASQtreeStorageDriver) Update( Logc(ctx).WithFields(fields).Debug(">>>> Update") defer Logc(ctx).WithFields(fields).Debug("<<<< Update") - if err := acp.API().IsFeatureEnabled(ctx, acp.FeatureReadOnlyClone); err != nil { - Logc(ctx).WithFields(fields).WithError(err).Error("Could not update volume.") - return nil, err - } - updateGenericError := fmt.Sprintf("failed to update volume %v", volConfig.Name) if updateInfo == nil { @@ -2335,7 +2330,7 @@ func (d *NASQtreeStorageDriver) Update( // Update snapshot directory if updateInfo.SnapshotDirectory != "" { - updatedVols, updateErr = d.setSnapshotDirectory(ctx, volConfig, updateInfo.SnapshotDirectory, updateInfo.PoolLevel, flexvol, allVolumes) + updatedVols, updateErr = d.updateSnapshotDirectory(ctx, volConfig, updateInfo.SnapshotDirectory, updateInfo.PoolLevel, flexvol, allVolumes) if updateErr != nil { Logc(ctx).WithError(updateErr).Error(updateGenericError) return nil, updateErr @@ -2352,23 +2347,29 @@ func (d *NASQtreeStorageDriver) Update( return updatedVols, updateErr } -func (d *NASQtreeStorageDriver) setSnapshotDirectory( +func (d *NASQtreeStorageDriver) updateSnapshotDirectory( ctx context.Context, volConfig *storage.VolumeConfig, snapshotDir string, poolLevel bool, poolName string, allVolumes map[string]*storage.Volume, ) (map[string]*storage.Volume, error) { fields := LogFields{ - "Method": "setSnapshotDirectory", + "Method": "updateSnapshotDirectory", "Type": "NASQtreeStorageDriver", "name": volConfig.Name, "snapshotDir": snapshotDir, "poolLevel": poolLevel, "poolName": poolName, } - Logc(ctx).WithFields(fields).Debug(">>>> setSnapshotDirectory") - defer Logc(ctx).WithFields(fields).Debug("<<<< setSnapshotDirectory") + Logc(ctx).WithFields(fields).Debug(">>>> updateSnapshotDirectory") + defer Logc(ctx).WithFields(fields).Debug("<<<< updateSnapshotDirectory") - genericErrMsg := fmt.Sprintf("failed to set snapshot directory for %v.", volConfig.Name) + genericErrMsg := fmt.Sprintf("Failed to update snapshot directory for %v.", volConfig.Name) + + // Ensure ACP is enabled + if err := acp.API().IsFeatureEnabled(ctx, acp.FeatureReadOnlyClone); err != nil { + Logc(ctx).WithFields(fields).WithError(err).Error(genericErrMsg) + return nil, err + } // Validate request snapDirRequested, err := strconv.ParseBool(snapshotDir) diff --git a/storage_drivers/ontap/ontap_nas_qtree_test.go b/storage_drivers/ontap/ontap_nas_qtree_test.go index 7056a11de..23b8f8292 100644 --- a/storage_drivers/ontap/ontap_nas_qtree_test.go +++ b/storage_drivers/ontap/ontap_nas_qtree_test.go @@ -4430,39 +4430,6 @@ func TestNASQtreeStorageDriver_UpdateVolume_Success(t *testing.T) { } } -func TestNASQtreeStorageDriver_UpdateVolume_Disabled(t *testing.T) { - // Reset the package-level state after the test completes. - defer acp.SetAPI(acp.API()) - - mockCtrl := gomock.NewController(t) - mockACP := mockacp.NewMockTridentACP(mockCtrl) - acp.SetAPI(mockACP) - - _, driver := newMockOntapNasQtreeDriver(t) - - internalID := "/svm/iscsi0/flexvol/trident_qtree_pool_trident_XHPULXSCYE/qtree/trident_pvc_99138d85_6259_4830_ada0_30e45e21f854" - mockVol := getMockVolume("pvc-99138d85-6259-4830-ada0-30e45e21f854", internalID) - mockVol.Config.SnapshotDir = "true" - - allVolumes := map[string]*storage.Volume{ - "pvc-99138d85-6259-4830-ada0-30e45e21f854": mockVol, - } - - updateInfo := &utils.VolumeUpdateInfo{ - SnapshotDirectory: "false", - PoolLevel: true, - } - - // Mock out any expected calls on the ACP API. - err := errors.UnsupportedError("unsupported") - mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(err) - - result, resultErr := driver.Update(ctx, mockVol.Config, updateInfo, allVolumes) - - assert.Error(t, resultErr) - assert.Nil(t, result) -} - func TestNASQtreeStorageDriver_UpdateVolume_Failure(t *testing.T) { // Reset the package-level state after the test completes. defer acp.SetAPI(acp.API()) @@ -4547,8 +4514,14 @@ func TestNASQtreeStorageDriver_UpdateVolume_Failure(t *testing.T) { assert.Nil(t, result) } -func TestNASQtreeStorageDriver_SetSnapshotDirectory_Success(t *testing.T) { +func TestNASQtreeStorageDriver_UpdateSnapshotDirectory_Success(t *testing.T) { + // Reset the package-level state after the test completes. + defer acp.SetAPI(acp.API()) + mockAPI, driver := newMockOntapNasQtreeDriver(t) + mockCtrl := gomock.NewController(t) + mockACP := mockacp.NewMockTridentACP(mockCtrl) + acp.SetAPI(mockACP) internalID1 := "/svm/iscsi0/flexvol/trident_qtree_pool_trident_XHPULXSCYE/qtree/trident_pvc_99138d85_6259_4830_ada0_30e45e21f854" internalID2 := "/svm/iscsi0/flexvol/trident_qtree_pool_trident_XHPULXSCYE/qtree/trident_pvc_99138d85_6259_4830_ada0_30e45e21f877" @@ -4568,9 +4541,10 @@ func TestNASQtreeStorageDriver_SetSnapshotDirectory_Success(t *testing.T) { "pvc-99138d85-6259-4830-ada0-30e45e21f843": mockVol3, } + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(nil).AnyTimes() mockAPI.EXPECT().VolumeModifySnapshotDirectoryAccess(gomock.Any(), "trident_qtree_pool_trident_XHPULXSCYE", false).Return(nil) - result, resultErr := driver.setSnapshotDirectory(ctx, mockVol1.Config, "false", true, "trident_qtree_pool_trident_XHPULXSCYE", allVolumes) + result, resultErr := driver.updateSnapshotDirectory(ctx, mockVol1.Config, "false", true, "trident_qtree_pool_trident_XHPULXSCYE", allVolumes) assert.NoError(t, resultErr) assert.NotNil(t, result) @@ -4580,8 +4554,15 @@ func TestNASQtreeStorageDriver_SetSnapshotDirectory_Success(t *testing.T) { } } -func TestNASQtreeStorageDriver_SetSnapshotDirectory_Failure(t *testing.T) { +func TestNASQtreeStorageDriver_UpdateSnapshotDirectory_Failure(t *testing.T) { + // Reset the package-level state after the test completes. + defer acp.SetAPI(acp.API()) + mockAPI, driver := newMockOntapNasQtreeDriver(t) + mockCtrl := gomock.NewController(t) + mockACP := mockacp.NewMockTridentACP(mockCtrl) + acp.SetAPI(mockACP) + fakeErr := errors.New("fake error") internalID1 := "/svm/iscsi0/flexvol/trident_qtree_pool_trident_XHPULXSCYE/qtree/trident_pvc_99138d85_6259_4830_ada0_30e45e21f854" @@ -4602,26 +4583,36 @@ func TestNASQtreeStorageDriver_SetSnapshotDirectory_Failure(t *testing.T) { "pvc-99138d85-6259-4830-ada0-30e45e21f843": mockVol3, } - // CASE 1: Invalid snapshot dir value - result, resultErr := driver.setSnapshotDirectory(ctx, mockVol1.Config, "invalid", true, "", allVolumes) + // CASE 1: ACP is not enabled for this feature + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(errors.UnsupportedError("unsupported")) + + result, resultErr := driver.updateSnapshotDirectory(ctx, mockVol1.Config, "invalid", true, "", allVolumes) + + assert.Error(t, resultErr) + assert.Nil(t, result) + + // CASE 2: Invalid snapshot dir value + mockACP.EXPECT().IsFeatureEnabled(gomock.Any(), acp.FeatureReadOnlyClone).Return(nil).AnyTimes() + + result, resultErr = driver.updateSnapshotDirectory(ctx, mockVol1.Config, "invalid", true, "", allVolumes) assert.Error(t, resultErr) assert.True(t, errors.IsInvalidInputError(resultErr)) assert.Nil(t, result) - // CASE 2: Pool level value is false - result, resultErr = driver.setSnapshotDirectory(ctx, mockVol1.Config, "false", false, "", allVolumes) + // CASE 3: Pool level value is false + result, resultErr = driver.updateSnapshotDirectory(ctx, mockVol1.Config, "false", false, "", allVolumes) assert.Error(t, resultErr) assert.True(t, errors.IsInvalidInputError(resultErr)) assert.Equal(t, fmt.Sprintf("pool level must be set to true for updating snapshot directory of %v volume", driver.Config.StorageDriverName), resultErr.Error()) assert.Nil(t, result) - // CASE 3: Error while modifying snapshot directory + // CASE 4: Error while modifying snapshot directory mockVol1.Config.InternalID = internalID1 mockAPI.EXPECT().VolumeModifySnapshotDirectoryAccess(gomock.Any(), "trident_qtree_pool_trident_XHPULXSCYE", false).Return(fakeErr) - result, resultErr = driver.setSnapshotDirectory( + result, resultErr = driver.updateSnapshotDirectory( ctx, mockVol1.Config, "false", true, "trident_qtree_pool_trident_XHPULXSCYE", allVolumes)