Skip to content

Commit d68e297

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#1563 from amacaskill/add-cmd-flag
Add command line flag for enabling Storage Pools feature
2 parents f78e4ff + af8b2c8 commit d68e297

File tree

9 files changed

+103
-48
lines changed

9 files changed

+103
-48
lines changed

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

+3-1
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ var (
7070

7171
fallbackRequisiteZonesFlag = flag.String("fallback-requisite-zones", "", "Comma separated list of requisite zones that will be used if there are not sufficient zones present in requisite topologies when provisioning a disk")
7272

73+
enableStoragePoolsFlag = flag.Bool("enable-storage-pools", false, "If set to true, the CSI Driver will allow volumes to be provisioned in Storage Pools")
74+
7375
version string
7476
)
7577

@@ -160,7 +162,7 @@ func handle() {
160162
}
161163
initialBackoffDuration := time.Duration(*errorBackoffInitialDurationMs) * time.Millisecond
162164
maxBackoffDuration := time.Duration(*errorBackoffMaxDurationMs) * time.Millisecond
163-
controllerServer = driver.NewControllerServer(gceDriver, cloudProvider, initialBackoffDuration, maxBackoffDuration, fallbackRequisiteZones)
165+
controllerServer = driver.NewControllerServer(gceDriver, cloudProvider, initialBackoffDuration, maxBackoffDuration, fallbackRequisiteZones, *enableStoragePoolsFlag)
164166
} else if *cloudConfigFilePath != "" {
165167
klog.Warningf("controller service is disabled but cloud config given - it has no effect")
166168
}

‎pkg/common/parameters.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ type StoragePool struct {
120120
// put them into a well defined struct making sure to default unspecified fields.
121121
// extraVolumeLabels are added as labels; if there are also labels specified in
122122
// parameters, any matching extraVolumeLabels will be overridden.
123-
func ExtractAndDefaultParameters(parameters map[string]string, driverName string, extraVolumeLabels map[string]string) (DiskParameters, error) {
123+
func ExtractAndDefaultParameters(parameters map[string]string, driverName string, extraVolumeLabels map[string]string, enableStoragePools bool) (DiskParameters, error) {
124124
p := DiskParameters{
125125
DiskType: "pd-standard", // Default
126126
ReplicationType: replicationTypeNone, // Default
@@ -200,6 +200,9 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
200200

201201
p.EnableConfidentialCompute = paramEnableConfidentialCompute
202202
case ParameterKeyStoragePools:
203+
if !enableStoragePools {
204+
return p, fmt.Errorf("parameters contains invalid option %q", ParameterKeyStoragePools)
205+
}
203206
storagePools, err := ParseStoragePools(v)
204207
if err != nil {
205208
return p, fmt.Errorf("parameters contain invalid value for %s parameter: %w", ParameterKeyStoragePools, err)

‎pkg/common/parameters_test.go

+48-33
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,12 @@ import (
2525

2626
func TestExtractAndDefaultParameters(t *testing.T) {
2727
tests := []struct {
28-
name string
29-
parameters map[string]string
30-
labels map[string]string
31-
expectParams DiskParameters
32-
expectErr bool
28+
name string
29+
parameters map[string]string
30+
labels map[string]string
31+
enableStoragePools bool
32+
expectParams DiskParameters
33+
expectErr bool
3334
}{
3435
{
3536
name: "defaults",
@@ -202,9 +203,10 @@ func TestExtractAndDefaultParameters(t *testing.T) {
202203
},
203204
},
204205
{
205-
name: "storage pool parameters",
206-
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1,projects/my-project/zones/us-central1-b/storagePools/storagePool-2"},
207-
labels: map[string]string{},
206+
name: "storage pool parameters",
207+
enableStoragePools: true,
208+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1,projects/my-project/zones/us-central1-b/storagePools/storagePool-2"},
209+
labels: map[string]string{},
208210
expectParams: DiskParameters{
209211
DiskType: "hyperdisk-balanced",
210212
ReplicationType: "none",
@@ -227,46 +229,59 @@ func TestExtractAndDefaultParameters(t *testing.T) {
227229
},
228230
},
229231
{
230-
name: "invalid storage pool parameters, starts with /projects instead of projects",
231-
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "/projects/my-project/zones/us-central1-a/storagePools/storagePool-1"},
232-
labels: map[string]string{},
233-
expectErr: true,
232+
name: "invalid storage pool parameters, starts with /projects instead of projects",
233+
enableStoragePools: true,
234+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "/projects/my-project/zones/us-central1-a/storagePools/storagePool-1"},
235+
labels: map[string]string{},
236+
expectErr: true,
234237
},
235238
{
236-
name: "invalid storage pool parameters, missing projects",
237-
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/storagePools/storagePool-1"},
238-
labels: map[string]string{},
239-
expectErr: true,
239+
name: "invalid storage pool parameters, missing projects",
240+
enableStoragePools: true,
241+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/storagePools/storagePool-1"},
242+
labels: map[string]string{},
243+
expectErr: true,
240244
},
241245
{
242-
name: "invalid storage pool parameters, missing zones",
243-
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/storagePool-1"},
244-
labels: map[string]string{},
245-
expectErr: true,
246+
name: "invalid storage pool parameters, missing zones",
247+
enableStoragePools: true,
248+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/storagePool-1"},
249+
labels: map[string]string{},
250+
expectErr: true,
246251
},
247252
{
248-
name: "invalid storage pool parameters, duplicate projects",
249-
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/projects/my-project/storagePools/storagePool-1"},
250-
labels: map[string]string{},
251-
expectErr: true,
253+
name: "invalid storage pool parameters, duplicate projects",
254+
enableStoragePools: true,
255+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/projects/my-project/storagePools/storagePool-1"},
256+
labels: map[string]string{},
257+
expectErr: true,
252258
},
253259
{
254-
name: "invalid storage pool parameters, duplicate zones",
255-
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/zones/us-central1-a/storagePools/storagePool-1"},
256-
labels: map[string]string{},
257-
expectErr: true,
260+
name: "invalid storage pool parameters, duplicate zones",
261+
enableStoragePools: true,
262+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/zones/us-central1-a/storagePools/storagePool-1"},
263+
labels: map[string]string{},
264+
expectErr: true,
258265
},
259266
{
260-
name: "invalid storage pool parameters, duplicate storagePools",
261-
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/us-central1-a/storagePools/storagePool-1"},
262-
labels: map[string]string{},
263-
expectErr: true,
267+
name: "invalid storage pool parameters, duplicate storagePools",
268+
enableStoragePools: true,
269+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/us-central1-a/storagePools/storagePool-1"},
270+
labels: map[string]string{},
271+
expectErr: true,
272+
},
273+
{
274+
name: "storage pool parameters, enableStoragePools is false",
275+
enableStoragePools: false,
276+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1,projects/my-project/zones/us-central1-b/storagePools/storagePool-2"},
277+
labels: map[string]string{},
278+
expectErr: true,
264279
},
265280
}
266281

267282
for _, tc := range tests {
268283
t.Run(tc.name, func(t *testing.T) {
269-
p, err := ExtractAndDefaultParameters(tc.parameters, "testDriver", tc.labels)
284+
p, err := ExtractAndDefaultParameters(tc.parameters, "testDriver", tc.labels, tc.enableStoragePools)
270285
if gotErr := err != nil; gotErr != tc.expectErr {
271286
t.Fatalf("ExtractAndDefaultParameters(%+v) = %v; expectedErr: %v", tc.parameters, err, tc.expectErr)
272287
}

‎pkg/gce-cloud-provider/compute/gce-compute.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -676,9 +676,9 @@ func (cloud *CloudProvider) insertZonalDisk(
676676
var insertOp *computealpha.Operation
677677
var storagePool *common.StoragePool
678678
if storagePoolsEnabled {
679-
storagePool = common.StoragePoolInZone(params.StoragePools, diskToCreate.Zone)
679+
storagePool = common.StoragePoolInZone(params.StoragePools, volKey.Zone)
680680
if storagePool == nil {
681-
return status.Errorf(codes.InvalidArgument, "cannot create disk in zone %q: no Storage Pools exist in zone", diskToCreate.Zone)
681+
return status.Errorf(codes.InvalidArgument, "cannot create disk in zone %q: no Storage Pools exist in zone", volKey.Zone)
682682
}
683683
}
684684
alphaDiskToCreate := convertV1DiskToAlphaDisk(diskToCreate, params.ProvisionedThroughputOnCreate, storagePool)

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

+5-2
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,9 @@ type GCEControllerServer struct {
100100
// provisioning in GKE Autopilot, where a GKE cluster to
101101
// be scaled down to 1 zone.
102102
fallbackRequisiteZones []string
103+
104+
// If set to true, the CSI Driver will allow volumes to be provisioned in Storage Pools.
105+
enableStoragePools bool
103106
}
104107

105108
type csiErrorBackoffId string
@@ -251,7 +254,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
251254

252255
// Apply Parameters (case-insensitive). We leave validation of
253256
// the values to the cloud provider.
254-
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels)
257+
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels, gceCS.enableStoragePools)
255258
diskTypeForMetric = params.DiskType
256259
enableConfidentialCompute = strconv.FormatBool(params.EnableConfidentialCompute)
257260
hasStoragePools := len(params.StoragePools) > 0
@@ -835,7 +838,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
835838
}
836839

837840
// Validate the disk parameters match the disk we GET
838-
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels)
841+
params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels, gceCS.enableStoragePools)
839842
if err != nil {
840843
return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err.Error())
841844
}

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

+35-6
Original file line numberDiff line numberDiff line change
@@ -444,10 +444,11 @@ func TestListSnapshotsArguments(t *testing.T) {
444444

445445
func TestCreateVolumeArguments(t *testing.T) {
446446
testCases := []struct {
447-
name string
448-
req *csi.CreateVolumeRequest
449-
expVol *csi.Volume
450-
expErrCode codes.Code
447+
name string
448+
req *csi.CreateVolumeRequest
449+
enableStoragePools bool
450+
expVol *csi.Volume
451+
expErrCode codes.Code
451452
}{
452453
{
453454
name: "success default",
@@ -898,6 +899,7 @@ func TestCreateVolumeArguments(t *testing.T) {
898899
},
899900
},
900901
},
902+
enableStoragePools: true,
901903
expVol: &csi.Volume{
902904
CapacityBytes: common.GbToBytes(20),
903905
VolumeId: "projects/test-project/zones/us-central1-a/disks/test-name",
@@ -909,6 +911,29 @@ func TestCreateVolumeArguments(t *testing.T) {
909911
},
910912
},
911913
},
914+
{
915+
name: "fail with storage pools parameter, enableStoragePools is false",
916+
req: &csi.CreateVolumeRequest{
917+
Name: name,
918+
CapacityRange: stdCapRange,
919+
VolumeCapabilities: stdVolCaps,
920+
Parameters: map[string]string{"storage-pools": "projects/test-project/zones/us-central1-a/storagePools/storagePool-1", "type": "hyperdisk-balanced"},
921+
AccessibilityRequirements: &csi.TopologyRequirement{
922+
Requisite: []*csi.Topology{
923+
{
924+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
925+
},
926+
},
927+
Preferred: []*csi.Topology{
928+
{
929+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
930+
},
931+
},
932+
},
933+
},
934+
enableStoragePools: false,
935+
expErrCode: codes.InvalidArgument,
936+
},
912937
{
913938
name: "fail with invalid storage pools parameter",
914939
req: &csi.CreateVolumeRequest{
@@ -929,7 +954,8 @@ func TestCreateVolumeArguments(t *testing.T) {
929954
},
930955
},
931956
},
932-
expErrCode: codes.InvalidArgument,
957+
enableStoragePools: true,
958+
expErrCode: codes.InvalidArgument,
933959
},
934960
}
935961

@@ -938,7 +964,7 @@ func TestCreateVolumeArguments(t *testing.T) {
938964
t.Logf("test case: %s", tc.name)
939965
// Setup new driver each time so no interference
940966
gceDriver := initGCEDriver(t, nil)
941-
967+
gceDriver.cs.enableStoragePools = tc.enableStoragePools
942968
// Start Test
943969
resp, err := gceDriver.cs.CreateVolume(context.Background(), tc.req)
944970
if err != nil {
@@ -1374,6 +1400,7 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
13741400
requestCapacityRange *csi.CapacityRange
13751401
sourceTopology *csi.TopologyRequirement
13761402
requestTopology *csi.TopologyRequirement
1403+
enableStoragePools bool
13771404
expCloneKey *meta.Key
13781405
// Accessible topologies validates that the replica zones are valid for regional disk clones.
13791406
expAccessibleTop []*csi.Topology
@@ -1461,6 +1488,7 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
14611488
sourceVolumeID: testZonalVolumeSourceID,
14621489
requestCapacityRange: stdCapRange,
14631490
sourceCapacityRange: stdCapRange,
1491+
enableStoragePools: true,
14641492
reqParameters: map[string]string{
14651493
common.ParameterKeyType: "test-type",
14661494
common.ParameterKeyReplicationType: replicationTypeNone,
@@ -1880,6 +1908,7 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
18801908
for _, tc := range testCases {
18811909
t.Logf("test case: %s", tc.name)
18821910
gceDriver := initGCEDriver(t, nil)
1911+
gceDriver.cs.enableStoragePools = tc.enableStoragePools
18831912

18841913
req := &csi.CreateVolumeRequest{
18851914
Name: testCloneVolumeName,

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -152,14 +152,15 @@ func NewNodeServer(gceDriver *GCEDriver, mounter *mount.SafeFormatAndMount, devi
152152
}
153153
}
154154

155-
func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, errorBackoffInitialDuration, errorBackoffMaxDuration time.Duration, fallbackRequisiteZones []string) *GCEControllerServer {
155+
func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, errorBackoffInitialDuration, errorBackoffMaxDuration time.Duration, fallbackRequisiteZones []string, enableStoragePools bool) *GCEControllerServer {
156156
return &GCEControllerServer{
157157
Driver: gceDriver,
158158
CloudProvider: cloudProvider,
159159
seen: map[string]int{},
160160
volumeLocks: common.NewVolumeLocks(),
161161
errorBackoff: newCsiErrorBackoff(errorBackoffInitialDuration, errorBackoffMaxDuration),
162162
fallbackRequisiteZones: fallbackRequisiteZones,
163+
enableStoragePools: enableStoragePools,
163164
}
164165
}
165166

‎pkg/gce-pd-csi-driver/gce-pd-driver_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,9 @@ func initGCEDriverWithCloudProvider(t *testing.T, cloudProvider gce.GCECompute)
4747
errorBackoffInitialDuration := 200 * time.Millisecond
4848
errorBackoffMaxDuration := 5 * time.Minute
4949
fallbackRequisiteZones := []string{}
50+
enableStoragePools := false
5051

51-
controllerServer := NewControllerServer(gceDriver, cloudProvider, errorBackoffInitialDuration, errorBackoffMaxDuration, fallbackRequisiteZones)
52+
controllerServer := NewControllerServer(gceDriver, cloudProvider, errorBackoffInitialDuration, errorBackoffMaxDuration, fallbackRequisiteZones, enableStoragePools)
5253
err := gceDriver.SetupGCEDriver(driver, vendorVersion, nil, nil, controllerServer, nil)
5354
if err != nil {
5455
t.Fatalf("Failed to setup GCE Driver: %v", err)

‎test/sanity/sanity_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -62,13 +62,14 @@ func TestSanity(t *testing.T) {
6262
}
6363

6464
fallbackRequisiteZones := []string{}
65+
enableStoragePools := false
6566

6667
mounter := mountmanager.NewFakeSafeMounter()
6768
deviceUtils := deviceutils.NewFakeDeviceUtils(true)
6869

6970
//Initialize GCE Driver
7071
identityServer := driver.NewIdentityServer(gceDriver)
71-
controllerServer := driver.NewControllerServer(gceDriver, cloudProvider, 0, 5*time.Minute, fallbackRequisiteZones)
72+
controllerServer := driver.NewControllerServer(gceDriver, cloudProvider, 0, 5*time.Minute, fallbackRequisiteZones, enableStoragePools)
7273
nodeServer := driver.NewNodeServer(gceDriver, mounter, deviceUtils, metadataservice.NewFakeService(), mountmanager.NewFakeStatter(mounter))
7374
err = gceDriver.SetupGCEDriver(driverName, vendorVersion, extraLabels, identityServer, controllerServer, nodeServer)
7475
if err != nil {

0 commit comments

Comments
 (0)