Skip to content

Commit

Permalink
Handle volume import mismatch when 2 GCNV Volumes have same name
Browse files Browse the repository at this point in the history
  • Loading branch information
@alloydsa
alloydsa authored Feb 4, 2025
1 parent a4fc3fa commit 0cb47a9
Show file tree
Hide file tree
Showing 5 changed files with 158 additions and 24 deletions.
15 changes: 15 additions & 0 deletions mocks/mock_storage_drivers/mock_gcp/mock_gcnvapi.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 25 additions & 4 deletions storage_drivers/gcp/gcnvapi/gcnv.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,18 +478,18 @@ func (c Client) VolumeByName(ctx context.Context, name string) (*Volume, error)
flexPoolsCount := c.flexPoolCount()
locations := c.findAllLocationsFromCapacityPool(flexPoolsCount)

gcnvVolumes := make(map[string]*netapppb.Volume)
var gcnvVolume *netapppb.Volume
var err error
// We iterate over a list of region and zones to check if the volume is in that region or zone.
// We use this logic to reduce the number of API calls to the GCNV API
for location := range locations {

req := &netapppb.GetVolumeRequest{
Name: c.createVolumeID(location, name),
}
gcnvVolume, err = c.sdkClient.gcnv.GetVolume(sdkCtx, req)
if gcnvVolume != nil {
break
gcnvVolumes[gcnvVolume.Name] = gcnvVolume
} else if err != nil {
if IsGCNVNotFoundError(err) {
Logc(ctx).WithFields(logFields).Debugf("Volume not found in '%s' location.", location)
Expand All @@ -500,14 +500,19 @@ func (c Client) VolumeByName(ctx context.Context, name string) (*Volume, error)
}
}

if gcnvVolume == nil {
if len(gcnvVolumes) == 0 {
if IsGCNVNotFoundError(err) {
return nil, errors.WrapWithNotFoundError(err, "volume '%s' not found", name)
}
return nil, err
} else if len(gcnvVolumes) > 1 {
return nil, errors.New("found multiple volumes with the same name in the given location")
}
Logd(ctx, c.config.StorageDriverName, c.config.DebugTraceFlags["api"]).
WithFields(logFields).Debug("Found volume by name.")
for _, volume := range gcnvVolumes {
gcnvVolume = volume
}

return c.newVolumeFromGCNVVolume(ctx, gcnvVolume)
}
Expand Down Expand Up @@ -543,7 +548,7 @@ func (c Client) VolumeByID(ctx context.Context, id string) (*Volume, error) {
}

Logd(ctx, c.config.StorageDriverName, c.config.DebugTraceFlags["api"]).
WithFields(logFields).Trace("Fetching volume by id.")
WithFields(logFields).Trace("Fetching volume by volumeID.")

sdkCtx, sdkCancel := context.WithTimeout(ctx, c.config.SDKTimeout)
defer sdkCancel()
Expand Down Expand Up @@ -862,6 +867,22 @@ func (c Client) DeleteVolume(ctx context.Context, volume *Volume) error {
return nil
}

// VolumeByNameOrID retrieves a gcnv volume for import by volumeName or volumeID
func (c Client) VolumeByNameOrID(ctx context.Context, volumeID string) (*Volume, error) {
var volume *Volume
var err error
match := volumeNameRegex.FindStringSubmatch(volumeID)
if match == nil {
volume, err = c.VolumeByName(ctx, volumeID)
} else {
volume, err = c.VolumeByID(ctx, volumeID)
}
if err != nil {
return nil, err
}
return volume, nil
}

// ///////////////////////////////////////////////////////////////////////////////
// Functions to retrieve and manage snapshots
// ///////////////////////////////////////////////////////////////////////////////
Expand Down
1 change: 1 addition & 0 deletions storage_drivers/gcp/gcnvapi/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type GCNV interface {
VolumeExistsByName(context.Context, string) (bool, *Volume, error)
VolumeByID(context.Context, string) (*Volume, error)
VolumeExistsByID(context.Context, string) (bool, *Volume, error)
VolumeByNameOrID(context.Context, string) (*Volume, error)
WaitForVolumeState(context.Context, *Volume, string, []string, time.Duration) (string, error)
CreateVolume(context.Context, *VolumeCreateRequest) (*Volume, error)
ModifyVolume(context.Context, *Volume, map[string]string, *string, *bool, *ExportRule) error
Expand Down
8 changes: 5 additions & 3 deletions storage_drivers/gcp/gcp_gcnv.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1102,11 +1102,13 @@ func (d *NASStorageDriver) Import(ctx context.Context, volConfig *storage.Volume
}

// Get the volume
volume, err := d.API.VolumeByName(ctx, originalName)
volume, err := d.API.VolumeByNameOrID(ctx, originalName)
if err != nil {
return fmt.Errorf("could not find volume %s; %v", originalName, err)
}

if volume.State != gcnvapi.VolumeStateReady {
return fmt.Errorf("volume %s is in state %s and is not available", originalName, volume.State)
}
// Don't allow import for dual-protocol volume.
// For dual-protocol volume the ProtocolTypes has two values [NFSv3, CIFS]
if d.isDualProtocolVolume(volume) {
Expand Down Expand Up @@ -2118,7 +2120,7 @@ func (d *NASStorageDriver) GetVolumeForImport(ctx context.Context, volumeID stri
return nil, fmt.Errorf("could not update GCNV resource cache; %v", err)
}

filesystem, err := d.API.VolumeByName(ctx, volumeID)
filesystem, err := d.API.VolumeByNameOrID(ctx, volumeID)
if err != nil {
return nil, err
}
Expand Down
129 changes: 112 additions & 17 deletions storage_drivers/gcp/gcp_gcnv_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3660,7 +3660,7 @@ func TestImport_Managed(t *testing.T) {
expectedUnixPermissions := "0770"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, &expectedUnixPermissions, &snapshotDirAccess, nil).Return(nil).Times(1)
Expand All @@ -3675,6 +3675,43 @@ func TestImport_Managed(t *testing.T) {
assert.Equal(t, originalVolume.FullName, volConfig.InternalID, "internal ID not set on volConfig")
}

func TestImport_ManagedVolumeFullName(t *testing.T) {
mockAPI, driver := newMockGCNVDriver(t)

driver.Config.BackendName = "gcnv"
driver.Config.ServiceLevel = gcnvapi.ServiceLevelPremium
driver.Config.NASType = "nfs"
driver.Config.UnixPermissions = "0770"

err := driver.populateConfigurationDefaults(ctx, &driver.Config)
assert.NoError(t, err, "error occurred")

driver.initializeStoragePools(ctx)
driver.initializeTelemetry(ctx, gcnvapi.BackendUUID)

volConfig, originalVolume := getStructsForNFSImport(ctx, driver)

expectedLabels := driver.updateTelemetryLabels(ctx, originalVolume)
var snapshotDirAccess bool
originalFullName := "projects/123456789/locations/fake-location/volumes/testvol1"
expectedUnixPermissions := "0770"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalFullName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, &expectedUnixPermissions, &snapshotDirAccess, nil).Return(nil).Times(1)

mockAPI.EXPECT().WaitForVolumeState(ctx, originalVolume, gcnvapi.VolumeStateReady, []string{gcnvapi.VolumeStateError},
driver.defaultTimeout()).Return(gcnvapi.VolumeStateReady, nil).Times(1)

result := driver.Import(ctx, volConfig, originalFullName)

assert.NoError(t, result, "import failed")
assert.Equal(t, originalVolume.Name, volConfig.Name, "internal name mismatch")
assert.Equal(t, originalFullName, volConfig.InternalID, "internal ID not set on volConfig")
}

func TestImport_ManagedWithSnapshotDir(t *testing.T) {
mockAPI, driver := newMockGCNVDriver(t)

Expand All @@ -3699,7 +3736,7 @@ func TestImport_ManagedWithSnapshotDir(t *testing.T) {
snapshotDirAccess := true

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, &expectedUnixPermissions, &snapshotDirAccess, nil).Return(nil).Times(1)
Expand Down Expand Up @@ -3738,7 +3775,7 @@ func TestImport_ManagedWithSnapshotDirFalse(t *testing.T) {
snapshotDirAccess := false

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, &expectedUnixPermissions, &snapshotDirAccess, nil).Return(nil).Times(1)
Expand Down Expand Up @@ -3773,7 +3810,7 @@ func TestImport_ManagedWithInvalidSnapshotDirValue(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

result := driver.Import(ctx, volConfig, originalName)
Expand Down Expand Up @@ -3802,7 +3839,7 @@ func TestImport_SMB_Managed(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, nil, &snapshotDirAccess, nil).Return(nil).Times(1)
Expand Down Expand Up @@ -3837,7 +3874,7 @@ func TestImport_SMB_Failed(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, nil, &snapshotDirAccess, nil).Return(errFailed).Times(1)
Expand Down Expand Up @@ -3868,7 +3905,7 @@ func TestImport_DualProtocolVolume(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)

result := driver.Import(ctx, volConfig, originalName)

Expand All @@ -3895,7 +3932,7 @@ func TestImport_CapacityPoolError(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(errFailed).Times(1)

result := driver.Import(ctx, volConfig, originalName)
Expand Down Expand Up @@ -3924,7 +3961,7 @@ func TestImport_NotManaged(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

result := driver.Import(ctx, volConfig, originalName)
Expand All @@ -3934,6 +3971,36 @@ func TestImport_NotManaged(t *testing.T) {
assert.Equal(t, originalVolume.FullName, volConfig.InternalID, "internal ID not set on volConfig")
}

func TestImport_NotManagedVolumeFullName(t *testing.T) {
mockAPI, driver := newMockGCNVDriver(t)

driver.Config.BackendName = "gcnv"
driver.Config.ServiceLevel = gcnvapi.ServiceLevelPremium
driver.Config.NASType = "nfs"
driver.Config.UnixPermissions = "0770"

err := driver.populateConfigurationDefaults(ctx, &driver.Config)
assert.NoError(t, err, "error occurred")

driver.initializeStoragePools(ctx)
driver.initializeTelemetry(ctx, gcnvapi.BackendUUID)

volConfig, originalVolume := getStructsForNFSImport(ctx, driver)

volConfig.ImportNotManaged = true
originalFullName := "projects/123456789/locations/fake-location/volumes/testvol1"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalFullName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

result := driver.Import(ctx, volConfig, originalFullName)

assert.NoError(t, result, "import failed")
assert.Equal(t, originalVolume.Name, volConfig.Name, "internal name mismatch")
assert.Equal(t, originalFullName, volConfig.InternalID, "internal ID not set on volConfig")
}

func TestImport_DiscoveryFailed(t *testing.T) {
mockAPI, driver := newMockGCNVDriver(t)

Expand Down Expand Up @@ -3981,7 +4048,7 @@ func TestImport_NotFound(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(nil, errFailed).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(nil, errFailed).Times(1)

result := driver.Import(ctx, volConfig, originalName)

Expand All @@ -3990,6 +4057,34 @@ func TestImport_NotFound(t *testing.T) {
assert.Equal(t, "", volConfig.InternalID, "internal ID set on volConfig")
}

func TestImport_DuplicateVolumes(t *testing.T) {
mockAPI, driver := newMockGCNVDriver(t)

driver.Config.BackendName = "gcnv"
driver.Config.ServiceLevel = gcnvapi.ServiceLevelPremium
driver.Config.NASType = "nfs"
driver.Config.UnixPermissions = "0770"

err := driver.populateConfigurationDefaults(ctx, &driver.Config)
assert.NoError(t, err, "error occurred")

driver.initializeStoragePools(ctx)
driver.initializeTelemetry(ctx, gcnvapi.BackendUUID)

volConfig, _ := getStructsForNFSImport(ctx, driver)

originalName := "importMe"

// Mock API to return an error indicating duplicate volumes
mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(nil, errors.New("found multiple volumes with the same name in the given location")).Times(1)

result := driver.Import(ctx, volConfig, originalName)

assert.Error(t, result, "expected error")
assert.Contains(t, result.Error(), "found multiple volumes with the same name in the given location", "unexpected error message")
}

func TestImport_InvalidUnixPermissions(t *testing.T) {
mockAPI, driver := newMockGCNVDriver(t)

Expand All @@ -4009,7 +4104,7 @@ func TestImport_InvalidUnixPermissions(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

result := driver.Import(ctx, volConfig, originalName)
Expand Down Expand Up @@ -4041,7 +4136,7 @@ func TestImport_ModifyVolumeFailed(t *testing.T) {
snapshotDirAccess := false

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, &expectedUnixPermissions, &snapshotDirAccess, nil).Return(errFailed).Times(1)
Expand Down Expand Up @@ -4075,7 +4170,7 @@ func TestImport_VolumeWaitFailed(t *testing.T) {
snapshotDirAccess := false

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

mockAPI.EXPECT().ModifyVolume(ctx, originalVolume, expectedLabels, &expectedUnixPermissions, &snapshotDirAccess, nil).Return(nil).Times(1)
Expand Down Expand Up @@ -4109,7 +4204,7 @@ func TestImport_BackendVolumeMismatch(t *testing.T) {
originalName := "importMe"

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, originalName).Return(originalVolume, nil).Times(1)
mockAPI.EXPECT().EnsureVolumeInValidCapacityPool(ctx, originalVolume).Return(nil).Times(1)

result := driver.Import(ctx, volConfig, originalName)
Expand Down Expand Up @@ -6658,7 +6753,7 @@ func TestGCNVGetVolumeForImport(t *testing.T) {
}

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, "testvol1").Return(volume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, "testvol1").Return(volume, nil).Times(1)

result, resultErr := driver.GetVolumeForImport(ctx, "testvol1")

Expand All @@ -6683,7 +6778,7 @@ func TestGCNVGetVolumeForImport_VolumeNameWithPrefix(t *testing.T) {
}

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, "testvol1").Return(volume, nil).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, "testvol1").Return(volume, nil).Times(1)

result, resultErr := driver.GetVolumeForImport(ctx, "testvol1")

Expand Down Expand Up @@ -6715,7 +6810,7 @@ func TestGetVolumeForImport_GetFailed(t *testing.T) {
driver.Config.StoragePrefix = &storagePrefix

mockAPI.EXPECT().RefreshGCNVResources(ctx).Return(nil).Times(1)
mockAPI.EXPECT().VolumeByName(ctx, "testvol1").Return(nil, errFailed).Times(1)
mockAPI.EXPECT().VolumeByNameOrID(ctx, "testvol1").Return(nil, errFailed).Times(1)

result, resultErr := driver.GetVolumeForImport(ctx, "testvol1")

Expand Down

0 comments on commit 0cb47a9

Please sign in to comment.