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 49615: Implemented 'shouldInject()' in the 'NvidiaVolume' component.
Date Tue, 05 Jul 2016 22:42:02 GMT

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 381 - 384)
<https://reviews.apache.org/r/49615/#comment206224>

    Why duplicate the comment from the header in the .cpp file?



src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 387 - 393)
<https://reviews.apache.org/r/49615/#comment206225>

    You don't need either of these checks



src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 395)
<https://reviews.apache.org/r/49615/#comment206226>

    why auto instead of `const Label&`?



src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 396)
<https://reviews.apache.org/r/49615/#comment206227>

    You don't need the has_key check here



src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 397 - 399)
<https://reviews.apache.org/r/49615/#comment206228>

    I'm a bit confused by the comment, what is it used for exactly? As discussed, let's say
that it is used as the volume name, which is unnecessary for us because we use the host path
directly.



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 488 - 512)
<https://reviews.apache.org/r/49615/#comment206231>

    Seems we also need a false case here?


- Benjamin Mahler


On July 4, 2016, 10:09 p.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49615/
> -----------------------------------------------------------
> 
> (Updated July 4, 2016, 10:09 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5401
>     https://issues.apache.org/jira/browse/MESOS-5401
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We use the the `com.nvidia.volumes.needed` label from
> nvidia-docker to decide if we should inject the volume or not:
> 
> https://github.com/NVIDIA/nvidia-docker/wiki/Image-inspection
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/gpu/volume.cpp 9aac3cc8622c21014de87576f84180d30ae3fadd

>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 474040c89d69b50c051122d873c11a102b33c538

> 
> Diff: https://reviews.apache.org/r/49615/diff/
> 
> 
> Testing
> -------
> 
> `GTEST_FILTER="" make -j check && GTEST_FILTER="*NVIDIA_GPU_VolumeShouldInject"
bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


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