----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50123/#review143639 -----------------------------------------------------------
Some early comments. src/slave/containerizer/docker.hpp (line 500) <https://reviews.apache.org/r/50123/#comment209467> We generally don't rely on transitive includes, because it's easier to maintain (quickly scan a file and add includes for all symbols present). It is better adding `#include<list>` in this header file. BTW: I saw that this header file is also missing some other include files after a scan: ``` #include<list> #include <map> #include <string> ``` It would be great if you can do a clean up here as well. src/slave/containerizer/docker.cpp (line 68) <https://reviews.apache.org/r/50123/#comment209468> 1) move this right before line 66 2) include set in the header. ``` #include <set> ``` src/slave/containerizer/docker.cpp (lines 170 - 185) <https://reviews.apache.org/r/50123/#comment209469> The `else` is not needed here. ``` if (nvidia.isSome()) { return new DockerContainerizer( flags, fetcher, Owned<ContainerLogger>(logger.get()), docker, nvidia.get().allocator); } return new DockerContainerizer( flags, fetcher, Owned<ContainerLogger>(logger.get()), docker, None()); ``` Also the right format of `if-else` should be: ``` if(xxxx) { xxxx; } else { xxxx; } ``` No need to add new line for `else {`. src/slave/containerizer/docker.cpp (line 1307) <https://reviews.apache.org/r/50123/#comment209495> I prefer that moving this before the use of this variable, i.e. putting it right before 1326 src/slave/containerizer/docker.cpp (line 1313) <https://reviews.apache.org/r/50123/#comment209497> s/.get()/-> src/slave/containerizer/docker.cpp (lines 1313 - 1318) <https://reviews.apache.org/r/50123/#comment209498> What about the following format plus some comments here: ``` const Resources& resources = taskInfo->resources(); Option<double> gpus = resources.gpus(); // Make sure that the `gpus` resource is not fractional. // We rely on scalar resources only having 3 digits of precision. if (static_cast<long long>(gpus.getOrElse(0.0) * 1000.0) % 1000 != 0) { return Failure("The 'gpus' resource must be an unsigned integer"); } size_t requested = static_cast<size_t>(resources.gpus().getOrElse(0.0)); ``` src/slave/containerizer/docker.cpp (lines 1325 - 1358) <https://reviews.apache.org/r/50123/#comment209500> How about simplify the logic as this: ``` Option<string> nvidiaGpus = None(); if (requested > 0) { } ``` src/slave/containerizer/docker.cpp (line 1333) <https://reviews.apache.org/r/50123/#comment209509> 1) s/allocator.get().allocate(requested)/allocator->allocate(requested) 2) I think it is not right to use `Option` here as the `allocator->allocate(requested)` is returning a `Future`. One proposal is as following: ``` allocator->allocate(requested) .then(defer(...)); ``` You can also refer to https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/gpu/isolator.cpp#L356 to get some detail. src/slave/containerizer/docker.cpp (lines 1335 - 1336) <https://reviews.apache.org/r/50123/#comment209504> ``` return Failure("Not enough GPUs available to reserve " + stringify(requested) + " GPUs"); ``` src/slave/containerizer/docker.cpp (lines 1337 - 1338) <https://reviews.apache.org/r/50123/#comment209506> new line here src/slave/containerizer/docker.cpp (lines 1341 - 1344) <https://reviews.apache.org/r/50123/#comment209510> s/string/const string src/slave/containerizer/docker.cpp (lines 1344 - 1345) <https://reviews.apache.org/r/50123/#comment209512> new line here and move this right before `argv.push_back` src/slave/containerizer/docker.cpp (line 1352) <https://reviews.apache.org/r/50123/#comment209513> s/string/const string src/slave/containerizer/docker.cpp (lines 1353 - 1354) <https://reviews.apache.org/r/50123/#comment209514> new line here src/slave/containerizer/docker.cpp (lines 1356 - 1357) <https://reviews.apache.org/r/50123/#comment209515> new line here src/slave/containerizer/docker.cpp (lines 1550 - 1561) <https://reviews.apache.org/r/50123/#comment209503> What about update the format as following: ``` // Release GPUs after the task exit. set<Gpu> deallocated; if (!containers_[containerId]->gpuAllocated.empty()) { foreach (const Gpu& &pu, containers_[containerId]->gpuAllocated) { deallocated.insert(gpu); } containers_[containerId]->gpuAllocated.clear(); if (!allocator.isSome()) { return Failure("Can not deallocate GPU without enabling GPU support."); } allocator->deallocate(deallocated); } ``` src/slave/containerizer/docker.cpp (line 1553) <https://reviews.apache.org/r/50123/#comment209501> 1) s/Gpu &gpu/const Gpu& gpu 2) we always put the `&` together with the definition src/slave/containerizer/docker.cpp (line 1560) <https://reviews.apache.org/r/50123/#comment209502> how about ``` allocator->deallocate(deallocated); ``` Why do you need a `.get()` at last? src/slave/containerizer/docker.cpp (lines 1917 - 1926) <https://reviews.apache.org/r/50123/#comment209507> ditto as above for the format - Guangya Liu On 七月 18, 2016, 9:17 a.m., Yubo Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50123/ > ----------------------------------------------------------- > > (Updated 七月 18, 2016, 9:17 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 'NvidiaGpuAllocator' to docker containerizer so that the > docker containerizer can use it to allocate GPUs to the task with 'gpus' > resource. Also, allocated GPUs will automatically deallocated after the > job destroyed. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp 43ca4317d608b3b43dd7bd0d1b55c721e7364885 > src/slave/containerizer/docker.cpp f1ecf3b25d85597f6c3dcaa47968860ed119dbd5 > > Diff: https://reviews.apache.org/r/50123/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Yubo Li > >
