----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50523/#review155041 -----------------------------------------------------------
src/slave/containerizer/docker.hpp (lines 198 - 200) <https://reviews.apache.org/r/50523/#comment224880> How about move this under #286 so as to let them share one `#ifdef __linux__` src/slave/containerizer/docker.hpp (lines 199 - 200) <https://reviews.apache.org/r/50523/#comment224881> indent is not right, should be 4 spaces src/slave/containerizer/docker.hpp (line 271) <https://reviews.apache.org/r/50523/#comment224867> Can you please add a comment here? ``` Allocate a set of GPU resources for a specified container. ``` src/slave/containerizer/docker.hpp (lines 273 - 274) <https://reviews.apache.org/r/50523/#comment224868> 1) Put `containerId` as first parameter. 2) 4 spaces ``` process::Future<Nothing> allocateNvidiaGpus( const ContainerID& containerId, const std::set<Gpu>& gpus ); ``` src/slave/containerizer/docker.cpp (lines 686 - 687) <https://reviews.apache.org/r/50523/#comment224869> ``` return Failure("Attempted to allocate GPUs" " without Nvidia libraries available"); ``` src/slave/containerizer/docker.cpp (line 701) <https://reviews.apache.org/r/50523/#comment224882> add a new lines here src/slave/containerizer/docker.cpp (lines 1054 - 1060) <https://reviews.apache.org/r/50523/#comment224871> The `recoverDevices` will block here, what about using continuation as https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L1373-L1390 ? src/slave/containerizer/docker.cpp (line 1125) <https://reviews.apache.org/r/50523/#comment224883> How about move this right under #793 so as to let all the gpu related logic share one `#ifdef __linux__`? src/slave/containerizer/docker.cpp (lines 1131 - 1182) <https://reviews.apache.org/r/50523/#comment224872> indent is not right here, 2 spaces is good enough. src/slave/containerizer/docker.cpp (lines 1137 - 1139) <https://reviews.apache.org/r/50523/#comment224873> It is not good to use short name such as `NV` here, full words is better. ``` // Look for nvidia device in the vector of devices. Get // the GPU device numbers from the vector of devices used // by the container. ``` src/slave/containerizer/docker.cpp (line 1143) <https://reviews.apache.org/r/50523/#comment224874> s/Docker::Device &device/Docker::Device& device src/slave/containerizer/docker.cpp (line 1144) <https://reviews.apache.org/r/50523/#comment224875> s/string/const string src/slave/containerizer/docker.cpp (line 1158) <https://reviews.apache.org/r/50523/#comment224876> How about s/leftOverString/minorString ? src/slave/containerizer/docker.cpp (line 1159) <https://reviews.apache.org/r/50523/#comment224879> Seems most if the code is using `strings::remove()`, why not use it? src/slave/containerizer/docker.cpp (line 1162) <https://reviews.apache.org/r/50523/#comment224886> Here should be `unsigned int` src/slave/containerizer/docker.cpp (line 1164) <https://reviews.apache.org/r/50523/#comment224877> It is better put the error message to the failure message. src/slave/containerizer/docker.cpp (lines 1170 - 1171) <https://reviews.apache.org/r/50523/#comment224885> new line here src/tests/containerizer/docker_containerizer_tests.cpp (lines 4257 - 4259) <https://reviews.apache.org/r/50523/#comment224878> kill this I think that you can put those two tests with one `#ifdef __linux__`. src/tests/containerizer/docker_containerizer_tests.cpp (line 4260) <https://reviews.apache.org/r/50523/#comment224884> s/checks/verifies - Guangya Liu On 十一月 4, 2016, 9:22 a.m., Rajat Phull wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50523/ > ----------------------------------------------------------- > > (Updated 十一月 4, 2016, 9:22 a.m.) > > > Review request for mesos, Benjamin Mahler, Kevin Klues, Yubo Li, and Vikrama > Ditya. > > > Bugs: MESOS-5795 > https://issues.apache.org/jira/browse/MESOS-5795 > > > Repository: mesos > > > Description > ------- > > Updated docker recovery to account for GPU resources. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp 006f929eca0e0a6b1de941821ac72869ba393d2d > src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c > src/tests/containerizer/docker_containerizer_tests.cpp > c478d56ffd734b26fd2dbd7bb1ca02ce929f3f16 > > Diff: https://reviews.apache.org/r/50523/diff/ > > > Testing > ------- > > GTEST_FILTER="DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_LaunchWithGpuRecovery" > make -j check > > > Thanks, > > Rajat Phull > >
