----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50599/#review153839 -----------------------------------------------------------
src/slave/containerizer/docker.hpp (line 509) <https://reviews.apache.org/r/50599/#comment223325> I dont think we need an Option here. We can just use a vector alone, and then check if there are any devices in it with a `.empty()` instead of a `.isSome()`. src/slave/containerizer/docker.cpp (line 691) <https://reviews.apache.org/r/50599/#comment223322> This is OK since we do this the same in other functions for this class, but we should really wrap pointers like this in Owned<> or Shared<> depending on their semantics. src/slave/containerizer/docker.cpp (line 694) <https://reviews.apache.org/r/50599/#comment223326> This is unnecessary, given the comment above. Doing this here would have been problematic anyway, because it's possible that future code would want to add things to this vector outside of this function. src/slave/containerizer/docker.cpp (lines 696 - 698) <https://reviews.apache.org/r/50599/#comment223329> We should reword this to: ``` // TODO(Yubo): Nvidia devices are hard-coded here. We should move these // constants to a common file shared by both the mesos containerizer and // the docker containerizer. ``` src/slave/containerizer/docker.cpp (line 700) <https://reviews.apache.org/r/50599/#comment223328> I would reword this to explicitly mention that tasks launched with the docker containerizer require these permissions in order to operate. src/slave/containerizer/docker.cpp (lines 701 - 726) <https://reviews.apache.org/r/50599/#comment223340> It might be better to just set the Device fields explicity. Otherwise, building a string and passing it to the parse function should be OK, but we need to use the new version of the parse function, which *only* takes a string, rather than the constructor like one used here. src/slave/containerizer/docker.cpp (line 705) <https://reviews.apache.org/r/50599/#comment223341> This should be "rmw", not "mrw" src/slave/containerizer/docker.cpp (line 722) <https://reviews.apache.org/r/50599/#comment223332> Can we rename this variable: `devicePath`. src/slave/containerizer/docker.cpp (line 723) <https://reviews.apache.org/r/50599/#comment223331> Can you just use `stringify()` here? Also, can you use `Path().join()` instead of just `+`. - Kevin Klues On Oct. 24, 2016, 5 a.m., Yubo Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50599/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2016, 5 a.m.) > > > Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and > Rajat Phull. > > > Bugs: MESOS-5795 > https://issues.apache.org/jira/browse/MESOS-5795 > > > Repository: mesos > > > Description > ------- > > Assigned Nvidia GPU devices to docker container based on > GPUs allocated by Nvidia GPU allocator. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d > src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 > > Diff: https://reviews.apache.org/r/50599/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Yubo Li > >
