Add gce disk labels support via storageclass/CreateVolumeRequest parameters

[ernestas2k]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds support to assign gce disk labels via storageclass / CreateVolumeRequest parameters.

Which issue(s) this PR fixes:

Fixes #340

Special notes for your reviewer:

• Any test I shouldn't have altered or should additionally update?
• note: API contract has changed - support of new "labels" parameter added.

Does this PR introduce a user-facing change?:

Added support to assign gce disk labels by providing storageclass parameter `labels`.

[k8s-ci-robot]

Welcome @ernestas2k!

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]

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

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ernestas2k
To complete the pull request process, please assign mattcary
You can assign the PR to them by writing /assign @mattcary 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

[ernestas2k]

/assign @mattcary

[msau42]

/ok-to-test

[mattcary]

Thanks for your PR!

A couple of minor nits, LMK what you think.

[mattcary]

(dropping some notes here for future reference)

naming looks good, 'labels' is consistent with GCP label APIs:
https://cloud.google.com/sdk/gcloud/reference/compute/disks/add-labels
https://cloud.google.com/resource-manager/docs/creating-managing-labels

[ernestas2k]

Thanks @mattcary for fast feedback. I agree with all of the observations and I'll prepare updates.

Regarding the validation, absolutely let's do it, with slight caveat that changes on google side will have to be reflected in the code too (hopefully labels api won't change often).

---

Also, is there any integration test that I should update or actually introduce for this feature?

The kubernetes-integration and verify jobs failed:
verify: E0729 17:31:41.945] Build timed out
integration: W0729 17:52:03.603] - The zone 'projects/k8s-boskos-gce-project-07/zones/us-central1-b' does not have enough resources available to fulfill the request. Try a different zone, or try again later.

Is there anything to be done from my side regarding these failures?

---

PS: just pushed newest changes to address the comments (please take a look if you are ok with regexps, error messages, etc). I'm ready to rework it so it's not too different from other functionality.

[mattcary]

Regarding the validation, absolutely let's do it, with slight caveat that changes on google side will have to be reflected in the code too (hopefully labels api won't change often).

Great, thanks.

Also, is there any integration test that I should update or actually introduce for this feature?

That's a great question. The storage class test/k8s-integrations/configs/sc-standard.yaml is what gets used for the integration tests, adding a label there would be a good idea.

The kubernetes-integration and verify jobs failed:
verify: E0729 17:31:41.945] Build timed out

It succeeded after your last push. You can run it manually with hack/verify-all.sh.

integration: W0729 17:52:03.603] - The zone 'projects/k8s-boskos-gce-project-07/zones/us-central1-b' does not have enough resources available to fulfill the request. Try a different zone, or try again later.

Trying again later :-)

Is there anything to be done from my side regarding these failures?

PS: just pushed newest changes to address the comments (please take a look if you are ok with regexps, error messages, etc). I'm ready to rework it so it's not too different from other functionality.

Will look shortly, thanks!

[ernestas2k]

Awesome, I just added e2e test case It("Should create and delete disk with labels", func() {. To be honest, I lean more towards updating existing test case, such as It("Should create and delete disk with default zone", func() {.

What are your thoughts on this? 👍

[mattcary]

Awesome, I just added e2e test case It("Should create and delete disk with labels", func() {. To be honest, I lean more towards updating existing test case, such as It("Should create and delete disk with default zone", func() {.

What are your thoughts on this? 👍

I prefer adding a new test case, if it starts failing it will be a lot easier to tell what's going on; eg if the non-label version is still succeeding than we immediately know what's going on.

I think it would be good to add a label to the storage class in test/k8s-integeration/configs. This will be used for the k/k e2e tests and would be good to ferret out if something is depending on the storage class parameters in a way it shouldn't be. Also it will run on real PDs and give us, well, a real e2e test.

[mattcary]

I think it would be good to add a label to the storage class in test/k8s-integeration/configs. This will be used for the k/k e2e tests and would be good to ferret out if something is depending on the storage class parameters in a way it shouldn't be. Also it will run on real PDs and give us, well, a real e2e test.

Oops, you did that already. Thanks!

[ernestas2k]

Updated the PR with requested changes. I appreciate the comments and quick feedback 👍

[mattcary]

What would be the behavior of validating exisiting disk in CreateVolume calls? do we comapre labels too?

AFAICT we don't have the PV plumbed through so it would be hard to check even if we wanted to. We also would only want to check the labels in the case where the PV was dynamically provisioned, information which we also don't seem to have.

Also, if the user were to delete the labels, and we enforced validation, it would be very hard to recreate them correctly. Since AFAIU the labels are used for informational purposes (accounting etc) it would be better to have a tool to recreate them appropriately rather than risk breaking storage usage if they get out of sync.

[mattcary]

@ernestas2k any updates---are you waiting on anything?

[msau42]

Since the request for supporting the k8s metadata came in late, would it be simpler to split up into two PRs and treat the k8s metadata separately?

[ernestas2k]

I'm in melancholic autumn mood right now, but since you pinged me I'll address your points this week 👍 . Unless you reconsider and split the task. Until then I'll address all of the points added.

[mattcary]

I'm in melancholic autumn mood right now, but since you pinged me I'll address your points this week 👍 . Unless you reconsider and split the task. Until then I'll address all of the points added.

Thanks! Summer is still in full swing here in Seattle and I still have a month before the autumn rains set in :-)

I think if you just make the test improvements Saikat mentioned, I'm okay with leaving the tag addition to a future CL. Let me know if you agree and I'll make in issue to track the tags (we're working on the filestore driver and will be adding labels in the same way).

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[ernestas2k]

Just updated the PR, addressed comments on test cases.

Regarding kubernetes tags and sanitizing keys/values. Pretty straightforward change, but I would be surprized when strings that I've passed are accepted and silently converted. Take parameter my_company/my_object_id=something/something-1-2-3 - conversion would change my key and value in some way that makes me not be able to query the disk anymore. At least, it would be a big surprise.

At most, I'd go with sanitizing the predefined keys of tag* constants (for example tagKeyCreatedForClaimNamespace = "kubernetes.io/created-for/pvc/namespace"). Tag values are also okay if it's guaranteed their values are always 'compliant' with google rules or the actual values aren't really that important.

What are your thoughts?

[mattcary]

Just updated the PR, addressed comments on test cases.

Regarding kubernetes tags and sanitizing keys/values. Pretty straightforward change, but I would be surprized when strings that I've passed are accepted and silently converted. Take parameter my_company/my_object_id=something/something-1-2-3 - conversion would change my key and value in some way that makes me not be able to query the disk anymore. At least, it would be a big surprise.

At most, I'd go with sanitizing the predefined keys of tag* constants (for example tagKeyCreatedForClaimNamespace = "kubernetes.io/created-for/pvc/namespace"). Tag values are also okay if it's guaranteed their values are always 'compliant' with google rules or the actual values aren't really that important.

What are your thoughts?

But since these labels are actually put on the PDs, what happens if an invalid character is passed through? Will PD creation fail or will something be silently dropped?

I think the common case would be that users would prefer to have labels actually created rather than having PD creation fail (which will occur some time after the SC creation and so be hard to debug).

We could also think about a validating webhook as is currently being done for snapshotting.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[k8s-ci-robot]

@ernestas2k: PR needs rebase.

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.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[mattcary]

/remove-lifecycle rotten
/lifecycle frozen

[StokeHead]

Just wanted to check if this issue is still being worked on?
I followed #693 and #570 , however both dont seem to exactly implement what i would need.

We are applying different actions on GCE PDs according to the originating StorageClass. However with the description field implemented in #570 i am only able to see the PVC/PV name. Without querying the cluster itself its therefore not possible to get the originating StorageClass. (Or am i missing something?)
Having the possibility to add custom labels, or extending the description field with the originating storage class should either be of help in my use case.

[mattcary]

I'll take over this PR and get it in. Watch #340 for the status.

/close

[k8s-ci-robot]

@mattcary: Closed this PR.

In response to this:

I'll take over this PR and get it in. Watch #340 for the status.

/close

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.