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