Skip to content

Commit efae0fc

Browse files
TeweiLuoTeweiLuo
committed
Allow users to specify "storage-locations" for snapshots.
Use a comma separated string, for example "us-central1,us-west1" as the "snapshot-locations" field of snapshot class parameters. See the PR for detailed examples.
1 parent 1067990 commit efae0fc

File tree

8 files changed

+181
-13
lines changed

8 files changed

+181
-13
lines changed

‎pkg/common/parameters.go

+28
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,15 @@ import (
2222
)
2323

2424
const (
25+
// Parameters for StorageClass
2526
ParameterKeyType = "type"
2627
ParameterKeyReplicationType = "replication-type"
2728
ParameterKeyDiskEncryptionKmsKey = "disk-encryption-kms-key"
2829
ParameterKeyLabels = "labels"
2930

31+
// Parameters for VolumeSnapshotClass
32+
ParameterKeyStorageLocations = "storage-locations"
33+
3034
replicationTypeNone = "none"
3135

3236
// Keys for PV and PVC parameters as reported by external-provisioner
@@ -60,6 +64,11 @@ type DiskParameters struct {
6064
Labels map[string]string
6165
}
6266

67+
// SnapshotParameters contains normalized and defaulted parameters for snapshots
68+
type SnapshotParameters struct {
69+
StorageLocations []string
70+
}
71+
6372
// ExtractAndDefaultParameters will take the relevant parameters from a map and
6473
// put them into a well defined struct making sure to default unspecified fields.
6574
// extraVolumeLabels are added as labels; if there are also labels specified in
@@ -118,3 +127,22 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
118127
}
119128
return p, nil
120129
}
130+
131+
func ExtractAndDefaultSnapshotParameters(parameters map[string]string) (SnapshotParameters, error) {
132+
p := SnapshotParameters{
133+
StorageLocations: []string{},
134+
}
135+
for k, v := range parameters {
136+
switch strings.ToLower(k) {
137+
case ParameterKeyStorageLocations:
138+
normalizedStorageLocations, err := ProcessStorageLocations(v)
139+
if err != nil {
140+
return p, err
141+
}
142+
p.StorageLocations = normalizedStorageLocations
143+
default:
144+
return p, fmt.Errorf("parameters contains invalid option %q", k)
145+
}
146+
}
147+
return p, nil
148+
}

‎pkg/common/parameters_test.go

+37
Original file line numberDiff line numberDiff line change
@@ -164,3 +164,40 @@ func TestExtractAndDefaultParameters(t *testing.T) {
164164
})
165165
}
166166
}
167+
168+
// Currently the only parameter is storage-locations, which is already tested
169+
// in utils_test/TestSnapshotStorageLocations. Here we just test the case where
170+
// no parameter is set in the snapshot class.
171+
func TestSnapshotParameters(t *testing.T) {
172+
tests := []struct {
173+
desc string
174+
parameters map[string]string
175+
expectedSnapshotParames SnapshotParameters
176+
}{
177+
{
178+
desc: "valid parameter",
179+
parameters: map[string]string{ParameterKeyStorageLocations: " us-East1, ASIA "},
180+
expectedSnapshotParames: SnapshotParameters{
181+
StorageLocations: []string{"us-east1", "asia"},
182+
},
183+
},
184+
{
185+
desc: "nil parameter",
186+
parameters: nil,
187+
expectedSnapshotParames: SnapshotParameters{
188+
StorageLocations: []string{},
189+
},
190+
},
191+
}
192+
for _, tc := range tests {
193+
t.Run(tc.desc, func(t *testing.T) {
194+
p, err := ExtractAndDefaultSnapshotParameters(tc.parameters)
195+
if err != nil {
196+
t.Errorf("Got error processing snapshot parameters: %v; expect no error", err)
197+
}
198+
if !reflect.DeepEqual(p, tc.expectedSnapshotParames) {
199+
t.Errorf("Got ExtractAndDefaultSnapshotParameters(%+v) = %+v; expect %+v", tc.parameters, p, tc.expectedSnapshotParames)
200+
}
201+
})
202+
}
203+
}

‎pkg/common/utils.go

+30
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ const (
5050
nodeIDTotalElements = 6
5151

5252
regionalDeviceNameSuffix = "_regional"
53+
54+
// Snapshot storage location format
55+
// Reference: https://cloud.google.com/storage/docs/locations
56+
// Example: us
57+
multiRegionalLocationFmt = "^[a-z]+$"
58+
// Example: us-east1
59+
regionalLocationFmt = "^[a-z]+-[a-z]+[0-9]$"
5360
)
5461

5562
func BytesToGbRoundDown(bytes int64) int64 {
@@ -217,3 +224,26 @@ func ConvertLabelsStringToMap(labels string) (map[string]string, error) {
217224

218225
return labelsMap, nil
219226
}
227+
228+
// Transform storage locations from a comma separated string to an array,
229+
// and validate the format of each location. Note that empty string does
230+
// not need specail treatment because Split() turns it into an empty
231+
// array, which is the desired value.
232+
func ProcessStorageLocations(storageLocations string) ([]string, error) {
233+
locations := strings.Split(storageLocations, ",")
234+
235+
var normalizedLocations []string
236+
237+
multiRegionalPattern, _ := regexp.Compile(multiRegionalLocationFmt)
238+
239+
regionalPattern, _ := regexp.Compile(regionalLocationFmt)
240+
241+
for _, loc := range locations {
242+
normalizedLoc := strings.ToLower(strings.TrimSpace(loc))
243+
if !multiRegionalPattern.MatchString(normalizedLoc) && !regionalPattern.MatchString(normalizedLoc) {
244+
return []string{}, fmt.Errorf("invalid location for snapshot: %q", loc)
245+
}
246+
normalizedLocations = append(normalizedLocations, normalizedLoc)
247+
}
248+
return normalizedLocations, nil
249+
}

‎pkg/common/utils_test.go

+37
Original file line numberDiff line numberDiff line change
@@ -534,3 +534,40 @@ func TestConvertLabelsStringToMap(t *testing.T) {
534534
})
535535

536536
}
537+
538+
func TestSnapshotStorageLocations(t *testing.T) {
539+
tests := []struct {
540+
desc string
541+
locationString string
542+
expectedNormalizedLocations []string
543+
expectError bool
544+
}{
545+
{
546+
"valid locations",
547+
"us, us-West1, asia , AUSTRALIA-SOUTHEAST1 ",
548+
[]string{"us", "us-west1", "asia", "australia-southeast1"},
549+
false,
550+
},
551+
{
552+
// Zones are not valid bucket/snapshot locations.
553+
"single zone",
554+
"us-east1a, asia-east",
555+
[]string{},
556+
true,
557+
},
558+
}
559+
for _, tc := range tests {
560+
t.Run(tc.desc, func(t *testing.T) {
561+
normalizedLocations, err := ProcessStorageLocations(tc.locationString)
562+
if err != nil && !tc.expectError {
563+
t.Errorf("Got error %v processing storage locations %q; expect no error", err, tc.locationString)
564+
}
565+
if err == nil && tc.expectError {
566+
t.Errorf("Got no error processing storage locations %q; expect an error", tc.locationString)
567+
}
568+
if err == nil && !reflect.DeepEqual(normalizedLocations, tc.expectedNormalizedLocations) {
569+
t.Errorf("Got %v for normalized storage locations; expect %v", normalizedLocations, tc.expectedNormalizedLocations)
570+
}
571+
})
572+
}
573+
}

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

+4-3
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ func (cloud *FakeCloudProvider) GetSnapshot(ctx context.Context, project, snapsh
387387
return snapshot, nil
388388
}
389389

390-
func (cloud *FakeCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string) (*computev1.Snapshot, error) {
390+
func (cloud *FakeCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) {
391391
if snapshot, ok := cloud.snapshots[snapshotName]; ok {
392392
return snapshot, nil
393393
}
@@ -398,6 +398,7 @@ func (cloud *FakeCloudProvider) CreateSnapshot(ctx context.Context, project stri
398398
CreationTimestamp: Timestamp,
399399
Status: "UPLOADING",
400400
SelfLink: cloud.getGlobalSnapshotURI(project, snapshotName),
401+
StorageLocations: snapshotParams.StorageLocations,
401402
}
402403
switch volKey.Type() {
403404
case meta.Zonal:
@@ -493,11 +494,11 @@ type FakeBlockingCloudProvider struct {
493494
// Upon starting a CreateSnapshot, it passes a chan 'executeCreateSnapshot' into readyToExecute, then blocks on executeCreateSnapshot.
494495
// The test calling this function can block on readyToExecute to ensure that the operation has started and
495496
// allowed the CreateSnapshot to continue by passing a struct into executeCreateSnapshot.
496-
func (cloud *FakeBlockingCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string) (*computev1.Snapshot, error) {
497+
func (cloud *FakeBlockingCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) {
497498
executeCreateSnapshot := make(chan struct{})
498499
cloud.ReadyToExecute <- executeCreateSnapshot
499500
<-executeCreateSnapshot
500-
return cloud.FakeCloudProvider.CreateSnapshot(ctx, project, volKey, snapshotName)
501+
return cloud.FakeCloudProvider.CreateSnapshot(ctx, project, volKey, snapshotName, snapshotParams)
501502
}
502503

503504
func notFoundError() *googleapi.Error {

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

+10-8
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ type GCECompute interface {
7373
ListZones(ctx context.Context, region string) ([]string, error)
7474
ListSnapshots(ctx context.Context, filter string, maxEntries int64, pageToken string) ([]*computev1.Snapshot, string, error)
7575
GetSnapshot(ctx context.Context, project, snapshotName string) (*computev1.Snapshot, error)
76-
CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string) (*computev1.Snapshot, error)
76+
CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error)
7777
DeleteSnapshot(ctx context.Context, project, snapshotName string) error
7878
}
7979

@@ -787,13 +787,13 @@ func (cloud *CloudProvider) DeleteSnapshot(ctx context.Context, project, snapsho
787787
return nil
788788
}
789789

790-
func (cloud *CloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string) (*computev1.Snapshot, error) {
790+
func (cloud *CloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) {
791791
klog.V(5).Infof("Creating snapshot %s for volume %v", snapshotName, volKey)
792792
switch volKey.Type() {
793793
case meta.Zonal:
794-
return cloud.createZonalDiskSnapshot(ctx, project, volKey, snapshotName)
794+
return cloud.createZonalDiskSnapshot(ctx, project, volKey, snapshotName, snapshotParams)
795795
case meta.Regional:
796-
return cloud.createRegionalDiskSnapshot(ctx, project, volKey, snapshotName)
796+
return cloud.createRegionalDiskSnapshot(ctx, project, volKey, snapshotName, snapshotParams)
797797
default:
798798
return nil, fmt.Errorf("could not create snapshot, key was neither zonal nor regional, instead got: %v", volKey.String())
799799
}
@@ -864,9 +864,10 @@ func (cloud *CloudProvider) resizeRegionalDisk(ctx context.Context, project stri
864864
return requestGb, nil
865865
}
866866

867-
func (cloud *CloudProvider) createZonalDiskSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string) (*computev1.Snapshot, error) {
867+
func (cloud *CloudProvider) createZonalDiskSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) {
868868
snapshotToCreate := &computev1.Snapshot{
869-
Name: snapshotName,
869+
Name: snapshotName,
870+
StorageLocations: snapshotParams.StorageLocations,
870871
}
871872

872873
_, err := cloud.service.Disks.CreateSnapshot(project, volKey.Zone, volKey.Name, snapshotToCreate).Context(ctx).Do()
@@ -878,9 +879,10 @@ func (cloud *CloudProvider) createZonalDiskSnapshot(ctx context.Context, project
878879
return cloud.waitForSnapshotCreation(ctx, project, snapshotName)
879880
}
880881

881-
func (cloud *CloudProvider) createRegionalDiskSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string) (*computev1.Snapshot, error) {
882+
func (cloud *CloudProvider) createRegionalDiskSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) {
882883
snapshotToCreate := &computev1.Snapshot{
883-
Name: snapshotName,
884+
Name: snapshotName,
885+
StorageLocations: snapshotParams.StorageLocations,
884886
}
885887

886888
_, err := cloud.service.RegionDisks.CreateSnapshot(project, volKey.Region, volKey.Name, snapshotToCreate).Context(ctx).Do()

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

+5-1
Original file line numberDiff line numberDiff line change
@@ -609,7 +609,11 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C
609609
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown get snapshot error: %v", err))
610610
}
611611
// If we could not find the snapshot, we create a new one
612-
snapshot, err = gceCS.CloudProvider.CreateSnapshot(ctx, project, volKey, req.Name)
612+
snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(req.GetParameters())
613+
if err != nil {
614+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("Invalid snapshot parameters: %v", err))
615+
}
616+
snapshot, err = gceCS.CloudProvider.CreateSnapshot(ctx, project, volKey, req.Name, snapshotParams)
613617
if err != nil {
614618
if gce.IsGCEError(err, "notFound") {
615619
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volKey.String(), err))

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

+30-1
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ func TestCreateSnapshotArguments(t *testing.T) {
8484
req: &csi.CreateSnapshotRequest{
8585
Name: name,
8686
SourceVolumeId: testVolumeID,
87+
Parameters: map[string]string{common.ParameterKeyStorageLocations: "us-east1, US-WEST2"},
8788
},
8889
seedDisks: []*gce.CloudDisk{
8990
createZonalCloudDisk(name),
@@ -128,6 +129,30 @@ func TestCreateSnapshotArguments(t *testing.T) {
128129
},
129130
expErrCode: codes.InvalidArgument,
130131
},
132+
{
133+
name: "invalid snapshot parameter key",
134+
req: &csi.CreateSnapshotRequest{
135+
Name: name,
136+
SourceVolumeId: testVolumeID,
137+
Parameters: map[string]string{"bad-key": ""},
138+
},
139+
seedDisks: []*gce.CloudDisk{
140+
createZonalCloudDisk(name),
141+
},
142+
expErrCode: codes.InvalidArgument,
143+
},
144+
{
145+
name: "invalid snapshot locations",
146+
req: &csi.CreateSnapshotRequest{
147+
Name: name,
148+
SourceVolumeId: testVolumeID,
149+
Parameters: map[string]string{common.ParameterKeyStorageLocations: "bad-region"},
150+
},
151+
seedDisks: []*gce.CloudDisk{
152+
createZonalCloudDisk(name),
153+
},
154+
expErrCode: codes.InvalidArgument,
155+
},
131156
}
132157

133158
for _, tc := range testCases {
@@ -848,7 +873,11 @@ func TestCreateVolumeWithVolumeSource(t *testing.T) {
848873
}
849874

850875
if tc.snapshotOnCloud {
851-
gceDriver.cs.CloudProvider.CreateSnapshot(context.Background(), tc.project, tc.volKey, name)
876+
snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(req.GetParameters())
877+
if err != nil {
878+
t.Errorf("Got error extracting snapshot parameters: %v", err)
879+
}
880+
gceDriver.cs.CloudProvider.CreateSnapshot(context.Background(), tc.project, tc.volKey, name, snapshotParams)
852881
}
853882
resp, err := gceDriver.cs.CreateVolume(context.Background(), req)
854883
//check response

0 commit comments

Comments
 (0)