mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 72733: Implemented the `prepare` method of `volume/csi` isolator.
Date Thu, 06 Aug 2020 09:16:33 GMT


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


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