-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSI: Include MountOptions in capabilities sent to CSI for all RPCs #10643
Conversation
Include the VolumeCapability.MountVolume data in ControllerPublishVolume, CreateVolume, and ValidateVolumeCapabilities RPCs sent to the CSI controller. The previous behavior was to only include the MountVolume capability in the NodeStageVolume request, which on some CSI implementations would be rejected since the Volume was not originally provisioned with the specific mount capabilities requested.
79634bd
to
554af9f
Compare
Fixes issue #10644 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm a little concerned about the user experience of this relative to the volume.mount_options
block. By having multiple places where this can be defined, we're exposing a lot of the CSI "guts" to end users. For the time being we can paper over that with some documentation... I'll open a separate PR with that. Once I've written up the docs and just walked through it end-to-end once myself, we should be good to merge this.
@@ -471,6 +482,7 @@ func (v *CSIVolume) controllerPublishVolume(req *structs.CSIVolumeClaimRequest, | |||
ClientCSINodeID: externalNodeID, | |||
AttachmentMode: req.AttachmentMode, | |||
AccessMode: req.AccessMode, | |||
MountOptions: csiVolumeMountOptions(vol.MountOptions), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a note for myself here: this confused the heck out of me, but it looks like we crossed some wires between client/structs
and nomad/structs
where we shouldn't have. I need to check what that looks like on the wire to see if we can fix it without breaking backwards compat.
Good morning, thanks for reviewing this so quickly! I agree with both if your points there. It is redundant to specify the mount options etc in both the volume creation and the job. I think you've already seen #10617 where this came up before for someone else. And "crossing the wires" w.r.t. to those two structs is a good way to put it. I think one was there to keep the public API interface simple, but it may have bubbled deeper into the code than it should. |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Include the VolumeCapability.MountVolume data in
ControllerPublishVolume, CreateVolume, and ValidateVolumeCapabilities
RPCs sent to the CSI controller. The previous behavior was to only
include the MountVolume capability in the NodeStageVolume request, which
on some CSI implementations would be rejected since the Volume was not
originally provisioned with the specific mount capabilities requested.