----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49565/#review140520 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/mesos/isolators/gpu/volume.hpp (line 34) <https://reviews.apache.org/r/49565/#comment205938> It would be nice to have an nvidia namespace for all of this stuff and avoid the `mesos::internal::slave` namespaceing. This could be `nvidia::Volume`. Will leave for now. src/slave/containerizer/mesos/isolators/gpu/volume.hpp (line 37) <https://reviews.apache.org/r/49565/#comment205937> Include try.hpp? src/slave/containerizer/mesos/isolators/gpu/volume.hpp (line 45) <https://reviews.apache.org/r/49565/#comment205936> const? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 159 - 160) <https://reviews.apache.org/r/49565/#comment205939> "Failed to " to be consistent with the tense of the other error messages. Nice error messages in this patch! src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 184 - 192) <https://reviews.apache.org/r/49565/#comment205940> Seems like this belongs up in the .hpp interface documentation, the caller should be aware of this? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 209) <https://reviews.apache.org/r/49565/#comment205941> how about s/result/mkdir/ sytle naming here and below? note that this is what we generally do instead of 'result' src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 260) <https://reviews.apache.org/r/49565/#comment205942> s/:/: / src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 285) <https://reviews.apache.org/r/49565/#comment205943> We should probably use unique_ptr here to avoid the libprocess depedency (no need for an asynchronous library), but I'll leave for now since we generally need to polish the unique_ptr / Owned usage. src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 290) <https://reviews.apache.org/r/49565/#comment205944> Error context? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 296) <https://reviews.apache.org/r/49565/#comment205946> We could do Option<string> here and CHECK_SOME after the logic to ensure we're never passing through with the empty string case. src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 300 - 301) <https://reviews.apache.org/r/49565/#comment205945> quotes on the same line? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 306) <https://reviews.apache.org/r/49565/#comment205947> indentation is off here? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 308) <https://reviews.apache.org/r/49565/#comment205948> This compiles? I'd think you need a stringify here since you're doing +(char[], enum) src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 319 - 320) <https://reviews.apache.org/r/49565/#comment205949> quotes on the same line? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 325) <https://reviews.apache.org/r/49565/#comment205950> "libraries" typo src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 350) <https://reviews.apache.org/r/49565/#comment205951> double backtick? src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 31) <https://reviews.apache.org/r/49565/#comment205935> Alphabetical? Also, we can do a finer-grained os/exists.hpp include here. src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 477 - 481) <https://reviews.apache.org/r/49565/#comment205934> Looks like these can be EXPECTs? - Benjamin Mahler On July 2, 2016, 7:41 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49565/ > ----------------------------------------------------------- > > (Updated July 2, 2016, 7:41 p.m.) > > > Review request for mesos, Benjamin Mahler, Yubo Li, and Vikrama Ditya. > > > Bugs: MESOS-5401 > https://issues.apache.org/jira/browse/MESOS-5401 > > > Repository: mesos > > > Description > ------- > > This component is responsible for building up a consolidated volume of > binaries / libraries from the user-space portion of the Nvidia GPU > drivers so that it can be injected into a container as a single > volume. Its core functionality mimics that of the > `nvidia-docker-plugin`: https://github.com/NVIDIA/nvidia-docker/ > > We currently only implement the 'create()' function which is > responsible for building up the volume itself. In a subsequent commit > we will add support for reading a Docker ImageManifest and deciding if > we should inject the volume into the docker container or not. > > > Diffs > ----- > > src/CMakeLists.txt 09dd0e0dd2f25fb71db5051d5f5a797a981c2e59 > src/Makefile.am 52d63f26e0455ef31411e964e497a509d96aaf4a > src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp > 01cdab2f87d3ac823b8647c084c481593b6fa07f > src/slave/containerizer/mesos/isolators/gpu/volume.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/gpu/volume.cpp PRE-CREATION > src/tests/containerizer/nvidia_gpu_isolator_tests.cpp > 6770f05455f19d12b7984162f93a8b458951926f > > Diff: https://reviews.apache.org/r/49565/diff/ > > > Testing > ------- > > `GTEST_FILTER="*NVIDIA_GPU_Volume*" make -j check` > > > Thanks, > > Kevin Klues > >
