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