Add support for mount options to CSI drivers #67898
Conversation
k8s-ci-robot
added
release-note
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
| @@ -288,7 +288,10 @@ func (p *csiPlugin) ConstructVolumeSpec(volumeName, mountPath string) (*volume.S | |||
| func (p *csiPlugin) SupportsMountOption() bool { | |||
| // TODO (vladimirvivien) use CSI VolumeCapability.MountVolume.mount_flags | |||
| // to probe for the result for this method | |||
| return false | |||
| // (bswartz) Until the CSI spec supports probing, our only option is to | |||
There was a problem hiding this comment.
I'm looking at the CSI spec and I think that every driver should support mount_flags. Of course, a driver may return some error when an invalid flag is set (or even when any flag is set), but that's up to system admin to fix.
I think we don't need the comments around and we can return true without any regrets.
There was a problem hiding this comment.
Well one could imagine a CSI driver that doesn't support mount options yet, or for which mount options are irrelevant. In those cases, returning false here would allow kubernetes to report an actual useful error message instead of hiding any error messages down in some CSI driver log or silently ignoring mount options.
There was a problem hiding this comment.
The CSI spec says that the all the volume capabilities must be validated by the driver. So a properly implemented CSI driver should error on any invalid or unsupported volume capability.
The question I have is what happens in the future if a new volume capability is added? Will plugins have to explicitly upgrade their proto version to see the new field or will it show up as an optional pointer?
There was a problem hiding this comment.
I can't answer the question about future capabilities and how existing drivers can validate them, but I agree with your point about existing validation of mount options. A driver has the ability to reject requests with mount option now, if it doesn't support them. Of course drivers can also silently ignore them, but there's not much we can do about that.
There was a problem hiding this comment.
The CSI spec says that the all the volume capabilities must be validated by the driver. So a properly implemented CSI driver should error on any invalid or unsupported volume capability.
The question I have is what happens in the future if a new volume capability is added? Will plugins have to explicitly upgrade their proto version to see the new field or will it show up as an optional pointer?
Good question. AFAIK proto backwards compat means that it won't show up in the request. Should bring this up in the CSI community meeting.
| @@ -288,7 +288,10 @@ func (p *csiPlugin) ConstructVolumeSpec(volumeName, mountPath string) (*volume.S | |||
| func (p *csiPlugin) SupportsMountOption() bool { | |||
| // TODO (vladimirvivien) use CSI VolumeCapability.MountVolume.mount_flags | |||
| // to probe for the result for this method | |||
| return false | |||
| // (bswartz) Until the CSI spec supports probing, our only option is to | |||
There was a problem hiding this comment.
The CSI spec says that the all the volume capabilities must be validated by the driver. So a properly implemented CSI driver should error on any invalid or unsupported volume capability.
The question I have is what happens in the future if a new volume capability is added? Will plugins have to explicitly upgrade their proto version to see the new field or will it show up as an optional pointer?
| @@ -186,6 +186,7 @@ func (c *csiMountMgr) SetUpAt(dir string, fsGroup *int64) error { | |||
| attribs, | |||
| nodePublishSecrets, | |||
| fsType, | |||
| c.spec.PersistentVolume.Spec.MountOptions, | |||
bswartz
force-pushed
the
csi-mount-options
branch
from
2717514
to
85b2248
Compare
|
Rebased and added unit tests |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @jsafrane |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bswartz, jsafrane 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest |
Declare support for mount option in CSI drivers, and implement usage for the mountOptions field of the storage class when invoking CSI RPCs that accept mount options.
Fixes #67897