----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50127/#review153844 -----------------------------------------------------------
src/tests/containerizer/docker_containerizer_tests.cpp (line 3958) <https://reviews.apache.org/r/50127/#comment223354> Given the comment below (which we move up here), this comment is now unnecessary. src/tests/containerizer/docker_containerizer_tests.cpp (lines 3973 - 3977) <https://reviews.apache.org/r/50127/#comment223352> Can we move this up to the line just after: ``` EXPECT_EQ(TASK_RUNNING, statusRunning->state()); ``` Thta way it is clearer to see why we are about to do an `http::get()` on the master `state` endpoint. src/tests/containerizer/docker_containerizer_tests.cpp (lines 3982 - 3987) <https://reviews.apache.org/r/50127/#comment223356> Can we move this down to just before the `inspect` variable is used? Also, can we make this comment more explicit about exactly what is being checked below here: i.e. We use `docker inspect` to check that all injected nvidia devices are present inside the running container. This includes both the control devices (e.g. `/dev/nvidia-uvm`) as well as the GPU device itself (i.e. `/dev/nvidia0`). src/tests/containerizer/docker_containerizer_tests.cpp (lines 3988 - 3990) <https://reviews.apache.org/r/50127/#comment223355> Can we move this right above the usage for `minor`? src/tests/containerizer/docker_containerizer_tests.cpp (line 3992) <https://reviews.apache.org/r/50127/#comment223359> Can we rename this to `devicePaths`. We typically prefer longer variable names to abbreviated ones. Also, can we make this a `set<>` so we can use a `foreach()` below and just check for the existence of the device in this set instead of relying on some implicit ordering of the devices in this vector. src/tests/containerizer/docker_containerizer_tests.cpp (line 4002) <https://reviews.apache.org/r/50127/#comment223357> Can we incorprate this into the comment above? src/tests/containerizer/docker_containerizer_tests.cpp (line 4005) <https://reviews.apache.org/r/50127/#comment223358> Don't think this comment is necessary, given the big comment we will add above. src/tests/containerizer/docker_containerizer_tests.cpp (line 4006) <https://reviews.apache.org/r/50127/#comment223360> Given the comment above, we can change this to a `foreach()` over `inspect->devices` and just check for the existence of the device at the current loop iteration in the `devPaths` set. - Kevin Klues On Oct. 24, 2016, 5:08 a.m., Yubo Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50127/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2016, 5:08 a.m.) > > > Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull. > > > Bugs: MESOS-5795 > https://issues.apache.org/jira/browse/MESOS-5795 > > > Repository: mesos > > > Description > ------- > > This added a testing case for end-to-end GPU support for docker > containerizer. > > > Diffs > ----- > > src/tests/containerizer/docker_containerizer_tests.cpp > 73ae3906ff1efab1af11ba49bfe0c5a5d9d1d5a1 > > Diff: https://reviews.apache.org/r/50127/diff/ > > > Testing > ------- > > GTEST_FILTER="DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_Launch" make -j > check > > > Thanks, > > Yubo Li > >
