> On Aug. 6, 2020, 9:27 a.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp > > Lines 85 (patched) > > <https://reviews.apache.org/r/72733/diff/1/?file=2236984#file2236984line85> > > > > Does the value of this map really need to be an `Owned`, rather than > > just a raw `Info`?
I think that's a convention in our isolator implementation, most of the isolators use `Owned` rather than a raw `Info`, like: https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp#L169 https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp#L117 https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/filesystem/linux.hpp#L99 https://github.com/apache/mesos/blob/1.10.0/src/slave/containerizer/mesos/isolators/network/cni/cni.hpp#L223 > On Aug. 6, 2020, 9: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? 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? > On Aug. 6, 2020, 9:27 a.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > > Lines 132-134 (patched) > > <https://reviews.apache.org/r/72733/diff/1/?file=2236985#file2236985line132> > > > > Do we have any validation code which could catch this earlier? Ditto > > for the `static_provisioning` check below. Good catch! I think I can do the validation earlier here: https://github.com/apache/mesos/blob/1.10.0/src/common/validation.cpp#L213 > On Aug. 6, 2020, 9: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? 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? > On Aug. 6, 2020, 9:27 a.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > > Lines 179-184 (patched) > > <https://reviews.apache.org/r/72733/diff/1/?file=2236985#file2236985line179> > > > > Can you help me understand why we do not require an absolute container > > path to already exist in this case, while we require it in other cases > > (both here and in the Linux filesystem isolator)? The reason that we do not require an absolute container path to already exist is that the container can be launched with any images, we cannot require all images have the specified absolute container path to already exist, and since the container is launched with an image (i.e. it has its own rootfs), we can just create the absolute container path in its own rootfs which will not affect the agent host rootfs. However if the container is launched without an image (i.e. it has no its own rootfs and just uses the agent host rootfs), we require the absolute container path to already exist in the agent host rootfs and do not want to create the path via `os::mkdir()` because we do not want to change the agent host rootfs. Could you please point me the code block of the Linux filesystem that you mentioned above? > On Aug. 6, 2020, 9:27 a.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > > Lines 287-300 (patched) > > <https://reviews.apache.org/r/72733/diff/1/?file=2236985#file2236985line287> > > > > Can we eliminate this if you use `collect()` above instead of `await()`? I think we still need to use `await()` so if any CSI publish volume call fails we can get the actual error message. > On Aug. 6, 2020, 9: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 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. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72733/#review221469 ----------------------------------------------------------- On Aug. 4, 2020, 4:20 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72733/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2020, 4:20 p.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 > ----- > > src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > 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/1/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
