> 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 > >
