Allow to label PD disk with k8s cluster ID #693

tsmetana · 2021-01-11T09:41:39Z

/kind feature

What this PR does / why we need it:
Inspired by the similar feature of AWS EBS CSI driver: allow to optionally label the PD disk with cluster ID to denote the disk ownership. This allows e.g. better automated resource cleanup on cluster teardown.

Special notes for your reviewer:
The label was meant to be named kubernetes.io/cluster/<cluster ID> (again, taken from the EBS CSI driver) however GCE API does not allow '/' or '.' characters in the label, so I swapped them for underscores. I can change it to anything else, I'm more interested in comments on the general approach.

Does this PR introduce a user-facing change?:

A new `k8s-tag-cluster-id` command line option has been added. If specified, the resulting PD disk will be labelled with "kubernetes_io_cluster_<cluster ID>": "owned".

k8s-ci-robot · 2021-01-11T09:41:47Z

Welcome @tsmetana!

It looks like this is your first PR to kubernetes-sigs/gcp-compute-persistent-disk-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gcp-compute-persistent-disk-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-01-11T09:41:48Z

Hi @tsmetana. 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.

tsmetana · 2021-01-11T09:43:35Z

@jsafrane: FYI...

mattcary · 2021-01-11T17:05:09Z

Thanks Thomas. I haven't had a chance to look at this yet. Have you seen #566? The author We were basically happy with that PR, but the author seems to have dropped out. We want to get that in soon though.

If we had the ability of that CL (to label the PD disk via storage class parameters), would that fit your use case? That might be more flexible than a flag with a hardcoded label.

msau42 · 2021-01-11T19:09:54Z

/ok-to-test

msau42 · 2021-01-11T19:12:09Z

If we had the ability of that CL (to label the PD disk via storage class parameters), would that fit your use case? That might be more flexible than a flag with a hardcoded label.

I think the main downside is then people can't use the default storageclass, unless we had a way to generate default storageclasses and inject the cluster name into it (k8s doesn't have a first class concept of cluster name)

mattcary · 2021-01-11T20:27:22Z

I think the main downside is then people can't use the default storageclass, unless we had a way to generate default storageclasses and inject the cluster name into it (k8s doesn't have a first class concept of cluster name)

Since this proposal involves a command-line flag for the driver, anyone who can install the driver can also change the default storageclass to have whatever label they want.

Since this label isn't going to match the AWS EBS label anyway due to the slash-vs-underscore issue, there probably needs to be flexibility for the operator to tweak the label to what is most convenient for their tools. Doing that via the storageclass parameter seems nicer than via a driver command-line argument.

My $0.02 at least.

msau42 · 2021-01-11T20:41:34Z

previous discussion: #469

tsmetana · 2021-01-12T16:07:42Z

I think the main downside is then people can't use the default storageclass, unless we had a way to generate default storageclasses and inject the cluster name into it (k8s doesn't have a first class concept of cluster name)

Since this proposal involves a command-line flag for the driver, anyone who can install the driver can also change the default storageclass to have whatever label they want.

The users able to install drivers and those able to create storage classes might be different. The aim is to be able to create an environment where the clusters get automatically set-up (click in a browser) and torn-down (another click) and don't leave any leftovers while still not require any additional responsibilities from their admins (not forget to label StorageClasses).

mattcary · 2021-01-22T00:43:34Z

The users able to install drivers and those able to create storage classes might be different. The aim is to be able to create an environment where the clusters get automatically set-up (click in a browser) and torn-down (another click) and don't leave any leftovers while still not require any additional responsibilities from their admins (not forget to label StorageClasses).

Okay, that makes sense. I think fixing the label format here will be too restrictive. I'd support a change that was something like --additional-pd-label, eg --additional-pd-label=k8s-cluster-id-112233. The flag could even be given multiple times, for multiple labels.

jsafrane · 2021-01-22T15:04:24Z

AWS has --extra-tags=foo=bar,baz=qux, maybe --extra-labels? Not sure if pd should be in the parameter - should it affect also snapshots?

mattcary · 2021-01-22T16:56:47Z

AWS has --extra-tags=foo=bar,baz=qux, maybe --extra-labels? Not sure if pd should be in the parameter - should it affect also snapshots?

+1 to both the name as well as labeling snapshots in addition to disks.

tsmetana · 2021-02-02T12:56:25Z

Changed to allow generic disk labelling. PTAL.

mattcary

Just a couple of nits.

Thanks!

mattcary · 2021-02-05T01:59:32Z

pkg/common/parameters_test.go

 			expectParams: DiskParameters{
 				DiskType:             "pd-standard",
 				ReplicationType:      "none",
 				DiskEncryptionKMSKey: "",
 				Tags:                 map[string]string{tagKeyCreatedForClaimName: "testPVCName", tagKeyCreatedForClaimNamespace: "testPVCNamespace", tagKeyCreatedForVolumeName: "testPVName", tagKeyCreatedBy: "testDriver"},
+				Labels:               make(map[string]string),


Did I miss it or shouldn't there be a test case for the new labels?

I changed the code since ExtractAndDefaultParameter now takes more parameters and to have the parameteres structure complete. There's nothing much to test with the labels though -- they're only being copied.

If they're only we being copied we should test that---these also serve as regression tests.

I added the test in the latest commit.

mattcary · 2021-02-05T02:00:31Z

go.sum

@@ -476,9 +476,6 @@ github.com/kr/pty v1.1.3/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/pty v1.1.5/go.mod h1:9r2w37qlBe7rQ6e1fg1S/9xpWHSnaqNdHD3WcMdbPDA=
 github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
-github.com/kubernetes-csi/csi-proxy v0.2.2 h1:LqablYFEGw7FYBjwoh5TeXFzlcx8C+YQjKfGy6fFWJs=


I don't think we want to remove the csi-proxy? It's used by windows. At any rate we should clean this up in a separate PR.

@jingxu97 We seem to have both 0.2.1 and 0.2.2 of the proxy here, not sure what's going on.

At any rate this PR should not be changing go.mod (sorry I wasn't clear on that).

OK. I'll fix that.

The above is a result of go mod tidy. I use k8s.io/component-base/cli/flag for the CLI parsing and it was not vendored yet so I actually have to update go.mod for this patch. I think the removal of the (older) csi-proxy v0.2.1 is correct and the v0.2.2 is listed twice in go.sum.

Ok, fair enough. I'm not going to argue with go mod tidy.

mattcary

/lgtm
/approve

Thanks for working through this!

mattcary · 2021-02-12T17:13:01Z

go.sum

@@ -476,9 +476,6 @@ github.com/kr/pty v1.1.3/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/pty v1.1.5/go.mod h1:9r2w37qlBe7rQ6e1fg1S/9xpWHSnaqNdHD3WcMdbPDA=
 github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
-github.com/kubernetes-csi/csi-proxy v0.2.2 h1:LqablYFEGw7FYBjwoh5TeXFzlcx8C+YQjKfGy6fFWJs=


Ok, fair enough. I'm not going to argue with go mod tidy.

k8s-ci-robot · 2021-02-12T17:14:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary, tsmetana

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattcary]

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

Cherrypick #682, #684, #693, #703, #718, #719, #720 to release-1.2 from master

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 11, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 11, 2021

k8s-ci-robot requested review from davidz627 and mattcary January 11, 2021 09:41

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 11, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2021

tsmetana force-pushed the add-k8s-cluster-label branch from 2d73433 to 0b51c07 Compare February 2, 2021 12:55

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 2, 2021

mattcary reviewed Feb 5, 2021

View reviewed changes

Add option to specify extra PD labels for the provisioned disk.

68cdfa9

tsmetana force-pushed the add-k8s-cluster-label branch from 0b51c07 to 68cdfa9 Compare February 12, 2021 14:17

mattcary reviewed Feb 12, 2021

View reviewed changes

k8s-ci-robot assigned mattcary Feb 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2021

k8s-ci-robot merged commit be9a453 into kubernetes-sigs:master Feb 12, 2021

StokeHead mentioned this pull request Feb 23, 2021

Add gce disk labels support via storageclass/CreateVolumeRequest parameters #566

Closed

saikat-royc mentioned this pull request Mar 24, 2021

Cherrypick #682, #684, #693, #703, #718, #719, #720 to release-1.2 from master #730

Merged

k8s-ci-robot added a commit that referenced this pull request Mar 24, 2021

Merge pull request #730 from saikat-royc/release-1.2

4b1d767

Cherrypick #682, #684, #693, #703, #718, #719, #720 to release-1.2 from master

tsmetana mentioned this pull request May 30, 2023

Allow to add extra volume labels from command line option kubernetes-sigs/gcp-filestore-csi-driver#526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to label PD disk with k8s cluster ID #693

Allow to label PD disk with k8s cluster ID #693

tsmetana commented Jan 11, 2021

k8s-ci-robot commented Jan 11, 2021

k8s-ci-robot commented Jan 11, 2021

tsmetana commented Jan 11, 2021

mattcary commented Jan 11, 2021

msau42 commented Jan 11, 2021

msau42 commented Jan 11, 2021

mattcary commented Jan 11, 2021

msau42 commented Jan 11, 2021

tsmetana commented Jan 12, 2021

mattcary commented Jan 22, 2021

jsafrane commented Jan 22, 2021

mattcary commented Jan 22, 2021

tsmetana commented Feb 2, 2021

mattcary left a comment

mattcary Feb 5, 2021

tsmetana Feb 8, 2021

mattcary Feb 8, 2021

tsmetana Feb 12, 2021

mattcary Feb 5, 2021

mattcary Feb 8, 2021

tsmetana Feb 10, 2021

tsmetana Feb 12, 2021

mattcary Feb 12, 2021

mattcary left a comment

mattcary Feb 12, 2021

k8s-ci-robot commented Feb 12, 2021

Allow to label PD disk with k8s cluster ID #693

Allow to label PD disk with k8s cluster ID #693

Conversation

tsmetana commented Jan 11, 2021

k8s-ci-robot commented Jan 11, 2021

k8s-ci-robot commented Jan 11, 2021

tsmetana commented Jan 11, 2021

mattcary commented Jan 11, 2021

msau42 commented Jan 11, 2021

msau42 commented Jan 11, 2021

mattcary commented Jan 11, 2021

msau42 commented Jan 11, 2021

tsmetana commented Jan 12, 2021

mattcary commented Jan 22, 2021

jsafrane commented Jan 22, 2021

mattcary commented Jan 22, 2021

tsmetana commented Feb 2, 2021

mattcary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 12, 2021