diff --git a/README.md b/README.md index 344573401..a87be0f7f 100644 --- a/README.md +++ b/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 | `//,//` | | 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 diff --git a/cmd/gce-pd-csi-driver/main.go b/cmd/gce-pd-csi-driver/main.go index cf4877633..099532de3 100644 --- a/cmd/gce-pd-csi-driver/main.go +++ b/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_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 ) diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go index cd973b319..5ea701f26 100644 --- a/pkg/common/parameters.go +++ b/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) } diff --git a/pkg/common/parameters_test.go b/pkg/common/parameters_test.go index 33434ec46..1ff498a45 100644 --- a/pkg/common/parameters_test.go +++ b/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 { diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index ee936955e..9fa8ed406 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/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" } diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index a70babebc..38d30e224 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/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 +} diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index 8fa882c23..4366cd832 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/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. diff --git a/test/e2e/utils/utils.go b/test/e2e/utils/utils.go index 0f9f7dd86..01bad4513 100644 --- a/test/e2e/utils/utils.go +++ b/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))