----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48373/#review139714 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp (lines 113 - 119) <https://reviews.apache.org/r/48373/#comment205042> Don't forget to include error context: ``` // Create the allocator. // // TODO(klueska): We need to share the allocator // with the docker containerizer. Try<NvidiaGpuAllocator*> create = NvidiaGpuAllocator::create(flags); if (create.isError()) { return Error("Failed to NvidiaGpuAllocator::create: " + _allocator.error()); } ``` src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp (line 218) <https://reviews.apache.org/r/48373/#comment205050> Hm.. wonder why we didn't just use foreach here? src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp (line 325) <https://reviews.apache.org/r/48373/#comment205051> Hm.. seems we should do another lookup of the 'info' since we are returning to the isolator context and things may have occurred in the interim. Perhaps it's worth pulling out an `_update` continuation to force this (no captures possible). src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp (lines 392 - 396) <https://reviews.apache.org/r/48373/#comment205052> Unfortunately this won't accomplish your intent of providing context in the failure message because the .onFailed method does not provide composition, your returned Failure here will fall into the void. To accomplish this, we need a .then which can accept a Future<T> in addition to being able to take a T. src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp (line 397) <https://reviews.apache.org/r/48373/#comment205054> This needs to defer to self since the infos map is managed by the isolator actor. In general, capturing and using 'this' within a continuation without defering to self is dangerous. src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp (lines 398 - 401) <https://reviews.apache.org/r/48373/#comment205053> To guard against races, how about: ``` CHECK(infos.contains(containerId)); delete infos.at(containerId); infos.erase(containerId); return Nothing(); ``` - Benjamin Mahler On June 23, 2016, 4:34 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48373/ > ----------------------------------------------------------- > > (Updated June 23, 2016, 4:34 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-5559 > https://issues.apache.org/jira/browse/MESOS-5559 > > > Repository: mesos > > > Description > ------- > > All logic to do GPU allocation is now conducted asynchronously through > the `NvidiaGpuAllocator` component. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp > 28ad0b3d1d8e7642a93e4ebb0fcc399e182be393 > src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp > cfb23244d0e6f6e0d94685a35826b35f3297c6fe > > Diff: https://reviews.apache.org/r/48373/diff/ > > > Testing > ------- > > GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests > > > Thanks, > > Kevin Klues > >
