mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 49565: Added a new 'NvidiaVolume' component.
Date Sat, 02 Jul 2016 23:39:58 GMT

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


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