Readable volume names #334
Conversation
…re enabled, use a hash of the PVC namespace and name in place of the UID, making the entire PV name deterministic
…rovisioner into readable-volume-names
…rovisioner into readable-volume-names
Signed-off-by: Grant Griffiths <ggp493@gmail.com> Backport kubernetes-csi#274 from the master
[release-1.2] Add secret support for Provision and Delete from pvc name and namespace
With the current implementation, In delayed binding case, CSI driver is offered with all nodes topology that are matched with 'selected node' topology keys in CreateVolumeRequest.AccessibilityRequirements. So this allows the driver to select any node from the passed preferred list to create volume. But this results in scheduling failure when the volume created on a node other than Kubernetes selected node. To address this, introduced new flag "--strict-topology', when set, in case of delayed binding, the driver is offered with only selected node topology, so that driver has to create the volume on this node. Modified tests so that now every test is run with and without 'strict topology'.
[release-1.2] Introduce new flag - strict-topology
…re Disk/File, as well as fixes for GCE PD
[cherry-pick] Bump version of csi-translation-lib for CSI Migration support for Azure Disk/File, as well as fixes for GCE PD
…r election processes from running; Leader election namespacing
…ble-in-lib-1.12 Cherry-pick of kubernetes-csi#296: "Leader election: disable duplicate LE in provisioner lib; add lock namespacing"
Cherry-pick 1.2 changelog to release-1.2
Cherry-pick kubernetes-csi#302 to release-1.2: update readme with --leader-election-namespace flag
…rovisioner into readable-volume-names
This reverts commit e3d22b0.
|
Welcome @mcronce! |
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @mcronce. Thanks for your PR. I'm waiting for a kubernetes-csi or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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
removed
the
needs-ok-to-test
| pvcHash := md5.Sum([]byte(pvcNamespace + "/" + pvcName)) | ||
| pvcNamespace = strings.Replace(pvcNamespace, "-", "", -1) | ||
| pvcName = strings.Replace(pvcName, "-", "", -1) | ||
| fullName = fmt.Sprintf("%s-%s-%x", pvcNamespace, pvcName, pvcHash) |
There was a problem hiding this comment.
Both namespace name and PV names can be up 63 characters long. PV name is also limited to 63 characters. With long namespace names, there may be no space for PVC name nor the hash and the resulting name is not unique.
There was a problem hiding this comment.
I didn't realize PV names were limited to 63 characters; however, even with that in mind, it's going to be desirable for some administrators (me, for example) to trade having to be careful about namespace/PVC naming for being able to find storage more easily.
I can certainly add more clarity to the help text on the flag.
There was a problem hiding this comment.
Both namespace name and PV names can be up 63 characters long. PV name is also limited to 63 characters
I checked today and I was wrong, namespace name is has limit 63 characters, but PVC name can have 253 characters. CSI specifies that any string, incl. volume name, can have up to 128 characters. So you don't have even place for full PVC name, not mentioning the namespace.
|
Please do not use merge commits, branch always from master branch and rebase periodically. |
|
And if you find a better way how to compute unique, yet human friendly names, please add unit tests. |
|
The PVC name + namespace is available in the PV object. Is it sufficient when listing PV objects to have it output the PVC name + namespace? |
| @@ -53,6 +53,7 @@ var ( | |||
| _ = deprecatedflags.Add("connection-timeout") | |||
| volumeNamePrefix = flag.String("volume-name-prefix", "pvc", "Prefix to apply to the name of a created volume.") | |||
| volumeNameUUIDLength = flag.Int("volume-name-uuid-length", -1, "Truncates generated UUID of a created volume to this length. Defaults behavior is to NOT truncate.") | |||
| volumeNamesReadable = flag.Bool("volume-names-readable", false, "If enabled, includes the PVC namespace and name in VolumeRequests' suggested names. Note that, combined with --volume-name-uuid-length, this can cause naming collisions.") | |||
There was a problem hiding this comment.
Name collision can lead to stealing data. If attacker crafts namespace + pvc name that leads to collision with existing volume, CSI driver, obeying idempotency, must return the same volume as someone else is already using. Kubernetes is multi-tenant system and must not allow stealing data between namespaces.
There was a problem hiding this comment.
Yes; that is a trade off that must be considered when using this option. Some of us are not administrating multi-tenant Kubernetes systems. We're administering single-tenant Kubernetes systems that, for example, require an easy to way to find a given application's data in the storage provider.
k8s-ci-robot
added
the
needs-rebase
Unfortunately, it does not. When I'm on a storage node I don't have access to the Kubernetes API. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
…rovisioner into readable-volume-names
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mcronce The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
removed
the
needs-rebase
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. In response to this:
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
added
the
needs-rebase
|
@mcronce: 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. |
|
/reopen |
|
@mcronce: Reopened this PR. In response to this:
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. |
|
@mcronce: The following tests failed, say
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. |
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. In response to this:
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. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds an option for the dynamic provisioner to produce PVs with human-readable names. This enables the storage administrator to go straight to the storage layer, usually[1] without having to detour to the Kubernetes API to ask about PV names, to manually inspect or hand-edit an application's config/database/etc.
[1] Even with this enabled, in cases where you have lots of PVCs with similar names in similarly-named namespaces, you might still need to ask the Kubernetes API to find the right PV.
Which issue(s) this PR fixes:
Fixes #67
Special notes for your reviewer:
I've had these changes made for nine months and have been merging master in periodically, using a container image built from my fork in a staging environment. It's definitely passed the test of time. :)
Does this PR introduce a user-facing change?: