Merge pull request #2089 from julianKatz/enable-disk-topology-with-StorageClass-parameter

Only add disk support topologies if StorageClass parameter is true

Author: k8s-ci-robot

README.md

@@ -61,6 +61,7 @@ See Github [Issues](https://github.com/kubernetes-sigs/gcp-compute-persistent-di
 | provisioned-iops-on-create  | string (int64 format). Values typically between 10,000 and 120,000 |               | Indicates how many IOPS to provision for the disk. See the [Extreme persistent disk documentation](https://cloud.google.com/compute/docs/disks/extreme-persistent-disk) for details, including valid ranges for IOPS. |
 | provisioned-throughput-on-create  | string (int64 format). Values typically between 1 and 7,124 mb per second |               | Indicates how much throughput to provision for the disk. See the [hyperdisk documentation]([TBD](https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/hyperdisk#create)) for details, including valid ranges for throughput. |
 | resource-tags               | `<parent_id1>/<tag_key1>/<tag_value1>,<parent_id2>/<tag_key2>/<tag_value2>` |               | Resource tags allow you to attach user-defined tags to each Compute Disk, Image and Snapshot. See [Tags overview](https://cloud.google.com/resource-manager/docs/tags/tags-overview), [Creating and managing tags](https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing). |
+| use-allowed-disk-topologies | `true` or `false`         | `false`       | Allows the use of specific disk topologies for provisioning. Must be used in combination with the `--disk-topology=true` flag on PDCSI binary to yield disk support labels in PV NodeAffinity blocks. |
 
 ### Topology
 

cmd/gce-pd-csi-driver/main.go

@@ -94,7 +94,7 @@ var (
 
 	extraTagsStr = flag.String("extra-tags", "", "Extra tags to attach to each Compute Disk, Image, Snapshot created. It is a comma separated list of parent id, key and value like '<parent_id1>/<tag_key1>/<tag_value1>,...,<parent_idN>/<tag_keyN>/<tag_valueN>'. parent_id is the Organization or the Project ID or Project name where the tag key and the tag value resources exist. A maximum of 50 tags bindings is allowed for a resource. See https://cloud.google.com/resource-manager/docs/tags/tags-overview, https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing for details")
 
-	diskTopology = flag.Bool("disk-topology", false, "If set to true, the driver will add a disk-type.gke.io/[some-disk-type] topology label to the Topologies returned in CreateVolumeResponse.")
+	diskTopology = flag.Bool("disk-topology", false, "If set to true, the driver will add a disk-type.gke.io/[disk-type] topology label when the StorageClass has the use-allowed-disk-topology parameter set to true. That topology label is included in the Topologies returned in CreateVolumeResponse. This flag is disabled by default.")
 
 	version string
 )

pkg/common/parameters.go

@@ -20,6 +20,8 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+
+	"k8s.io/klog/v2"
 )
 
 const (
@@ -36,6 +38,7 @@ const (
 	ParameterAvailabilityClass                = "availability-class"
 	ParameterKeyEnableConfidentialCompute     = "enable-confidential-storage"
 	ParameterKeyStoragePools                  = "storage-pools"
+	ParameterKeyUseAllowedDiskTopology        = "use-allowed-disk-topology"
 
 	// Parameters for Data Cache
 	ParameterKeyDataCacheSize               = "data-cache-size"
@@ -132,6 +135,9 @@ type DiskParameters struct {
 	// Values: READ_WRITE_SINGLE, READ_ONLY_MANY, READ_WRITE_MANY
 	// Default: READ_WRITE_SINGLE
 	AccessMode string
+	// Values {}
+	// Default: false
+	UseAllowedDiskTopology bool
 }
 
 func (dp *DiskParameters) IsRegional() bool {
@@ -160,6 +166,7 @@ type ParameterProcessor struct {
 	EnableStoragePools bool
 	EnableMultiZone    bool
 	EnableHdHA         bool
+	EnableDiskTopology bool
 }
 
 type ModifyVolumeParameters struct {
@@ -319,6 +326,19 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
 			if v != "" {
 				p.AccessMode = v
 			}
+		case ParameterKeyUseAllowedDiskTopology:
+			if !pp.EnableDiskTopology {
+				klog.Warningf("parameters contains invalid option %q when disk topology is not enabled", ParameterKeyUseAllowedDiskTopology)
+				continue
+			}
+
+			paramUseAllowedDiskTopology, err := ConvertStringToBool(v)
+			if err != nil {
+				klog.Warningf("failed to convert %s parameter with value %q to bool: %v", ParameterKeyUseAllowedDiskTopology, v, err)
+				continue
+			}
+
+			p.UseAllowedDiskTopology = paramUseAllowedDiskTopology
 		default:
 			return p, d, fmt.Errorf("parameters contains invalid option %q", k)
 		}

pkg/common/parameters_test.go

@@ -32,6 +32,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 		enableDataCache       bool
 		enableMultiZone       bool
 		enableHdHA            bool
+		enableDiskTopology    bool
 		extraTags             map[string]string
 		expectParams          DiskParameters
 		expectDataCacheParams DataCacheParameters
@@ -42,12 +43,13 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 			parameters: map[string]string{},
 			labels:     map[string]string{},
 			expectParams: DiskParameters{
-				DiskType:             "pd-standard",
-				ReplicationType:      "none",
-				DiskEncryptionKMSKey: "",
-				Tags:                 map[string]string{},
-				Labels:               map[string]string{},
-				ResourceTags:         map[string]string{},
+				DiskType:               "pd-standard",
+				ReplicationType:        "none",
+				DiskEncryptionKMSKey:   "",
+				Tags:                   map[string]string{},
+				Labels:                 map[string]string{},
+				ResourceTags:           map[string]string{},
+				UseAllowedDiskTopology: false,
 			},
 		},
 		{
@@ -464,6 +466,53 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				Labels:          map[string]string{},
 			},
 		},
+		{
+			// Disk topology feature shouldn't cause parameter parsing to fail, even when misconfigured.
+			name: "useAllowedDiskTopology specified, disk topology feature disabled",
+			parameters: map[string]string{
+				ParameterKeyUseAllowedDiskTopology: "true", // Correct type: boolean string.
+			},
+			labels: map[string]string{},
+			expectParams: DiskParameters{
+				DiskType:             "pd-standard",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "",
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{},
+				ResourceTags:         map[string]string{},
+			},
+		},
+		{
+			// Disk topology feature shouldn't cause parameter parsing to fail, even when misconfigured.
+			name:               "useAllowedDiskTopology specified, wrong type",
+			enableDiskTopology: true,
+			parameters: map[string]string{
+				ParameterKeyUseAllowedDiskTopology: "123", // Incorrect type: number.
+			},
+			labels: map[string]string{},
+			expectParams: DiskParameters{
+				DiskType:             "pd-standard",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "",
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{},
+				ResourceTags:         map[string]string{},
+			},
+		},
+		{
+			name:               "useAllowedDiskTopology specified as valid true boolean",
+			parameters:         map[string]string{ParameterKeyUseAllowedDiskTopology: "true"},
+			enableDiskTopology: true,
+			expectParams: DiskParameters{
+				DiskType:               "pd-standard",
+				ReplicationType:        "none",
+				DiskEncryptionKMSKey:   "",
+				Tags:                   map[string]string{},
+				Labels:                 map[string]string{},
+				ResourceTags:           map[string]string{},
+				UseAllowedDiskTopology: true,
+			},
+		},
 	}
 
 	for _, tc := range tests {
@@ -473,6 +522,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				EnableStoragePools: tc.enableStoragePools,
 				EnableMultiZone:    tc.enableMultiZone,
 				EnableHdHA:         tc.enableHdHA,
+				EnableDiskTopology: tc.enableDiskTopology,
 			}
 			p, d, err := pp.ExtractAndDefaultParameters(tc.parameters, tc.labels, tc.enableDataCache, tc.extraTags)
 			if gotErr := err != nil; gotErr != tc.expectErr {

pkg/gce-pd-csi-driver/controller.go

@@ -1339,6 +1339,7 @@ func (gceCS *GCEControllerServer) parameterProcessor() *common.ParameterProcesso
 		EnableStoragePools: gceCS.enableStoragePools,
 		EnableMultiZone:    gceCS.multiZoneVolumeHandleConfig.Enable,
 		EnableHdHA:         gceCS.enableHdHA,
+		EnableDiskTopology: gceCS.EnableDiskTopology,
 	}
 }
 
@@ -2418,7 +2419,11 @@ func (gceCS *GCEControllerServer) generateCreateVolumeResponseWithVolumeId(disk
 			},
 		}
 
-		if gceCS.EnableDiskTopology {
+		// The addition of the disk type label requires both the feature to be
+		// enabled on the PDCSI binary via the `--disk-topology=true` flag AND
+		// the StorageClass to have the `use-allowed-disk-topology` parameter
+		// set to `true`.
+		if gceCS.EnableDiskTopology && params.UseAllowedDiskTopology {
 			top.Segments[common.DiskTypeLabelKey(params.DiskType)] = "true"
 		}
 

pkg/gce-pd-csi-driver/controller_test.go

@@ -1349,13 +1349,43 @@ func TestCreateVolumeArguments(t *testing.T) {
 		},
 		// Disk Topology Enabled tests
 		{
-			name: "success with disk topology enabled",
+			name: "success with disk topology enabled and StorageClass useAllowedDiskTopology false",
 			req: &csi.CreateVolumeRequest{
 				Name:               "test-name",
 				CapacityRange:      stdCapRange,
 				VolumeCapabilities: stdVolCaps,
-				Parameters:         stdParams,
+				Parameters: mergeParameters(
+					stdParams,
+					map[string]string{common.ParameterKeyUseAllowedDiskTopology: "false"},
+				),
 			},
+			enableDiskTopology: true,
+			expVol: &csi.Volume{
+				CapacityBytes: common.GbToBytes(20),
+				VolumeId:      testVolumeID,
+				VolumeContext: nil,
+				AccessibleTopology: []*csi.Topology{
+					{
+						Segments: map[string]string{
+							common.TopologyKeyZone: zone,
+							// Disk not type not included since useAllowedDiskTopology is false
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "success with disk topology enabled and StorageClass useAllowedDiskTopology true",
+			req: &csi.CreateVolumeRequest{
+				Name:               "test-name",
+				CapacityRange:      stdCapRange,
+				VolumeCapabilities: stdVolCaps,
+				Parameters: mergeParameters(
+					stdParams,
+					map[string]string{common.ParameterKeyUseAllowedDiskTopology: "true"},
+				),
+			},
+			enableDiskTopology: true,
 			expVol: &csi.Volume{
 				CapacityBytes: common.GbToBytes(20),
 				VolumeId:      testVolumeID,
@@ -1370,46 +1400,46 @@ func TestCreateVolumeArguments(t *testing.T) {
 					},
 				},
 			},
-			enableDiskTopology: true,
 		},
 	}
 
 	// Run test cases
 	for _, tc := range testCases {
-		t.Logf("test case: %s", tc.name)
-
-		// Setup new driver each time so no interference
-		args := &GCEControllerServerArgs{EnableDiskTopology: tc.enableDiskTopology}
-		gceDriver := initGCEDriver(t, nil, args)
-		gceDriver.cs.enableStoragePools = tc.enableStoragePools
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup new driver each time so no interference
+			args := &GCEControllerServerArgs{EnableDiskTopology: tc.enableDiskTopology}
+			gceDriver := initGCEDriver(t, nil, args)
+			gceDriver.cs.enableStoragePools = tc.enableStoragePools
 
-		// Start Test
-		resp, err := gceDriver.cs.CreateVolume(context.Background(), tc.req)
-		if err != nil {
-			serverError, ok := status.FromError(err)
-			if !ok {
-				t.Fatalf("Could not get error status code from err: %v", serverError)
+			// Start Test
+			resp, err := gceDriver.cs.CreateVolume(context.Background(), tc.req)
+			if err != nil {
+				serverError, ok := status.FromError(err)
+				if !ok {
+					t.Fatalf("Could not get error status code from err: %v", serverError)
+				}
+				if serverError.Code() != tc.expErrCode {
+					t.Fatalf("Expected error code: %v, got: %v. err : %v", tc.expErrCode, serverError.Code(), err)
+				}
+				return
 			}
-			if serverError.Code() != tc.expErrCode {
-				t.Fatalf("Expected error code: %v, got: %v. err : %v", tc.expErrCode, serverError.Code(), err)
+
+			t.Logf("ErrorCode: %v", err)
+			if tc.expErrCode != codes.OK {
+				t.Fatalf("Expected error: %v, got no error", tc.expErrCode)
 			}
-			continue
-		}
-		t.Logf("ErroCode: %v", err)
-		if tc.expErrCode != codes.OK {
-			t.Fatalf("Expected error: %v, got no error", tc.expErrCode)
-		}
 
-		// Make sure responses match
-		vol := resp.GetVolume()
-		if vol == nil {
-			// If one is nil but not both
-			t.Fatalf("Expected volume %v, got nil volume", tc.expVol)
-		}
+			// Make sure responses match
+			vol := resp.GetVolume()
+			if vol == nil {
+				// If one is nil but not both
+				t.Fatalf("Expected volume %v, got nil volume", tc.expVol)
+			}
 
-		if diff := cmp.Diff(vol, tc.expVol, protocmp.Transform()); diff != "" {
-			t.Errorf("unexpected diff (-vol, +expVol): \n%s", diff)
-		}
+			if diff := cmp.Diff(vol, tc.expVol, protocmp.Transform()); diff != "" {
+				t.Errorf("unexpected diff (-vol, +expVol): \n%s", diff)
+			}
+		})
 	}
 }
 
@@ -5818,3 +5848,14 @@ func googleapiErrContainsReason(err *googleapi.Error, reason string) bool {
 	}
 	return false
 }
+
+func mergeParameters(a map[string]string, b map[string]string) map[string]string {
+	merged := make(map[string]string)
+	for k, v := range a {
+		merged[k] = v
+	}
+	for k, v := range b {
+		merged[k] = v
+	}
+	return merged
+}

test/e2e/tests/single_zone_e2e_test.go

@@ -283,6 +283,41 @@ var _ = Describe("GCE PD CSI Driver", func() {
 			Expect(err).To(BeNil(), "Could not find disk in correct zone")
 		}
 	})
+
+	It("Should create a volume with allowed disk topology and confirm disk support label", func() {
+		Expect(testContexts).ToNot(BeEmpty())
+		testContext := getRandomTestContext()
+
+		volName := testNamePrefix + string(uuid.NewUUID())
+		params := map[string]string{
+			"type": hdbDiskType,
+			// Required to enable the disk topology feature.
+			"use-allowed-disk-topology": "true",
+		}
+
+		topReq := &csi.TopologyRequirement{
+			Requisite: []*csi.Topology{
+				{
+					Segments: map[string]string{common.TopologyKeyZone: "us-central1-c"},
+				},
+			},
+		}
+
+		volume, err := testContext.Client.CreateVolume(volName, params, defaultSizeGb, topReq, nil)
+		Expect(err).To(BeNil(), "Failed to create volume")
+		defer func() {
+			err = testContext.Client.DeleteVolume(volume.VolumeId)
+			Expect(err).To(BeNil(), "Failed to delete volume")
+		}()
+
+		// Confirm that the topologies include a disk support label
+		Expect(volume.AccessibleTopology).ToNot(BeEmpty(), "Volume should have accessible topologies")
+		Expect(volume.AccessibleTopology).To(HaveLen(1), "Expected exactly one accessible topology") // Zonal clusters have a single Topology.
+		segments := volume.AccessibleTopology[0].Segments
+		Expect(segments).To(HaveKeyWithValue(common.TopologyKeyZone, "us-central1-c"), "Topology should include zone segment with value 'us-central1-c'")
+		Expect(segments).To(HaveKeyWithValue(common.DiskTypeLabelKey(hdbDiskType), "true"), "Topology should include disk type label with value 'true'")
+	})
+
 	// TODO(hime): Enable this test once all release branches contain the fix from PR#1708.
 	// It("Should return InvalidArgument when disk size exceeds limit", func() {
 	// 	// If this returns a different error code (like Unknown), the error wrapping logic in #1708 has regressed.

test/e2e/utils/utils.go

@@ -71,6 +71,7 @@ func GCEClientAndDriverSetup(instance *remote.InstanceInfo, driverConfig DriverC
 		"--allow-hdha-provisioning",
 		"--device-in-use-timeout=10s", // Set lower than the usual value to expedite tests
 		fmt.Sprintf("--fallback-requisite-zones=%s", strings.Join(driverConfig.Zones, ",")),
+		"--disk-topology=true",
 	}
 
 	extra_flags = append(extra_flags, fmt.Sprintf("--node-name=%s", utilcommon.TestNode))