Allow pre-binding of Persistent Volumes to PVClaims #14249
Conversation
|
@kubernetes/rh-storage |
|
GCE e2e build/test passed for commit 613fb0e692a6765fe44326bded4e636e99a7191a. |
k8s-github-robot
added
the
size/L
|
Labelling this PR as size/L |
| // A PVClaim containing this annotation requests a quality of service tier. | ||
| // The value of this field is arbitrary and known by the user and administrator. | ||
| // This is an experimental feature and likely to change in the future. | ||
| qualityOfServiceRequestKey = "experimental/k8s.io/persistentVolumeProvisioning-qos" |
There was a problem hiding this comment.
Since it's not yet used anywhere, let's remove this (it can be re-added along with code using it).
There was a problem hiding this comment.
Sure thing. I will re-introduce this with the provisioner. It will look for this on a PVC and provision a volume for it and add the 'createdForKey' annotation.
|
LGTM mod nit |
markturansky
force-pushed
the
prov_claim_annotations
branch
3 times, most recently
from
7102124 to
4d6ecba
Compare
|
thanks @saad-ali. I removed the annotation, as requested. I also changed the value of the 'createdFor' annotation to fit what Tim recently wrote for https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#label-selector-and-annotation-conventions |
|
GCE e2e build/test passed for commit 7102124d45f4850be273cad8e1269b9b55e89c34. |
|
GCE e2e build/test failed for commit 9e42268ba17e6208dc1c6e51a376bb9024ce26cb. |
|
GCE e2e build/test passed for commit 4d6ecba305373abfbe4a476a2d2fb9816a264c84. |
| // A PV created specifically for one claim must contain this annotation in order to bind to the claim. | ||
| // The value must be the name of the claim being bound to. | ||
| // This is an experimental feature and likely to change in the future. | ||
| createdForKey = "provisioning.experimental.kubernetes.io/createdFor" |
There was a problem hiding this comment.
nit: the name provisioning doesn't make it clear that it is PV provisioning (not load balancer provisioning, for example).
There was a problem hiding this comment.
good point. i also saw (after submitting the recent name change) that Tim called for dashes instead of camelCasing in the API doc.
Key value updated to reflect PV specifically and it uses hot looking dashes instead of the old and busted camelCase.
|
LGTM nit on the name, I'll leave it up to you if you want to change it |
markturansky
force-pushed
the
prov_claim_annotations
branch
from
4d6ecba to
15afd78
Compare
|
GCE e2e build/test passed for commit 15afd7896bd75cae014e6355da41fb798794bb58. |
|
Different flake in Shippable. 1 of 2 shippable builds passed.
|
|
@k8s-bot test this |
|
GCE e2e build/test passed for commit 15afd7896bd75cae014e6355da41fb798794bb58. |
|
Same Shippable error as I posted previously. Go 1.4 contains the error (as last time) while Go 1.3 does not. |
| // A PV created specifically for one claim must contain this annotation in order to bind to the claim. | ||
| // The value must be the name of the claim being bound to. | ||
| // This is an experimental feature and likely to change in the future. | ||
| createdForKey = "persistent-volume-provisioning.experimental.kubernetes.io/created-for" |
There was a problem hiding this comment.
we should centralize these tag names somewhere. (pkg/apis/experimental/annotations.go?)
There was a problem hiding this comment.
I can move them if that's where we'd prefer annotations to live. Do we care about scoping at all? These constants package protected and aren't relevant outside of this narrow functionality.
The docs explaining this feature will require the keys, of course.
There was a problem hiding this comment.
that's pretty wordy. how about volume.experimental.kubernetes.io/provisioned-for
There was a problem hiding this comment.
This whole PV feature has been wordy ... but I can type "persistent" very
quickly now, so I've got that going for me.
I'll change it to your suggestion :)
On Wed, Sep 23, 2015 at 5:46 PM, Tim Hockin notifications@github.com
wrote:
In pkg/controller/persistentvolume/types.go
#14249 (comment)
:@@ -25,6 +25,13 @@ import (
"k8s.io/kubernetes/pkg/client/cache"
)+const (
- // A PV created specifically for one claim must contain this annotation in order to bind to the claim.
- // The value must be the name of the claim being bound to.
- // This is an experimental feature and likely to change in the future.
- createdForKey = "persistent-volume-provisioning.experimental.kubernetes.io/created-for"
that's pretty wordy. how about
volume.experimental.kubernetes.io/provisioned-for—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/14249/files#r40261952.
|
LGTM, we're in merge freeze while we try to stabilize Shippable. This is fine to come in after we get out of that. |
markturansky
force-pushed
the
prov_claim_annotations
branch
from
15afd78 to
f2378a2
Compare
|
'else' removed, annotation key name changed to suggestion, and comments improved. |
|
GCE e2e build/test passed for commit f2378a2. |
thockin
added
the
lgtm
Allow pre-binding of Persistent Volumes to PVClaims
@thockin @saad-ali This small PR is necessary per our discussion about dynamic provisioning, but I think it's a good change overall for PVs. It would work for any type of PV, regardless of how it was provisioned.
Allow a PV to be intended for 1 specific claim by adding the annotation:
An admin can use this for any PV they are managing.