add pvc metadata to createvolume req #399

zetsub0u · 2020-01-17T21:25:57Z

Co-authored-by: abrden agustina.barbetta@gmail.com

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

What this PR does / why we need it:
Add PVC metadata to plugin call in the CreateVolume operation for it to be able to query for more information if needed during volume creation.
Which issue(s) this PR fixes:

Fixes #370

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

* Adds --extraCreateMetadata flag which, when enabled, will inject parameters onto CreateVolume driver requests with PVC and PV metadata.
  Injected keys:
  - csi.storage.k8s.io/pvc/name
  - csi.storage.k8s.io/pvc/namespace
  - csi.storage.k8s.io/pv/name

k8s-ci-robot · 2020-01-17T21:26:01Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

k8s-ci-robot · 2020-01-17T21:26:05Z

Welcome @zetsub0u!

It looks like this is your first PR to kubernetes-csi/external-provisioner 🎉. 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-csi/external-provisioner 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 · 2020-01-17T21:26:05Z

Hi @zetsub0u. Thanks for your PR.

I'm waiting for a kubernetes-csi 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.

aibarbetta · 2020-01-17T21:31:11Z

/assign @msau42

davidz627 · 2020-01-18T02:22:43Z

@zetsub0u please sign the CLA
/ok-to-test

zetsub0u · 2020-01-18T02:43:19Z

Hi, yes i followed the steps but i'm not getting the document to sign via email and i'm trying to figure out what to do now. Also i'll check the unittests, i completely forgot to run them as it was a simple change.

zetsub0u · 2020-01-19T17:20:52Z

CLA signed, i guess need to wait for the bot to retest.

davidz627

looks good, overall I think the major thing is we'd want to gate this on the binary and have it off by default. cc @jsafrane @saad-ali @msau42

davidz627 · 2020-01-24T01:32:24Z

pkg/controller/controller.go

@@ -101,6 +101,11 @@ const (
 	nodePublishSecretNameKey      = "csiNodePublishSecretName"
 	nodePublishSecretNamespaceKey = "csiNodePublishSecretNamespace"

+	// PVC's name and namespace passed on create requests, useful for the plugin to query for extra data
+	// prior to performing operations
+	pvcNameKey      = "kubernetes.io/created-for/pvc/name"


can we also add pv name as one of these?

We thought about this but, the PV name is generated in the provisioner and passed in the request as the Name field already and always, do you see any need for adding it into the parameters also?

would prefer it is added to the parameters as well. Using it as the name is just an implementation choice in the provisioner not an explicit contract so it could technically change.

davidz627 · 2020-01-24T01:34:54Z

pkg/controller/controller.go

@@ -567,6 +572,10 @@ func (p *csiProvisioner) ProvisionExt(options controller.ProvisionOptions) (*v1.
 		return nil, controller.ProvisioningFinished, fmt.Errorf("failed to strip CSI Parameters of prefixed keys: %v", err)
 	}

+	// add pvc metadata to request for use by the plugin
+	req.Parameters[pvcNameKey] = options.PVC.GetName()


we should gate this behind a feature flag or binary flag of some sort. This is a breaking change for drivers that do validation on parameters and don't expect extra stuff coming in. (this should default to off and you can turn it on if you want this information)

It seems like the featureGate flag used here is being pulled from the api server, i'm not too sure if i should extend it with custom provisioner-only feature gates or just add a boolean flag say --inject-pvc-metadata, what do you think is the best approach?

I think a boolean flag is fine. I suck at naming so I can't come up with another solution but I don't think it should contain pvc in it since we will inject pv/pvc name. It should also have some sort of information of "where" it is injecting the metadata. extra-create-params-metadata?

Can you add a comment disclaimer as a flag comment along the lines of:
This metadata should only be used for tagging or descriptive purposes and should in no circumstances be consumed and acted on programatically by a CSI Driver.

Are we going to follow some sort of Alpha->Beta->GA plan for this flag?
If so can we include the stage in the name '--extra-create-params=alpha-pvc'?

no need for a stage - just a single flag should be fine

Or do you prefer

--enable-extra-metadata-for-create-req If set, add pv/pvc metadata to plugin create requests as parameters.

I have both implement, just let me know which makes more sense.

I personally prefer the feature gate and have it go to default at some point.

we can't set this as a default unless we bump the major version of the provisioner - it is a breaking change to drivers as this is adding "unexpected" parameters to their CreateVolume API call. I don't really see the value of feature gating this as the intention for that is to slowly "graduate" features to minimize risk. I wouldn't really consider this a risky feature in terms of potential impact or implementation

Ok, i'll add a standard flag for this, any suggestions on naming? Anything else needing change? I couldn't find any place to document this feature for consumption...

Also open an issue to track changing it to default, and we can add it to the 2.0 milestone

davidz627

mostly lgtm

davidz627 · 2020-01-28T23:30:10Z

cmd/csi-provisioner/csi-provisioner.go

+	leaderElectionType              = flag.String("leader-election-type", "endpoints", "the type of leader election, options are 'endpoints' (default) or 'leases' (strongly recommended). The 'endpoints' option is deprecated in favor of 'leases'.")
+	leaderElectionNamespace         = flag.String("leader-election-namespace", "", "Namespace where the leader election resource lives. Defaults to the pod namespace if not set.")
+	strictTopology                  = flag.Bool("strict-topology", false, "Passes only selected node topology to CreateVolume Request, unlike default behavior of passing aggregated cluster topologies that match with topology keys of the selected node.")
+	enableExtraMetadataForCreateReq = flag.Bool("enable-extra-metadata-for-create-req", false, "If set, add pv/pvc metadata to plugin create requests as parameters.")


nit: enable not necessary (implied by boolean flag), req not required (implied by create)
extra-create-metadata is probably sufficient

Done, i hate naming things :)

davidz627 · 2020-01-28T23:31:17Z

pkg/controller/controller.go

+	// PV and PVC metadata, used for sending to drivers in the  create requests, added as parameters, optional.
+	pvcNameKey      = "kubernetes.io/created-for/pvc/name"
+	pvcNamespaceKey = "kubernetes.io/created-for/pvc/namespace"
+	pvNameKey       = "kubernetes.io/created-for/pv/name"


cc @saad-ali is this an acceptable prefix? maybe it should be one of the volume specific prefixes?

I would prefer to use the same prefix as for pod info on mount: csi.storage.k8s.io/...

Sounds good, updated.

zetsub0u · 2020-01-30T21:14:40Z

@davidz627 Let me know if you need any other changes. Thanks.

cmd/csi-provisioner/csi-provisioner.go

jsafrane · 2020-02-03T09:33:15Z

pkg/controller/controller.go

+	pvcNameKey      = "csi.storage.k8s.io/created-for/pvc/name"
+	pvcNamespaceKey = "csi.storage.k8s.io/created-for/pvc/namespace"
+	pvNameKey       = "csi.storage.k8s.io/created-for/pv/name"


Would it make sense to use the same schema we use in NodePublish?

csi.storage.k8s.io/pvc/name csi.storage.k8s.io/pvc/namespace csi.storage.k8s.io/pv/name

I know CSI drivers that support migration need to translate it to created-for/ format, still, it would make life of all the non-migrating drivers a bit easier.

jsafrane · 2020-02-03T09:42:08Z

Oh, and please edit the first comment and add useful release note there (instead of None) - this is user visible change and actually it's probably going to be highlight of the new release ;-).

zetsub0u · 2020-02-06T17:53:34Z

Updated the keys for the parameters and the Release notes.

davidz627 · 2020-02-07T01:21:41Z

@zetsub0u Thanks for the change! Please squash commits to 1 well described commit :)
/approve

Adds --extraCreateMetadata flag which, when enabled, will inject parameters onto CreateVolume driver requests with PVC and PV metadata. Injected keys: - csi.storage.k8s.io/pvc/name - csi.storage.k8s.io/pvc/namespace - csi.storage.k8s.io/pv/name Co-authored-by: abrden <agustina.barbetta@gmail.com>

jsafrane · 2020-02-07T09:27:52Z

/lgtm
thanks a lot!

jsafrane · 2020-02-07T09:28:20Z

/approve

k8s-ci-robot · 2020-02-07T09:28:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, jsafrane, zetsub0u

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~~ [jsafrane]

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

zetsub0u · 2020-02-07T13:56:18Z

Thanks! We really needed this feature and didn't want to fork :)

the external-csi provisioner optionally injects metadata in the CreateVolume that we now preserve as tags on the volume. kubernetes-csi/external-provisioner#399

The external CSI provisioner optionally injects metadata in the CreateVolume that we can pass onto the volume as additional tags. See kubernetes-csi/external-provisioner#399 Signed-off-by: Fabian Ruff <fabian.ruff@sap.com>

* [cinder-csi-plugin] Tag volume with optional metadata The external CSI provisioner optionally injects metadata in the CreateVolume that we can pass onto the volume as additional tags. See kubernetes-csi/external-provisioner#399 Signed-off-by: Fabian Ruff <fabian.ruff@sap.com> * Add --extra-create-metadata to csi-provisoner manifests Signed-off-by: Fabian Ruff <fabian.ruff@sap.com> * Add unit test for extra metadata Signed-off-by: Fabian Ruff <fabian.ruff@sap.com> * Bump chart again. Signed-off-by: Fabian Ruff <fabian.ruff@sap.com>

…tes#1492) * [cinder-csi-plugin] Tag volume with optional metadata The external CSI provisioner optionally injects metadata in the CreateVolume that we can pass onto the volume as additional tags. See kubernetes-csi/external-provisioner#399 Signed-off-by: Fabian Ruff <fabian.ruff@sap.com> * Add --extra-create-metadata to csi-provisoner manifests Signed-off-by: Fabian Ruff <fabian.ruff@sap.com> * Add unit test for extra metadata Signed-off-by: Fabian Ruff <fabian.ruff@sap.com> * Bump chart again. Signed-off-by: Fabian Ruff <fabian.ruff@sap.com>

Update CHANGELOG-4.1.md

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 17, 2020

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 17, 2020

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 17, 2020

k8s-ci-robot requested review from davidz627 and msau42 January 17, 2020 21:26

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 17, 2020

k8s-ci-robot assigned msau42 Jan 17, 2020

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 18, 2020

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 18, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 19, 2020

davidz627 reviewed Jan 24, 2020

View reviewed changes

davidz627 reviewed Jan 28, 2020

View reviewed changes

yoesmat mentioned this pull request Jan 29, 2020

Adding PVC's annotation to CreateVolumeRequest.Parameters #86

Open

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2020

zetsub0u force-pushed the master branch from 43a2937 to a28ef12 Compare January 30, 2020 20:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2020

jsafrane reviewed Feb 3, 2020

View reviewed changes

zetsub0u force-pushed the master branch from 37bc294 to 53f0949 Compare February 7, 2020 02:16

k8s-ci-robot assigned jsafrane Feb 7, 2020

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

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

k8s-ci-robot merged commit 4aa560e into kubernetes-csi:master Feb 7, 2020

jsafrane mentioned this pull request Feb 11, 2020

PVC.UID for volume name unique but hard for humans to differentiate #67

Closed

Madhu-1 mentioned this pull request Feb 25, 2020

Make dynamically provisioned volumes (PVC-PV-ceph) recognizably related ceph/ceph-csi#780

Closed

msau42 mentioned this pull request Feb 27, 2020

Clarify why kubernetes tag prefix is disallowed kubernetes-sigs/aws-ebs-csi-driver#450

Closed

dillaman mentioned this pull request Feb 29, 2020

Manage RBD image mirroring via StorageClass parameter ceph/ceph-csi#676

Closed

Madhu-1 mentioned this pull request Mar 12, 2020

Store additional metadata about RBD Image owner(PVC name) in RBD Image ceph/ceph-csi#72

Closed

MrRexZ mentioned this pull request Mar 26, 2020

[pure-csi] More human-readable name for dynamically created volumes purestorage/helm-charts#197

Open

mittachaitu mentioned this pull request Jun 4, 2020

chore(csi-driver): update driver to read PVC, PV name in CreateVolume request openebs-archive/cstor-csi#90

Merged

6 tasks

timoreimann mentioned this pull request Jul 26, 2020

Feature Request : Allow created resources to have a name prefix digitalocean/csi-digitalocean#30

Open

saad-ali mentioned this pull request Aug 6, 2020

Add metadata info in tag on PD kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#570

Merged

edanc mentioned this pull request Mar 11, 2021

Add PVC and PV metadata as tags to EFS access points kubernetes-sigs/aws-efs-csi-driver#349

Closed

databus23 mentioned this pull request Apr 22, 2021

[cinder-csi-plugin] Tag volume with optional pvc/pv metadata kubernetes/cloud-provider-openstack#1492

Merged

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#399 from sunnylovestiramisu/changelog

0e00c8a

Update CHANGELOG-4.1.md

add pvc metadata to createvolume req #399

add pvc metadata to createvolume req #399

Conversation

zetsub0u commented Jan 17, 2020 • edited

k8s-ci-robot commented Jan 17, 2020

k8s-ci-robot commented Jan 17, 2020

k8s-ci-robot commented Jan 17, 2020

aibarbetta commented Jan 17, 2020

davidz627 commented Jan 18, 2020

zetsub0u commented Jan 18, 2020

zetsub0u commented Jan 19, 2020

davidz627 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

zetsub0u Jan 24, 2020 • edited

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

davidz627 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

zetsub0u commented Jan 30, 2020

Choose a reason for hiding this comment

jsafrane commented Feb 3, 2020

zetsub0u commented Feb 6, 2020

davidz627 commented Feb 7, 2020

jsafrane commented Feb 7, 2020

jsafrane commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

zetsub0u commented Feb 7, 2020

zetsub0u commented Jan 17, 2020 •

edited

zetsub0u Jan 24, 2020 •

edited