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 Thu, 06 Aug 2020 01:27:36 GMT

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




src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp
Lines 85 (patched)
<https://reviews.apache.org/r/72733/#comment310485>

    Does the value of this map really need to be an `Owned`, rather than just a raw `Info`?



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 115-117 (patched)
<https://reviews.apache.org/r/72733/#comment310493>

    This seems pretty simple, could we do it now?



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 132-134 (patched)
<https://reviews.apache.org/r/72733/#comment310494>

    Do we have any validation code which could catch this earlier? Ditto for the `static_provisioning`
check below.



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 146-151 (patched)
<https://reviews.apache.org/r/72733/#comment310495>

    Should we check `_volume.mode()` against the access mode contained within the `VolumeCapability`?
Maybe we should have some separate validation code for that?



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 179-184 (patched)
<https://reviews.apache.org/r/72733/#comment310500>

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



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 229 (patched)
<https://reviews.apache.org/r/72733/#comment310496>

    How about "Create the directory to store this container's CSI volume state." ?



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 247 (patched)
<https://reviews.apache.org/r/72733/#comment310497>

    I think you can eliminate the `stringify` here, since we have a templated function which
can take protobuf messages: https://github.com/apache/mesos/blob/877b5886804a17ea4aee4f251a43a3f5dbc84ca1/src/slave/state.hpp#L213
    
    Also, it looks like the checkpoint() function will create the target directory, so you
could eliminate the call to `os::mkdir(containerDir)` above.



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 287-300 (patched)
<https://reviews.apache.org/r/72733/#comment310498>

    Can we eliminate this if you use `collect()` above instead of `await()`?



src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp
Lines 316 (patched)
<https://reviews.apache.org/r/72733/#comment310499>

    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



src/slave/containerizer/mesos/isolators/volume/csi/state.hpp
Lines 20 (patched)
<https://reviews.apache.org/r/72733/#comment310486>

    Also need includes for `std::string` and `std::hash`.


- Greg Mann


On Aug. 4, 2020, 8:20 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72733/
> -----------------------------------------------------------
> 
> (Updated Aug. 4, 2020, 8:20 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
> -----
> 
>   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