Allow users to specify "storage-locations" for snapshots.

[TeweiLuo]

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.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

/kind feature

What this PR does / why we need it:
Allow users to set storage locations[1] for their PD snapshots by specifying them in the snapshot class. For example:

apiVersion: snapshot.storage.k8s.io/v1beta1
kind: VolumeSnapshotClass
metadata:
  name: snapshot-class-with-storage-locations
parameters:
  storage-locations: us-central1
driver: pd.csi.storage.gke.io
deletionPolicy: Delete

Current the default location[2] will be used for all snapshots. GKE customers may have organization policies restricting the placement of snapshots that conflicts with the default location. This features will help customers to consistently apply their policies.

This parameter is described in the API document[3] for snapshots.

Which issue(s) this PR fixes:

Fixes #791

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Users will be able to set the storage locations for snapshots by specifying them in the snapshot class.

[1] https://cloud.google.com/compute/docs/disks/snapshots#custom_location
[2] https://cloud.google.com/compute/docs/disks/snapshots#default_location
[3] https://cloud.google.com/compute/docs/reference/rest/v1/snapshots

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TeweiLuo
To complete the pull request process, please assign verult after the PR has been reviewed.
You can assign the PR to them by writing /assign @verult in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @TeweiLuo. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mauriciopoppe]

I had a question about using an array but I checked that parameters is a map[string]string https://github.com/kubernetes-csi/external-snapshotter/blob/f255fe7b46bd5410370537319879d06f2bf1a156/client/apis/volumesnapshot/v1/types.go#L215, I don't know if users would do this though (NOTE the space between the comma and the other region):

parameters:
  storage-locations: us-central1, us-central2

[mauriciopoppe]

Probably need to add an e2e test too

[TeweiLuo]

Probably need to add an e2e test too

I'm not sure if this should be a k/k e2e test because this param is pd specific. Maybe a GKE internal test is better?

[mauriciopoppe]

got it, I saw a few tests in test/e2e but they seem to be integration tests, sure having a test either here or in GKE internal would be nice

[mattcary]

/okay-to-test

[mauriciopoppe]

/ok-to-test

[mattcary]

We should split, trim and validate the storage locations here (via a helper ftn of course). See for example common.ExtractAndDefaultParameters and its usages as a model.

[TeweiLuo]

I was thinking about the same thing initially but

1. It's debatable whether we should validate the regions themselves - the PD backend may change.
2. If (1) is true, then there is no other validation, because every string can be treated as a comma-separated string.

[mattcary]

discussed offline. The API takes a list of strings, so we definitely need to split on commas, so we should do that early. If we're splitting on commas, we need to trim whitespace, which means we should do a regexp validation of the location to make sure it's plausible. Things like validating the regions will be done over in the PD service anyway and not in the driver, even in the cloud provider.

[TeweiLuo]

Refactored to use a generalized SnapshotParameters struct and added normalization/validation of storage locations. Will add an e2e test.

[mauriciopoppe]

lgtm :)

[mattcary]

Can you add an example to https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/docs/kubernetes/user-guides/snapshots.md?

[TeweiLuo]

Update: while the storageLocations argument for the snapshot REST API is of type []string but it must be one location only. The only documentation I found is this sentence

Determine your snapshot storage location. You can use the default storage location, or a custom storage location.

from this page. I'll amend the PR accordingly.

[TeweiLuo]

Can you

Doc updated.

[mattcary]

thanks!

/lgtm

[k8s-ci-robot]

@TeweiLuo: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration	31a8f87	link	/test pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.