feat(lep): support a ReadOnlyMany behavior. #7853
Conversation
Signed-off-by: James Munson <james.munson@suse.com>
|
|
||
| ## Proposal | ||
|
|
||
| Add a parameter to storageclass and thus to volume spec named `mountReadOnly` with default value of `false`. |
There was a problem hiding this comment.
We should just leverage the access mode of a PVC to determine if the volume is ROM. The mount ro option should supersede the customized options provided by users.
There was a problem hiding this comment.
Agree with @innobead's suggestion.
If the PVC is ROM, we automatically append ro option.
| - Auto-remount feature (volumes that are read-only due to some mishap but should not be) needs a way to check the volume's intent. It would be cumbersome to make it parse the NFS mount options to figure that out. | ||
| - Since it requires using `nfsOptions`, the user is responsible for knowing all the other necessary mount options to add to "ro". | ||
|
|
||
| #### 2. New `ReadOnlyMany` access mode enum value: |
There was a problem hiding this comment.
As an official support, we should go with option 2 to provide a better user experience via a standard interface (access mode: ROM).
|
Good idea! Instead of "only" doing this on the |
There was a problem hiding this comment.
I agree with the comments so far: we should leverage the Kubernetes AccessMode = ReadOnlyMany (set in PVC and PV) instead of adding a new field to a StorageClass and Longhorn volume.
We should consider the possibility that we will be asked to create a volume with multiple capabilities / access modes simultaneously. For example:
- ReadOnlyMany, and
- ReadWriteMany.
Then, we may be asked to publish the volume with either of these access modes.
What should we do in this case?
I think we should probably create a Longhorn volume with AccessMode == rwx when responding to the CreateVolume request. Then, we should modify behavior appropriately in ControllerPublishVolume (if we want to enforce read-only on the export) and NodeStageVolume/NodePublishVolume (where we will modify mount options).
I am not sure this is strictly necessary, but it may be the "right" thing to do. It looks like we already template the Ganesha configuration, so it might not be too difficult to change the access type on the export if we can get the right information to the share manager pod (https://github.com/longhorn/longhorn-share-manager/blob/3168f43cad72c25cec704494064f3f58e28b7210/pkg/server/nfs/nfs_server.go#L148-L169). That may mean an additional API change though, since I don't think there's an existing ShareManager field to drop the read-only information into. |
|
As discussed with @james-munson, currently Kubernetes has 4 access modes in total (link):
We are missing |
This is a good point. I think we can take a look at how CSI sidecar is handling the selection when there are multiple modes specified. Some combinations should be blocked in CreateVolume: https://github.com/kubernetes-csi/csi-lib-utils/blob/f82f9de5b8aeb3c3b236d7f58fc5eeab34438078/accessmodes/access_modes.go#L36-L78 |
Agree with @ejweber that this might not be strictly necessary. When PVC requests ReadOnlyMany, it is not mandatory that we have to force the underlying storage layer to be read-only https://kubernetes.io/docs/concepts/storage/persistent-volumes/#:~:text=For%20example%2C%20even%20if%20a%20PersistentVolume%20is%20created%20as%20ReadOnlyMany%2C%20it%20is%20no%20guarantee%20that%20it%20will%20be%20read%2Donly |
|
Converting this to draft while I incorporate the feedback and renew with the recommended approach. I'll either re-open or close and replace with another PR. |
|
@james-munson This PR has been pending for a while. Let's resume this and try to get it for 1.7.0 still. |
|
A NFS based solution has highly available problems if we need a read-only rox solution could it decouple from NFS and use ISCSI instead since we don't need a write lock? |
@halfcrazy |
My use case it's to use a global rwx PVC to provide vmtool volume for multiple kubevirt pods on different nodes. If the share manage node shuts down unexpectedly, pods can't recover. We disable |
Thanks for the explanation. It's a real use case for the share-manager HA. cc @james-munson @innobead |
Which issue(s) this PR fixes:
Issue #5041
What this PR does / why we need it:
Make an LEP with a proposal for how to implement
ReadOnlyManybehavior in Longhorn code.Special notes for your reviewer:
Additional documentation or context