mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 72733: Implemented the `prepare` method of `volume/csi` isolator.
Date Tue, 11 Aug 2020 04:43:00 GMT


> On Aug. 6, 2020, 1:27 a.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
> > Lines 115-117 (patched)
> > <https://reviews.apache.org/r/72733/diff/1/?file=2236985#file2236985line115>
> >
> >     This seems pretty simple, could we do it now?
> 
> Qian Zhang wrote:
>     Actually we have this issue in the other two isolators:
>     https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/volume/image.cpp#L118:L123
>     https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp#L364:L367
>     
>     I think I can create a separate ticket for this issue and refactor all of them later,
HDYT?
> 
> Greg Mann wrote:
>     Personally I think it's OK to write this isolator the "correct" way now, we can always
update the others later. I don't feel a need to keep our isolator implementations super consistent
across isolators.
> 
> Qian Zhang wrote:
>     Sure, let me do it for this isolator now.

Thanks!!


> On Aug. 6, 2020, 1:27 a.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
> > Lines 146-151 (patched)
> > <https://reviews.apache.org/r/72733/diff/1/?file=2236985#file2236985line146>
> >
> >     Should we check `_volume.mode()` against the access mode contained within the
`VolumeCapability`? Maybe we should have some separate validation code for that?
> 
> Qian Zhang wrote:
>     Yeah, I thought this before, but I am not quite sure if we should do that. Actually
in CSI spec, there is no any explanation about the interaction between the `NodePublishVolumeRequest.readonly`
and the access mode in the `VolumeCapability`, like what if `NodePublishVolumeRequest.readonly
== false` but the access mode in the `VolumeCapability` is `SINGLE_NODE_READER_ONLY`? Should
we return a failure in this case?
> 
> Greg Mann wrote:
>     I'm not so worried about the `readonly` field, I don't think that we need to validate
it against the volume capabilities because mounting a volume as read-only isn't "incompatible"
with any of the capabilities (for example, I don't think there's anything wrong with mounting
a volume readonly if the volume has only the SINGLE_NODE_WRITER capability, since mounting
as readonly does not violate RW permissions).
>     
>     However, I do think we should perform validation for our internal `Volume::Mode`
field. If a task attempts to include a CSI volume which has only the SINGLE_NODE_READER_ONLY
capability and the volume mode is set as RW, that does not seem valid to me. WDYT?
> 
> Qian Zhang wrote:
>     > I don't think there's anything wrong with mounting a volume readonly if the
volume has only the SINGLE_NODE_WRITER capability, since mounting as readonly does not violate
RW permissions
>     
>     But what about the reverse? I mean `NodePublishVolumeRequest.readonly == false` and
the volume has only the `SINGLE_NODE_READER_ONLY` capability. Do you think it is wrong?
>     
>     > However, I do think we should perform validation for our internal Volume::Mode
field. If a task attempts to include a CSI volume which has only the SINGLE_NODE_READER_ONLY
capability and the volume mode is set as RW, that does not seem valid to me. WDYT?
>     
>     Yes, I agree. I think I should add more validation here like: if the CSI volume has
only the `SINGLE_NODE_READER_ONLY` or `MULTI_NODE_READER_ONLY` capability and our internal
`Volume::Mode` field it set as `RW`, then we should return a failure. WDYT?

> But what about the reverse? I mean NodePublishVolumeRequest.readonly == false and the
volume has only the SINGLE_NODE_READER_ONLY capability. Do you think it is wrong?

The 'readonly' field, when true, requires the volume to be mounted readonly. When 'readonly'
is false, it doesn't require anything, and the volume can still be mounted readonly. So, I
don't think the case you mentioned is wrong.

> I think I should add more validation here like: if the CSI volume has only the SINGLE_NODE_READER_ONLY
or MULTI_NODE_READER_ONLY capability and our internal Volume::Mode field it set as RW, then
we should return a failure. WDYT?

Yep, sounds good!


> On Aug. 6, 2020, 1:27 a.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
> > Lines 316 (patched)
> > <https://reviews.apache.org/r/72733/diff/1/?file=2236985#file2236985line316>
> >
> >     Why do we do a recursive bind mount here, but not for volumes in the Linux filesystem
isolator? https://github.com/apache/mesos/blob/c78dc333fc893a43d40dc33299a61987198a6ea9/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp#L1067-L1068
> 
> Qian Zhang wrote:
>     The reason that we do a recursive bind mount here is, if there is any submounts under
the `source`, we want all those submounts also bind mounted at the `target` so that the container
can access them. We have this logic in all the volume isolators, like:
>     https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/volume/host_path.cpp#L351
>     https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/volume/image.cpp#L255
>     
>     The code that you pointed out in the Linux filesystem isolator is to mount the PV,
since PV is originally created by Mesos and we are sure there is no submounts under it, so
we do not need `MS_REC` in this case.
> 
> Greg Mann wrote:
>     Why do we know that a persistent volume will not have any mounts under it, but we
don't know this for CSI volumes? The PV is originally created by Mesos, but applications can
do whatever they want in the volume after it's created.
> 
> Qian Zhang wrote:
>     A CSI volume can be a pre-provisioned volume so it can already have some submounts
under it before users try to use it via Mesos. But you are right, applicaiton can also create
submounts under a PV. Maybe we should fix it for PV in a separate ticket? WDYT?

Yea I think you're right, we want to do a recursive mount in both cases. Let's fix the PV
case in another patch.


- Greg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72733/#review221469
-----------------------------------------------------------


On Aug. 10, 2020, 2:37 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72733/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2020, 2:37 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10153
>     https://issues.apache.org/jira/browse/MESOS-10153
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented the `prepare` method of `volume/csi` isolator.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 0f91d88abba4bd49b46676e956211f1cf4e7219b 
>   include/mesos/v1/mesos.proto f25db8aeefd721653b2027282abbae8f2e6a40f3 
>   src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/common/validation.cpp 14a8c7b9d70c303b527e5e43012999471ced2d78 
>   src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/csi/state.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/csi/state.proto PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72733/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message