----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50841/#review147089 -----------------------------------------------------------
src/slave/containerizer/docker.hpp (line 506) <https://reviews.apache.org/r/50841/#comment214146> Shoud we use `set` here like we do in the mesos containerizer? src/slave/containerizer/docker.cpp (line 663) <https://reviews.apache.org/r/50841/#comment214148> We don't need this line if you follow the suggestion below. src/slave/containerizer/docker.cpp (lines 665 - 672) <https://reviews.apache.org/r/50841/#comment214149> We can't assume that `container` is still valid at the time we call this lambda. Because of this, we need to do the same check as above inside the lambda: ``` if (!containers_.contains(containerId)) { return Failure("Container is already destroyed"); } ``` However, if this fails, we need to make sure and deallocate the GPUs because the container we were trying to attach them to is now gone. I've run into this situation in the past, and @bmahler has usually had me break things like this out into separate function to avoid confusion with doing so many checks all over the place. Something like the following should do it: ``` Future<Nothing> DockerContainerizerProcess::allocateNvidiaGpus( const size_t count, const ContainerID& containerId) { if (!nvidiaGpuAllocator.isSome()) { return Failure("The 'allocateNvidiaGpus' function was called" " without an 'NvidiaGpuAllocator' set"); } if (!containers_.contains(containerId)) { return Failure("Container is already destroyed"); } return nvidiaGpuAllocator->allocate(count) .then(defer( self(), DockerContainerizerProcess::_allocateNvidiaGpus, lambda::_1, containerId); } Future<Nothing> DockerContainerizerProcess::_allocateNvidiaGpus( const set<Gpu>& allocated, const ContainerID& containerId) { if (!containers_.contains(containerId)) { return nvidiaGpuAllocator->deallocate(allocated) .onFailed([](const string& message) { return Failure("Failed to deallocate GPUs allocated" " to destroyed container: " + message); }); } containers_[containerId]->gpus = allocated; return Nothing(); } ``` src/slave/containerizer/docker.cpp (lines 676 - 695) <https://reviews.apache.org/r/50841/#comment214156> We need something similar here to what I suggested above for verifying that the container is still valid across the continuation. src/slave/containerizer/docker.cpp (line 1337) <https://reviews.apache.org/r/50841/#comment214154> You can just use `gpus.get()` here. src/slave/containerizer/docker.cpp (lines 1341 - 1346) <https://reviews.apache.org/r/50841/#comment214153> You can just use `if (gpus.get() > 0)` here. src/slave/containerizer/docker.cpp (lines 1541 - 1552) <https://reviews.apache.org/r/50841/#comment214157> You can't rely on `containers_[containerId]` to be valid at this point (hence the check for it below). Regardless, I just at the head of master to see where the most appropriate place to do this deallocation would be, and this function looks *completely* different from what I see here. When was the last time you rebased? src/slave/containerizer/docker.cpp (lines 2037 - 2041) <https://reviews.apache.org/r/50841/#comment214158> It feels too early to be doing the `deallocate()` here. I think we should only do it once the container is actually killed (i.e. in `___destroy()`). If we do it too early, the GPUs could still be being used by the container when they get allocated out to someone else. This would not go very well. There is the case, however, in `__destroy()` where we aren't able to kill the docker container at all! It's better to leak the GPUs in this case than to deallocate them and run the risk of giving them to a new container. We should mention this in the error string below if any GPUs are actually leaked: ``` container->termination.fail( "Failed to kill the Docker container: " + (kill.isFailed() ? kill.failure() : "discarded future")); ``` - Kevin Klues On Aug. 26, 2016, 11:02 a.m., Yubo Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50841/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 11:02 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 > ------- > > Added control logic to allocate/deallocate GPUs to GPU-related task > when the task is started/terminated. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d > src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 > > Diff: https://reviews.apache.org/r/50841/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Yubo Li > >
