Take mount options to GA by adding PV.spec.mountOptions #50919
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
kind/api-change
|
@kubernetes/sig-storage-api-reviews |
k8s-ci-robot
added
sig/storage
wongma7
force-pushed
the
mount-options
branch
2 times, most recently
from
efb3f50 to
bb67cef
Compare
wongma7
changed the title
|
/retest |
1 similar comment
|
/retest |
|
/assign |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
/assign @thockin |
k8s-github-robot
added
size/L
pkg/api/types.go
Outdated
| @@ -455,6 +455,9 @@ type PersistentVolumeSpec struct { | |||
| // means that this volume does not belong to any StorageClass. | |||
| // +optional | |||
| StorageClassName string | |||
| // A comma-separated list of mount options. Not validated--mount will simply fail if one is invalid. | |||
| // +optional | |||
| MountOptions *string | |||
There was a problem hiding this comment.
Why is this a comma-sep string rather than a []string ? I dislike embedded syntax unless well-justified...
There was a problem hiding this comment.
From the discussion kubernetes/community#321 (comment), it seems people had no strong objections to a comma-separated string once we decided to give up per-option validation, so it made it into the final proposal. I guess that's not a very good justification.
The only other reason I can think of is: since StorageClass.parameters is restricted to map[string]string, there is some consistency in making persistentVolume.spec.mountOptions a string like storageClass.parameters[mountOptions] is.
There was a problem hiding this comment.
Since beta version was implemented using annotations and that by definition means key and value has to be string (unless we perform special decoding of annotation and since this is exposed to end user, it gets hairy fast) - that was the main reason we gave up on the idea of the mount option being []string . It is probably still not a very good justification. But I think that - using string here is fine as well.
|
ping @gnufied |
|
WRT paramaters, the assumption was that complex params would be encoded
into strings somehow. E.g. `[ "ro", "remount", "soft" ]`. I don't feel
SUPER strongly, but I worry about exposing this to users as a comma-string.
…On Tue, Aug 22, 2017 at 2:53 PM, Matthew Wong ***@***.***> wrote:
ping @gnufied <https://github.com/gnufied>
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#50919 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVHJrQLsPE6bfEsegjOlYUgAJ1bFTks5sa03ygaJpZM4O7ytt>
.
|
|
In this mount options case, if it has to be a parameter string, I think a comma-string is better for users than a stricter kind of encoding. Because comma-separated options is how Hate to take a step backwards, but if we introduce storageClass.mountOptions as a first-class field instead, then we don't need to worry about this as we can have both PV.spec.mountoptions & storageclass.mountoptions be string arrays. |
k8s-github-robot
added
the
needs-rebase
|
Lots of beta things have use complex string encoding in annotations to get
structure for would-be fields. That alone isn't a good justification.
…On Wed, Aug 23, 2017 at 6:37 AM, Hemant Kumar ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/api/types.go
<#50919 (comment)>
:
> @@ -455,6 +455,9 @@ type PersistentVolumeSpec struct {
// means that this volume does not belong to any StorageClass.
// +optional
StorageClassName string
+ // A comma-separated list of mount options. Not validated--mount will simply fail if one is invalid.
+ // +optional
+ MountOptions *string
Since beta version was implemented using annotations and that by
definition means key and value has to be string (unless we perform special
decoding of annotation and since this is exposed to end user, it gets hairy
fast) - that was the main reason we gave up on the idea of the mount option
being []string . It is probably still not a very good justification. But
I think that - using string here is fine as well.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#50919 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVFLcznaR2DYakFYc42lYnfh4V4Baks5sbCsPgaJpZM4O7ytt>
.
|
k8s-github-robot
removed
the
needs-rebase
|
/retest |
|
@thockin ping again, PV.spec.mountOptions is ready for final pass ! |
|
/lgtm Don't forget a docs PR, please. @devin-donnelly |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
added
needs-rebase
k8s-github-robot
removed
lgtm
|
needs re-lgtm please, I just rebased! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, jsafrane, thockin, wongma7 Associated issue: 168 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
|
/retest |
|
/retest |
|
It looks like federation tests are failing for everyone: https://k8s-gubernator.appspot.com/builds/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-federation-e2e-gce |
|
/retest Review the full test history for this PR. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 50919, 51410, 50099, 51300, 50296) |
Automatic merge from submit-queue Add storageClass.mountOptions and use it in all applicable plugins split off from #50919 and still dependent on it. cc @gnufied issue: kubernetes/enhancements#168 ```release-note Add mount options field to StorageClass. The options listed there are automatically added to PVs provisioned using the class. ```
Automatic merge from submit-queue Add storageClass.mountOptions and use it in all applicable plugins split off from kubernetes/kubernetes#50919 and still dependent on it. cc @gnufied issue: kubernetes/enhancements#168 ```release-note Add mount options field to StorageClass. The options listed there are automatically added to PVs provisioned using the class. ```
Automatic merge from submit-queue Add storageClass.mountOptions and use it in all applicable plugins split off from kubernetes/kubernetes#50919 and still dependent on it. cc @gnufied issue: kubernetes/enhancements#168 ```release-note Add mount options field to StorageClass. The options listed there are automatically added to PVs provisioned using the class. ```
What this PR does / why we need it: Implements kubernetes/community#771
issue: kubernetes/enhancements#168
Special notes for your reviewer:
TODO:
StorageClass mountOptionsAs described in proposal, this adds PV.spec.mountOptions + mountOptions parameter to every plugin that is both provisionable & supports mount options.
(personally, even having done all the work already, i don't agree w/ the proposal that mountOptions should be SC parameter but... :))
Release note: