GCNV NAS: hyphen-to-underscore normalization for volume IDs

Author: onkarganjewar

storage_drivers/gcp/api/gcnv.go

@@ -479,6 +479,12 @@ func (c Client) newVolumeFromGCNVVolume(ctx context.Context, volume *netapppb.Vo
 		}
 	}
 
+	cPool := c.capacityPool(volume.StoragePool)
+	storagePoolType := ""
+	if cPool != nil {
+		storagePoolType = cPool.PoolType
+	}
+
 	result := &Volume{
 		Name:                      volumeName,
 		CreationToken:             volume.ShareName,
@@ -488,7 +494,8 @@ func (c Client) newVolumeFromGCNVVolume(ctx context.Context, volume *netapppb.Vo
 		CapacityPool:              volume.StoragePool,
 		NetworkName:               network,
 		NetworkFullName:           volume.Network,
-		ServiceLevel:              ServiceLevelFromCapacityPool(c.capacityPool(volume.StoragePool)),
+		ServiceLevel:              ServiceLevelFromCapacityPool(cPool),
+		StoragePoolType:           storagePoolType,
 		SizeBytes:                 volume.CapacityGib * GiBBytes,
 		ExportPolicy:              c.exportPolicyImport(volume.ExportPolicy),
 		ProtocolTypes:             protocolTypes,
@@ -651,14 +658,38 @@ func (c Client) Volumes(ctx context.Context) (*[]*Volume, error) {
 }
 
 // Volume uses a volume config record to fetch a volume by the most efficient means.
+func gcnvVolumeFallbackNames(volConfig *storage.VolumeConfig) (string, string) {
+	primary := volConfig.InternalName
+	secondary := strings.ReplaceAll(volConfig.Name, "-", "_")
+	if secondary == primary {
+		secondary = ""
+	}
+	return primary, secondary
+}
+
 func (c Client) Volume(ctx context.Context, volConfig *storage.VolumeConfig) (*Volume, error) {
 	// When we know the internal ID, use that as it is vastly more efficient
 	if volConfig.InternalID != "" {
 		return c.VolumeByID(ctx, volConfig.InternalID)
 	}
 
-	// Fall back to the name
-	return c.VolumeByName(ctx, volConfig.InternalName)
+	primaryName, secondaryName := gcnvVolumeFallbackNames(volConfig)
+
+	// Fall back to the creation token.
+	volume, err := c.VolumeByName(ctx, primaryName)
+	if err == nil {
+		return volume, nil
+	}
+	if !errors.IsNotFoundError(err) {
+		return nil, err
+	}
+
+	// For UNIFIED naming, also try underscore-normalized display name.
+	if secondaryName == "" {
+		return nil, err
+	}
+
+	return c.VolumeByName(ctx, secondaryName)
 }
 
 func (c Client) VolumeByName(ctx context.Context, name string) (*Volume, error) {
@@ -734,8 +765,23 @@ func (c Client) VolumeExists(ctx context.Context, volConfig *storage.VolumeConfi
 	if volConfig.InternalID != "" {
 		return c.VolumeExistsByID(ctx, volConfig.InternalID)
 	}
-	// Fall back to the creation token
-	return c.VolumeExistsByName(ctx, volConfig.InternalName)
+	primaryName, secondaryName := gcnvVolumeFallbackNames(volConfig)
+
+	// Fall back to the creation token.
+	exists, volume, err := c.VolumeExistsByName(ctx, primaryName)
+	if err != nil {
+		return false, nil, err
+	}
+	if exists {
+		return true, volume, nil
+	}
+
+	// For UNIFIED naming, also try underscore-normalized display name.
+	if secondaryName == "" {
+		return false, nil, nil
+	}
+
+	return c.VolumeExistsByName(ctx, secondaryName)
 }
 
 // VolumeByID returns a volume based on its full GCNV-style resource ID.

storage_drivers/gcp/api/gcnv_discovery.go

@@ -298,6 +298,7 @@ func (c Client) discoverCapacityPools(ctx context.Context) (*[]*CapacityPool, er
 				NetworkFullName: pool.Network,
 				Zone:            pool.Zone,
 				AutoTiering:     pool.AllowAutoTiering,
+				PoolType:        pool.GetType().String(),
 			})
 		}
 	}
@@ -327,13 +328,18 @@ func (c Client) CapacityPools() *[]*CapacityPool {
 // capacityPool returns a single discovered capacity pool by name.
 // Accepts either the short name (e.g., "pool-1") or the full name
 // (e.g., "projects/<project>/locations/<loc>/storagePools/<pool>").
+// Full-name lookups are O(1) via map; short names fall back to a linear scan.
 func (c Client) capacityPool(cPoolName string) *CapacityPool {
 	if cPoolName == "" {
 		return nil
 	}
+	cPools := c.sdkClient.resources.GetCapacityPools()
+	if cPool, ok := cPools.GetOk(cPoolName); ok && cPool != nil {
+		return cPool
+	}
 	var matchingCPool *CapacityPool
-	c.sdkClient.resources.GetCapacityPools().Range(func(_ string, cPool *CapacityPool) bool {
-		if cPool.Name == cPoolName || cPool.FullName == cPoolName {
+	cPools.Range(func(_ string, cPool *CapacityPool) bool {
+		if cPool.Name == cPoolName {
 			matchingCPool = cPool
 			return false
 		}

storage_drivers/gcp/api/gcnv_discovery_test.go

@@ -204,14 +204,35 @@ func TestCapacityPools(t *testing.T) {
 func TestCapacityPool_FullNameAndEmpty(t *testing.T) {
 	sdk := getFakeSDK(true)
 
-	// Full name should resolve
+	// Full name should resolve (O(1) map lookup) to same pool as short name (fallback scan)
 	fullName := "projects/" + ProjectNumber + "/locations/" + Location + "/storagePools/CP1"
 	assert.Equal(t, sdk.capacityPool("CP1"), sdk.capacityPool(fullName))
 
 	// Empty input should return nil
 	assert.Nil(t, sdk.capacityPool(""))
 }
 
+// TestCapacityPool_UnknownFullNameReturnsNil ensures that a full resource name not present
+// in the cache returns nil (map GetOk path), and does not match by short name.
+func TestCapacityPool_UnknownFullNameReturnsNil(t *testing.T) {
+	sdk := getFakeSDK(true)
+
+	unknownFullName := "projects/" + ProjectNumber + "/locations/" + Location + "/storagePools/nonexistent"
+	assert.Nil(t, sdk.capacityPool(unknownFullName))
+}
+
+// TestCapacityPool_ShortNameFallback ensures that when the cache is keyed by full name,
+// lookup by short name (Name) still returns the pool via fallback scan.
+func TestCapacityPool_ShortNameFallback(t *testing.T) {
+	sdk := getFakeSDK(true)
+
+	byShort := sdk.capacityPool("CP2")
+	byFull := sdk.capacityPool("projects/" + ProjectNumber + "/locations/" + Location + "/storagePools/CP2")
+	assert.NotNil(t, byShort)
+	assert.Equal(t, byShort, byFull)
+	assert.Equal(t, "CP2", byShort.Name)
+}
+
 func TestCapacityPoolsForStoragePools(t *testing.T) {
 	sdk := getFakeSDK(true)
 	sdk.sdkClient.resources.SetStoragePools(make(map[string]storage.Pool))

storage_drivers/gcp/api/gcnv_sdk_test.go

@@ -52,6 +52,37 @@ func TestNewVolumeFromGCNVVolume_ISCSIBlockDevicesAndPortals(t *testing.T) {
 	assert.Equal(t, 0, vol.LunID)
 }
 
+// TestNewVolumeFromGCNVVolume_ServiceLevelAndStoragePoolTypeFromSingleLookup verifies that
+// ServiceLevel and StoragePoolType are derived from a single capacity pool lookup (no duplicate scan),
+// and that StoragePoolType is empty when the pool is not found.
+func TestNewVolumeFromGCNVVolume_ServiceLevelAndStoragePoolTypeFromSingleLookup(t *testing.T) {
+	sdk := getFakeSDK(true)
+	cp1FullName := "projects/123456789/locations/fake-location/storagePools/CP1"
+
+	// Volume with StoragePool that resolves to CP1 (Premium); PoolType not set in getFakeSDK
+	volPB := &netapppb.Volume{
+		Name:        "projects/123456789/locations/fake-location/volumes/vol1",
+		Network:     "projects/123456789/global/networks/default",
+		StoragePool: cp1FullName,
+		CapacityGib: 10,
+		ShareName:   "share1",
+		Protocols:   []netapppb.Protocols{netapppb.Protocols_NFSV3},
+	}
+	vol, err := sdk.newVolumeFromGCNVVolume(ctx, volPB)
+	assert.NoError(t, err)
+	assert.NotNil(t, vol)
+	assert.Equal(t, ServiceLevelPremium, vol.ServiceLevel)
+	assert.Equal(t, "", vol.StoragePoolType)
+
+	// When pool is not in cache, StoragePoolType must be empty (nil-safe)
+	sdkNoPools := getFakeSDK()
+	volPB.StoragePool = cp1FullName
+	vol2, err := sdkNoPools.newVolumeFromGCNVVolume(ctx, volPB)
+	assert.NoError(t, err)
+	assert.NotNil(t, vol2)
+	assert.Equal(t, "", vol2.StoragePoolType)
+}
+
 func TestNewHostGroupFromGCNVHostGroup_Basic(t *testing.T) {
 	sdk := getFakeSDK()
 	hgPB := &netapppb.HostGroup{

storage_drivers/gcp/api/gcnv_structs.go

@@ -52,6 +52,10 @@ const (
 	StoragePoolStateDisabled    = "Disabled"
 	StoragePoolStateError       = "Error"
 
+	StoragePoolTypeFile            = "FILE"
+	StoragePoolTypeUnified         = "UNIFIED"
+	StoragePoolTypeUnifiedLargeCap = "UNIFIED_LARGE_CAPACITY"
+
 	VolumeStateUnspecified = "Unspecified"
 	VolumeStateReady       = "Ready"
 	VolumeStateCreating    = "Creating"
@@ -134,6 +138,12 @@ type CapacityPool struct {
 	NetworkFullName string
 	Zone            string
 	AutoTiering     bool
+	PoolType        string
+}
+
+// IsUnified returns true if the pool type is UNIFIED or UNIFIED_LARGE_CAPACITY.
+func (c *CapacityPool) IsUnified() bool {
+	return c.PoolType == StoragePoolTypeUnified || c.PoolType == StoragePoolTypeUnifiedLargeCap
 }
 
 // Volume records details of a discovered GCNV volume.
@@ -147,6 +157,7 @@ type Volume struct {
 	NetworkName               string
 	NetworkFullName           string
 	ServiceLevel              string
+	StoragePoolType           string
 	SizeBytes                 int64
 	ExportPolicy              *ExportPolicy
 	ProtocolTypes             []string
@@ -166,6 +177,11 @@ type Volume struct {
 	TieringMinimumCoolingDays *int32
 }
 
+// IsUnified returns true if the volume resides on a UNIFIED or UNIFIED_LARGE_CAPACITY pool.
+func (v *Volume) IsUnified() bool {
+	return v.StoragePoolType == StoragePoolTypeUnified || v.StoragePoolType == StoragePoolTypeUnifiedLargeCap
+}
+
 // VolumeCreateRequest embodies all the details of a volume to be created.
 // This unified request supports both NAS (NFS/SMB) and block (iSCSI) volumes.
 type VolumeCreateRequest struct {

storage_drivers/gcp/api/gcnv_test.go

@@ -1981,3 +1981,94 @@ func TestGetRegionalLocation(t *testing.T) {
 		assert.Contains(t, err.Error(), "backend config location is not set")
 	})
 }
+
+func TestCapacityPool_IsUnified(t *testing.T) {
+	tests := []struct {
+		PoolType string
+		Expected bool
+	}{
+		{StoragePoolTypeFile, false},
+		{StoragePoolTypeUnified, true},
+		{StoragePoolTypeUnifiedLargeCap, true},
+	}
+	for _, test := range tests {
+		t.Run(test.PoolType, func(t *testing.T) {
+			pool := &CapacityPool{
+				Name:     "test-pool",
+				PoolType: test.PoolType,
+			}
+
+			result := pool.IsUnified()
+
+			assert.Equal(t, test.Expected, result, "IsUnified() mismatch for pool type %s", test.PoolType)
+		})
+	}
+}
+
+func TestVolume_IsUnified(t *testing.T) {
+	tests := []struct {
+		StoragePoolType string
+		Expected        bool
+	}{
+		{StoragePoolTypeFile, false},
+		{StoragePoolTypeUnified, true},
+		{StoragePoolTypeUnifiedLargeCap, true},
+	}
+	for _, test := range tests {
+		t.Run(test.StoragePoolType, func(t *testing.T) {
+			vol := &Volume{
+				Name:            "test-volume",
+				StoragePoolType: test.StoragePoolType,
+			}
+
+			result := vol.IsUnified()
+
+			assert.Equal(t, test.Expected, result, "IsUnified() mismatch for pool type %s", test.StoragePoolType)
+		})
+	}
+}
+
+func TestGCNVVolumeFallbackNames(t *testing.T) {
+	tests := []struct {
+		name          string
+		volConfig     *storage.VolumeConfig
+		expectedFirst string
+		expectedNext  string
+	}{
+		{
+			name: "uses internal then underscore display variant",
+			volConfig: &storage.VolumeConfig{
+				InternalName: "my-vol",
+				Name:         "my-vol",
+			},
+			expectedFirst: "my-vol",
+			expectedNext:  "my_vol",
+		},
+		{
+			name: "no second lookup when same as internal",
+			volConfig: &storage.VolumeConfig{
+				InternalName: "my_vol",
+				Name:         "my_vol",
+			},
+			expectedFirst: "my_vol",
+			expectedNext:  "",
+		},
+		{
+			name: "no second lookup when display name empty",
+			volConfig: &storage.VolumeConfig{
+				InternalName: "token-1",
+				Name:         "",
+			},
+			expectedFirst: "token-1",
+			expectedNext:  "",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			first, next := gcnvVolumeFallbackNames(test.volConfig)
+			assert.Equal(t, test.expectedFirst, first)
+			assert.Equal(t, test.expectedNext, next)
+		})
+	}
+}