From b23cefdfb6c8b71ac8f6732914c62750808e63a8 Mon Sep 17 00:00:00 2001 From: mravi-na Date: Tue, 16 Jan 2024 10:54:33 +0530 Subject: [PATCH] Adding unit tests and configurable parameter for clone split delay. --- storage_drivers/ontap/ontap_common.go | 28 +++++++-------- storage_drivers/ontap/ontap_common_test.go | 42 ++++++++++++++++++++++ storage_drivers/types.go | 2 +- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/storage_drivers/ontap/ontap_common.go b/storage_drivers/ontap/ontap_common.go index 0c430c152..6d1cfce9e 100644 --- a/storage_drivers/ontap/ontap_common.go +++ b/storage_drivers/ontap/ontap_common.go @@ -108,8 +108,6 @@ const ( StateReasonSVMUnreachable = "SVM is not reachable" ) -var CloneSplitDelay = 10 * time.Second - // CleanBackendName removes brackets and replaces colons with periods to avoid regex parsing errors. func CleanBackendName(backendName string) string { backendName = strings.ReplaceAll(backendName, "[", "") @@ -1226,6 +1224,7 @@ const ( DefaultNfsMountOptionsKubernetes = "" DefaultSplitOnClone = "false" DefaultCloneSplitDelay = "10" + DefaultCloneSplitDelayValue = 10 DefaultLuksEncryption = "false" DefaultMirroring = "false" DefaultLimitAggregateUsage = "" @@ -1319,9 +1318,9 @@ func PopulateConfigurationDefaults(ctx context.Context, config *drivers.OntapSto if config.CloneSplitDelay == "" { config.CloneSplitDelay = DefaultCloneSplitDelay } else { - _, err := strconv.ParseInt(config.CloneSplitDelay, 10, 64) - if err != nil { - return fmt.Errorf("invalid integer value for cloneSplitDelay: %v", err) + v, err := strconv.ParseInt(config.CloneSplitDelay, 10, 0) + if err != nil || v <= 0 { + return fmt.Errorf("invalid value for cloneSplitDelay: %v", config.CloneSplitDelay) } } @@ -1654,13 +1653,14 @@ func SplitVolumeFromBusySnapshotWithDelay( cloneSplitTimers map[string]time.Time, ) { snapshotID := snapConfig.ID() - if config.CloneSplitDelay != "" { - cloneSplitDelayDuration, err := strconv.ParseInt(config.CloneSplitDelay, 10, 0) - if err == nil { - // Valid value provided, use it - CloneSplitDelay = time.Duration(cloneSplitDelayDuration) * time.Second - } + + // Find the configured value for cloneSplitDelay + cfgVal, err := strconv.ParseInt(config.CloneSplitDelay, 10, 0) + if err != nil || cfgVal < 0 { + // Should not come here, but just in case. + cfgVal = DefaultCloneSplitDelayValue } + cloneSplitDelay := time.Duration(cfgVal) * time.Second // If this is the first delete, just log the time and return. firstDeleteTime, ok := cloneSplitTimers[snapshotID] @@ -1669,7 +1669,7 @@ func SplitVolumeFromBusySnapshotWithDelay( Logc(ctx).WithFields(LogFields{ "snapshot": snapshotID, - "secondsBeforeSplit": fmt.Sprintf("%3.2f", CloneSplitDelay.Seconds()), + "secondsBeforeSplit": fmt.Sprintf("%3.2f", cloneSplitDelay.Seconds()), }).Warning("Initial locked snapshot delete, starting clone split timer.") return @@ -1686,7 +1686,7 @@ func SplitVolumeFromBusySnapshotWithDelay( } // This isn't the first delete, and the delay has not expired, so there is nothing to do. - if time.Now().Sub(firstDeleteTime) < CloneSplitDelay { + if time.Now().Sub(firstDeleteTime) < cloneSplitDelay { Logc(ctx).WithFields(LogFields{ "snapshot": snapshotID, @@ -1705,7 +1705,7 @@ func SplitVolumeFromBusySnapshotWithDelay( Logc(ctx).WithFields(LogFields{ "snapshot": snapshotID, - "secondsBeforeSplit": fmt.Sprintf("%3.2f", CloneSplitDelay.Seconds()), + "secondsBeforeSplit": fmt.Sprintf("%3.2f", cloneSplitDelay.Seconds()), }).Warning("Retried locked snapshot delete, clone split failed, restarted timer.") } else { diff --git a/storage_drivers/ontap/ontap_common_test.go b/storage_drivers/ontap/ontap_common_test.go index 3402e8694..4db0624fd 100644 --- a/storage_drivers/ontap/ontap_common_test.go +++ b/storage_drivers/ontap/ontap_common_test.go @@ -4734,6 +4734,42 @@ func TestSplitVolumeFromBusySnapshot(t *testing.T) { assert.NoError(t, err) } +func TestSplitVolumeFromBusySnapshotWithDelay(t *testing.T) { + ctx := context.Background() + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + commonConfig := &drivers.CommonStorageDriverConfig{ + DebugTraceFlags: map[string]bool{"method": true}, + } + config := &drivers.OntapStorageDriverConfig{ + CommonStorageDriverConfig: commonConfig, + } + snapConfig := &storage.SnapshotConfig{ + VolumeInternalName: "fakeVolInternalName", + InternalName: "fakeInternalName", + } + + // Test 1: No snap found + cloneSplitTimers := make(map[string]time.Time) + SplitVolumeFromBusySnapshotWithDelay(ctx, snapConfig, config, mockAPI, mockCloneSplitStart, cloneSplitTimers) + + // Test 2: time difference is < 0 + cloneSplitTimers[snapConfig.ID()] = time.Now().Add(10 * time.Millisecond) + SplitVolumeFromBusySnapshotWithDelay(ctx, snapConfig, config, mockAPI, mockCloneSplitStart, cloneSplitTimers) + + time.Sleep(50 * time.Millisecond) + // Test 3: time difference < config.CloneSplitDelay, which is defaulted to 10 seconds. + SplitVolumeFromBusySnapshotWithDelay(ctx, snapConfig, config, mockAPI, mockCloneSplitStart, cloneSplitTimers) + + // Test 4: SplitVolumeFromBusySnapshot returning error + // Add the time for first delete in past so that SplitVolume is called. + cloneSplitTimers[snapConfig.ID()] = time.Now().Add(-10 * time.Second) + mockAPI.EXPECT().VolumeListBySnapshotParent(ctx, snapConfig.InternalName, + snapConfig.VolumeInternalName).Return(api.VolumeNameList{}, + fmt.Errorf("error returned by VolumeListBySnapshotParent")) + SplitVolumeFromBusySnapshotWithDelay(ctx, snapConfig, config, mockAPI, mockCloneSplitStart, cloneSplitTimers) +} + func TestGetVolumeExternalCommon(t *testing.T) { volume := api.Volume{ Name: "fakeVolume", @@ -5122,6 +5158,12 @@ func TestPopulateConfigurationDefaults(t *testing.T) { err = PopulateConfigurationDefaults(ctx, config) assert.Error(t, err) + + // Test 8 - Invalid value for cloneSplitDelay + config.SplitOnClone = "true" + config.CloneSplitDelay = "-123" + err = PopulateConfigurationDefaults(ctx, config) + assert.Error(t, err) } func TestNewOntapTelemetry(t *testing.T) { diff --git a/storage_drivers/types.go b/storage_drivers/types.go index d6164affa..1a057b057 100644 --- a/storage_drivers/types.go +++ b/storage_drivers/types.go @@ -119,7 +119,7 @@ type OntapStorageDriverConfig struct { QtreesPerFlexvol string `json:"qtreesPerFlexvol"` // default to 200 LUNsPerFlexvol string `json:"lunsPerFlexvol"` // default to 100 EmptyFlexvolDeferredDeletePeriod string `json:"emptyFlexvolDeferredDeletePeriod"` // in seconds, default to 28800 - CloneSplitDelay string `json:"cloneSplitDelay"` // in seconds, default to 10 + CloneSplitDelay string `json:"cloneSplitDelay,omitEmpty"` // in seconds, default to 10 NfsMountOptions string `json:"nfsMountOptions"` LimitAggregateUsage string `json:"limitAggregateUsage"` AutoExportPolicy bool `json:"autoExportPolicy"`