diff --git a/storage_drivers/ontap/ontap_nas_qtree.go b/storage_drivers/ontap/ontap_nas_qtree.go index ced589710..9ac4a2ce5 100644 --- a/storage_drivers/ontap/ontap_nas_qtree.go +++ b/storage_drivers/ontap/ontap_nas_qtree.go @@ -799,6 +799,9 @@ func (d *NASQtreeStorageDriver) Import( return nil } +// Rename renames a qtree in place. newName may also be a "flexvol/qtree" path (the +// format Import() stores as ImportOriginalName) to revert a failed import; in that +// case the FlexVol is renamed back too, but only if it holds no other qtrees. func (d *NASQtreeStorageDriver) Rename(ctx context.Context, name, newName string) error { fields := LogFields{ "Method": "Rename", @@ -809,7 +812,75 @@ func (d *NASQtreeStorageDriver) Rename(ctx context.Context, name, newName string Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Rename") defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Rename") - return errors.New("rename is not implemented") + volumePattern, qtreeName, err := d.SetVolumePatternToFindQtree(ctx, "", name, d.FlexvolNamePrefix()) + if err != nil { + return err + } + + exists, flexvol, err := d.API.QtreeExists(ctx, qtreeName, volumePattern) + if err != nil { + return fmt.Errorf("error checking for existing qtree %s: %v", name, err) + } + if !exists { + return errors.NotFoundError("qtree %s not found", name) + } + + d.flexvolLocks.Lock(flexvol) + defer d.flexvolLocks.Unlock(flexvol) + + targetFlexvol := flexvol + targetQtreeName := newName + if pathElements := strings.Split(newName, "/"); len(pathElements) == 2 { + targetFlexvol = pathElements[0] + targetQtreeName = pathElements[1] + } + + currentPath := fmt.Sprintf("/vol/%s/%s", flexvol, qtreeName) + targetPath := fmt.Sprintf("/vol/%s/%s", flexvol, targetQtreeName) + + if err := d.API.QtreeRename(ctx, currentPath, targetPath); err != nil { + return fmt.Errorf("error renaming qtree %s to %s: %v", name, targetQtreeName, err) + } + + if targetFlexvol == flexvol { + return nil + } + + // The caller also wants the FlexVol renamed back. Only do so if this FlexVol is + // exclusive to the qtree we just renamed. + count, err := d.API.QtreeCount(ctx, flexvol) + if err != nil { + Logc(ctx).WithError(err).WithField("flexvol", flexvol). + Warn("Could not verify FlexVol holds only this qtree; leaving FlexVol name unchanged.") + return nil + } + if count != 1 { + Logc(ctx).WithFields(LogFields{ + "flexvol": flexvol, + "qtreeCount": count, + }).Warn("FlexVol holds other qtrees; leaving FlexVol name unchanged to avoid orphaning them.") + return nil + } + + flexvolExists, err := d.API.VolumeExists(ctx, targetFlexvol) + if err != nil { + return fmt.Errorf("error checking for existing FlexVol %s: %v", targetFlexvol, err) + } + if flexvolExists { + Logc(ctx).WithField("flexvol", targetFlexvol). + Warn("Target FlexVol name is already in use; leaving FlexVol name unchanged.") + return nil + } + + if err := d.API.VolumeRename(ctx, flexvol, targetFlexvol); err != nil { + // Roll back the qtree rename so we don't leave a half-reverted state. + if renameErr := d.API.QtreeRename(ctx, targetPath, currentPath); renameErr != nil { + Logc(ctx).WithError(renameErr).Warn("Failed to restore qtree name after FlexVol rename failure.") + } + return fmt.Errorf("error renaming FlexVol %s to %s: %v", flexvol, targetFlexvol, err) + } + + return nil } // Destroy the volume diff --git a/storage_drivers/ontap/ontap_nas_qtree_test.go b/storage_drivers/ontap/ontap_nas_qtree_test.go index 9add2aae2..89bc30696 100644 --- a/storage_drivers/ontap/ontap_nas_qtree_test.go +++ b/storage_drivers/ontap/ontap_nas_qtree_test.go @@ -1203,10 +1203,117 @@ func TestImport_QtreeRenameFails(t *testing.T) { assert.Contains(t, err.Error(), "rename failed") } -func TestRename_NotSupported(t *testing.T) { - _, driver := newMockOntapNasQtreeDriver(t) - result := driver.Rename(ctx, "", "") - assert.Error(t, result, "Expected error in Rename, got nil") +func TestRename_Success_PlainName(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + flexvol := "userVol1" + + mockAPI.EXPECT().QtreeExists(ctx, "old-qtree", gomock.Any()).Return(true, flexvol, nil) + mockAPI.EXPECT().QtreeRename(ctx, "/vol/"+flexvol+"/old-qtree", "/vol/"+flexvol+"/new-qtree").Return(nil) + + err := driver.Rename(ctx, "old-qtree", "new-qtree") + assert.NoError(t, err) +} + +func TestRename_QtreeNotFound(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + + mockAPI.EXPECT().QtreeExists(ctx, "missing-qtree", gomock.Any()).Return(false, "", nil) + + err := driver.Rename(ctx, "missing-qtree", "new-qtree") + assert.Error(t, err) +} + +func TestRename_QtreeExistsCheckFails(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + + mockAPI.EXPECT().QtreeExists(ctx, "qtree1", gomock.Any()).Return(false, "", fmt.Errorf("api error")) + + err := driver.Rename(ctx, "qtree1", "new-qtree") + assert.Error(t, err) +} + +func TestRename_QtreeRenameFails(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + flexvol := "userVol1" + + mockAPI.EXPECT().QtreeExists(ctx, "old-qtree", gomock.Any()).Return(true, flexvol, nil) + mockAPI.EXPECT().QtreeRename(ctx, "/vol/"+flexvol+"/old-qtree", "/vol/"+flexvol+"/new-qtree"). + Return(fmt.Errorf("rename blocked")) + + err := driver.Rename(ctx, "old-qtree", "new-qtree") + assert.Error(t, err) +} + +// TestRename_ImportCleanup_RevertsFlexVol asserts that when newName is a "flexvol/qtree" path (as +// used by import-failure cleanup via ImportOriginalName) and the current FlexVol holds only the +// qtree being reverted, both the qtree and the FlexVol are renamed back to their original names. +func TestRename_ImportCleanup_RevertsFlexVol(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + currentFlexvol := "trident_qtree_pool_abc123" + originalFlexvol := "userVol1" + + mockAPI.EXPECT().QtreeExists(ctx, "pvc-123", gomock.Any()).Return(true, currentFlexvol, nil) + mockAPI.EXPECT().QtreeRename(ctx, + "/vol/"+currentFlexvol+"/pvc-123", "/vol/"+currentFlexvol+"/old-qtree").Return(nil) + mockAPI.EXPECT().QtreeCount(ctx, currentFlexvol).Return(1, nil) + mockAPI.EXPECT().VolumeExists(ctx, originalFlexvol).Return(false, nil) + mockAPI.EXPECT().VolumeRename(ctx, currentFlexvol, originalFlexvol).Return(nil) + + err := driver.Rename(ctx, "pvc-123", originalFlexvol+"/old-qtree") + assert.NoError(t, err) +} + +// TestRename_ImportCleanup_SharedFlexVolNotReverted asserts that the FlexVol is left alone when it +// holds other qtrees besides the one being reverted, so those other qtrees aren't orphaned. +func TestRename_ImportCleanup_SharedFlexVolNotReverted(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + currentFlexvol := "trident_qtree_pool_abc123" + originalFlexvol := "userVol1" + + mockAPI.EXPECT().QtreeExists(ctx, "pvc-123", gomock.Any()).Return(true, currentFlexvol, nil) + mockAPI.EXPECT().QtreeRename(ctx, + "/vol/"+currentFlexvol+"/pvc-123", "/vol/"+currentFlexvol+"/old-qtree").Return(nil) + mockAPI.EXPECT().QtreeCount(ctx, currentFlexvol).Return(2, nil) + + err := driver.Rename(ctx, "pvc-123", originalFlexvol+"/old-qtree") + assert.NoError(t, err) +} + +// TestRename_ImportCleanup_TargetFlexVolAlreadyExists asserts that the FlexVol rename is skipped +// when a FlexVol with the target name already exists, to avoid a collision. +func TestRename_ImportCleanup_TargetFlexVolAlreadyExists(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + currentFlexvol := "trident_qtree_pool_abc123" + originalFlexvol := "userVol1" + + mockAPI.EXPECT().QtreeExists(ctx, "pvc-123", gomock.Any()).Return(true, currentFlexvol, nil) + mockAPI.EXPECT().QtreeRename(ctx, + "/vol/"+currentFlexvol+"/pvc-123", "/vol/"+currentFlexvol+"/old-qtree").Return(nil) + mockAPI.EXPECT().QtreeCount(ctx, currentFlexvol).Return(1, nil) + mockAPI.EXPECT().VolumeExists(ctx, originalFlexvol).Return(true, nil) + + err := driver.Rename(ctx, "pvc-123", originalFlexvol+"/old-qtree") + assert.NoError(t, err) +} + +// TestRename_ImportCleanup_FlexVolRenameFailsRollsBackQtree asserts that if the FlexVol rename +// fails, the qtree rename is rolled back so the driver doesn't leave a half-reverted state. +func TestRename_ImportCleanup_FlexVolRenameFailsRollsBackQtree(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + currentFlexvol := "trident_qtree_pool_abc123" + originalFlexvol := "userVol1" + + mockAPI.EXPECT().QtreeExists(ctx, "pvc-123", gomock.Any()).Return(true, currentFlexvol, nil) + mockAPI.EXPECT().QtreeRename(ctx, + "/vol/"+currentFlexvol+"/pvc-123", "/vol/"+currentFlexvol+"/old-qtree").Return(nil) + mockAPI.EXPECT().QtreeCount(ctx, currentFlexvol).Return(1, nil) + mockAPI.EXPECT().VolumeExists(ctx, originalFlexvol).Return(false, nil) + mockAPI.EXPECT().VolumeRename(ctx, currentFlexvol, originalFlexvol).Return(fmt.Errorf("volume rename blocked")) + mockAPI.EXPECT().QtreeRename(ctx, + "/vol/"+currentFlexvol+"/old-qtree", "/vol/"+currentFlexvol+"/pvc-123").Return(nil) + + err := driver.Rename(ctx, "pvc-123", originalFlexvol+"/old-qtree") + assert.Error(t, err) } func TestDestroy_Success(t *testing.T) { diff --git a/storage_drivers/ontap/ontap_san_economy.go b/storage_drivers/ontap/ontap_san_economy.go index 4fd180bae..648ce33a0 100644 --- a/storage_drivers/ontap/ontap_san_economy.go +++ b/storage_drivers/ontap/ontap_san_economy.go @@ -1193,6 +1193,9 @@ func (d *SANEconomyStorageDriver) Import( return nil } +// Rename renames a LUN in place. newName may also be a "flexvol/LUN" path (the +// format Import() stores as ImportOriginalName) to revert a failed import; in that +// case the FlexVol is renamed back too, but only if it holds no other LUNs. func (d *SANEconomyStorageDriver) Rename(ctx context.Context, name, newName string) error { fields := LogFields{ "Method": "Rename", @@ -1203,7 +1206,70 @@ func (d *SANEconomyStorageDriver) Rename(ctx context.Context, name, newName stri Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Rename") defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Rename") - return errors.New("rename is not implemented") + exists, bucketVol, err := d.LUNExists(ctx, name, "", d.FlexvolNamePrefix()) + if err != nil { + return fmt.Errorf("error checking for existing LUN %s: %v", name, err) + } + if !exists { + return errors.NotFoundError("LUN %s not found", name) + } + + lockedFlexvol := d.lockFlexvol(bucketVol) + defer lockedFlexvol.Unlock() + + targetBucketVol := bucketVol + targetLUNName := newName + if pathElements := strings.Split(newName, "/"); len(pathElements) == 2 { + targetBucketVol = pathElements[0] + targetLUNName = pathElements[1] + } + + currentPath := GetLUNPathEconomy(bucketVol, name) + targetPath := GetLUNPathEconomy(bucketVol, targetLUNName) + + if err := d.API.LunRename(ctx, currentPath, targetPath); err != nil { + return fmt.Errorf("error renaming LUN %s to %s: %v", name, targetLUNName, err) + } + + if targetBucketVol == bucketVol { + return nil + } + + // The caller also wants the FlexVol renamed back. Only do so if this FlexVol is + // exclusive to the LUN we just renamed. + luns, err := d.listFlexvolLUNs(ctx, bucketVol) + if err != nil { + Logc(ctx).WithError(err).WithField("flexvol", bucketVol). + Warn("Could not verify FlexVol holds only this LUN; leaving FlexVol name unchanged.") + return nil + } + if len(luns) != 1 { + Logc(ctx).WithFields(LogFields{ + "flexvol": bucketVol, + "lunCount": len(luns), + }).Warn("FlexVol holds other LUNs; leaving FlexVol name unchanged to avoid orphaning them.") + return nil + } + + flexvolExists, err := d.API.VolumeExists(ctx, targetBucketVol) + if err != nil { + return fmt.Errorf("error checking for existing FlexVol %s: %v", targetBucketVol, err) + } + if flexvolExists { + Logc(ctx).WithField("flexvol", targetBucketVol). + Warn("Target FlexVol name is already in use; leaving FlexVol name unchanged.") + return nil + } + + if err := d.API.VolumeRename(ctx, bucketVol, targetBucketVol); err != nil { + // Roll back the LUN rename so we don't leave a half-reverted state. + if renameErr := d.API.LunRename(ctx, targetPath, currentPath); renameErr != nil { + Logc(ctx).WithError(renameErr).Warn("Failed to restore LUN name after FlexVol rename failure.") + } + return fmt.Errorf("error renaming FlexVol %s to %s: %v", bucketVol, targetBucketVol, err) + } + + return nil } // Destroy the LUN diff --git a/storage_drivers/ontap/ontap_san_economy_test.go b/storage_drivers/ontap/ontap_san_economy_test.go index 4fe440d6d..3c1df2ec6 100644 --- a/storage_drivers/ontap/ontap_san_economy_test.go +++ b/storage_drivers/ontap/ontap_san_economy_test.go @@ -2342,12 +2342,110 @@ func TestOntapSanEconomyVolumeImport_ManagedNoRename(t *testing.T) { } } -func TestOntapSanEconomyVolumeRename(t *testing.T) { - _, d := newMockOntapSanEcoDriver(t) +func TestOntapSanEconomyVolumeRename_Success_PlainName(t *testing.T) { + mockAPI, d := newMockOntapSanEcoDriver(t) + bucketVol := "userVol1" + lunPathPattern := fmt.Sprintf("/vol/%s*/volInternal", d.FlexvolNamePrefix()) + + mockAPI.EXPECT().LunList(ctx, lunPathPattern).Return( + api.Luns{{Name: "/vol/" + bucketVol + "/volInternal", VolumeName: bucketVol}}, nil) + mockAPI.EXPECT().LunRename(ctx, + "/vol/"+bucketVol+"/volInternal", "/vol/"+bucketVol+"/newVolInternal").Return(nil) + + err := d.Rename(ctx, "volInternal", "newVolInternal") + assert.NoError(t, err) +} + +func TestOntapSanEconomyVolumeRename_LUNNotFound(t *testing.T) { + mockAPI, d := newMockOntapSanEcoDriver(t) + lunPathPattern := fmt.Sprintf("/vol/%s*/volInternal", d.FlexvolNamePrefix()) + + mockAPI.EXPECT().LunList(ctx, lunPathPattern).Return(api.Luns{}, nil) err := d.Rename(ctx, "volInternal", "newVolInternal") + assert.Error(t, err) +} + +func TestOntapSanEconomyVolumeRename_LUNRenameFails(t *testing.T) { + mockAPI, d := newMockOntapSanEcoDriver(t) + bucketVol := "userVol1" + lunPathPattern := fmt.Sprintf("/vol/%s*/volInternal", d.FlexvolNamePrefix()) - assert.EqualError(t, err, "rename is not implemented") + mockAPI.EXPECT().LunList(ctx, lunPathPattern).Return( + api.Luns{{Name: "/vol/" + bucketVol + "/volInternal", VolumeName: bucketVol}}, nil) + mockAPI.EXPECT().LunRename(ctx, + "/vol/"+bucketVol+"/volInternal", "/vol/"+bucketVol+"/newVolInternal").Return(fmt.Errorf("rename blocked")) + + err := d.Rename(ctx, "volInternal", "newVolInternal") + assert.Error(t, err) +} + +// TestOntapSanEconomyVolumeRename_ImportCleanup_RevertsFlexVol asserts that when newName is a +// "flexvol/LUN" path (as used by import-failure cleanup via ImportOriginalName) and the current +// FlexVol holds only the LUN being reverted, both the LUN and the FlexVol are renamed back. +func TestOntapSanEconomyVolumeRename_ImportCleanup_RevertsFlexVol(t *testing.T) { + mockAPI, d := newMockOntapSanEcoDriver(t) + currentFlexvol := "trident_pool_abc123" + originalFlexvol := "userVol1" + lunPathPattern := fmt.Sprintf("/vol/%s*/pvc-123", d.FlexvolNamePrefix()) + + mockAPI.EXPECT().LunList(ctx, lunPathPattern).Return( + api.Luns{{Name: "/vol/" + currentFlexvol + "/pvc-123", VolumeName: currentFlexvol}}, nil) + mockAPI.EXPECT().LunRename(ctx, + "/vol/"+currentFlexvol+"/pvc-123", "/vol/"+currentFlexvol+"/old-lun").Return(nil) + mockAPI.EXPECT().LunList(ctx, fmt.Sprintf("/vol/%s/*", currentFlexvol)).Return( + api.Luns{{Name: "/vol/" + currentFlexvol + "/old-lun", VolumeName: currentFlexvol}}, nil) + mockAPI.EXPECT().VolumeExists(ctx, originalFlexvol).Return(false, nil) + mockAPI.EXPECT().VolumeRename(ctx, currentFlexvol, originalFlexvol).Return(nil) + + err := d.Rename(ctx, "pvc-123", originalFlexvol+"/old-lun") + assert.NoError(t, err) +} + +// TestOntapSanEconomyVolumeRename_ImportCleanup_SharedFlexVolNotReverted asserts that the FlexVol +// is left alone when it holds other LUNs besides the one being reverted. +func TestOntapSanEconomyVolumeRename_ImportCleanup_SharedFlexVolNotReverted(t *testing.T) { + mockAPI, d := newMockOntapSanEcoDriver(t) + currentFlexvol := "trident_pool_abc123" + originalFlexvol := "userVol1" + lunPathPattern := fmt.Sprintf("/vol/%s*/pvc-123", d.FlexvolNamePrefix()) + + mockAPI.EXPECT().LunList(ctx, lunPathPattern).Return( + api.Luns{{Name: "/vol/" + currentFlexvol + "/pvc-123", VolumeName: currentFlexvol}}, nil) + mockAPI.EXPECT().LunRename(ctx, + "/vol/"+currentFlexvol+"/pvc-123", "/vol/"+currentFlexvol+"/old-lun").Return(nil) + mockAPI.EXPECT().LunList(ctx, fmt.Sprintf("/vol/%s/*", currentFlexvol)).Return( + api.Luns{ + {Name: "/vol/" + currentFlexvol + "/old-lun", VolumeName: currentFlexvol}, + {Name: "/vol/" + currentFlexvol + "/other-lun", VolumeName: currentFlexvol}, + }, nil) + + err := d.Rename(ctx, "pvc-123", originalFlexvol+"/old-lun") + assert.NoError(t, err) +} + +// TestOntapSanEconomyVolumeRename_ImportCleanup_FlexVolRenameFailsRollsBackLUN asserts that if the +// FlexVol rename fails, the LUN rename is rolled back so the driver doesn't leave a half-reverted +// state. +func TestOntapSanEconomyVolumeRename_ImportCleanup_FlexVolRenameFailsRollsBackLUN(t *testing.T) { + mockAPI, d := newMockOntapSanEcoDriver(t) + currentFlexvol := "trident_pool_abc123" + originalFlexvol := "userVol1" + lunPathPattern := fmt.Sprintf("/vol/%s*/pvc-123", d.FlexvolNamePrefix()) + + mockAPI.EXPECT().LunList(ctx, lunPathPattern).Return( + api.Luns{{Name: "/vol/" + currentFlexvol + "/pvc-123", VolumeName: currentFlexvol}}, nil) + mockAPI.EXPECT().LunRename(ctx, + "/vol/"+currentFlexvol+"/pvc-123", "/vol/"+currentFlexvol+"/old-lun").Return(nil) + mockAPI.EXPECT().LunList(ctx, fmt.Sprintf("/vol/%s/*", currentFlexvol)).Return( + api.Luns{{Name: "/vol/" + currentFlexvol + "/old-lun", VolumeName: currentFlexvol}}, nil) + mockAPI.EXPECT().VolumeExists(ctx, originalFlexvol).Return(false, nil) + mockAPI.EXPECT().VolumeRename(ctx, currentFlexvol, originalFlexvol).Return(fmt.Errorf("volume rename blocked")) + mockAPI.EXPECT().LunRename(ctx, + "/vol/"+currentFlexvol+"/old-lun", "/vol/"+currentFlexvol+"/pvc-123").Return(nil) + + err := d.Rename(ctx, "pvc-123", originalFlexvol+"/old-lun") + assert.Error(t, err) } func economyMainLUNPath(bucketVol string) string {