Re: Review Request 48371: Added `NvidiaGpuAllocator` component.

2016-06-24 Thread Benjamin Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48371/#review139416
---


Fix it, then Ship it!




Great, thanks! I did some re-working of the patch to make the allocator a 
shared object and to make the implementation of the allocator process more 
succinct. Let me know if you see any issues.


src/slave/containerizer/mesos/isolators/gpu/allocator.hpp (lines 86 - 111)


It seems a bit misleading that the allocation / deallocation methods are 
const. To avoid the need for making them const, returning a pointer and using 
process::Shared in the caller, we can make this a shared object, like 
process::Queue, process::http::Connection, process::Future, etc.



src/slave/containerizer/mesos/isolators/gpu/allocator.hpp (line 100)


Hm.. None seems equivalent to Some empty set? Why distinguish these two 
cases?



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 200 - 208)


We should include the context in the error messages (e.g. failed to get the 
handle or get the minor number).



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 402 - 403)


Thanks for the test!



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 417 - 419)


Should we be checking that the gpu allocator agrees with this total?

```
  ASSERT_EQ(total.get(), allocator->total().size());

```



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 442 - 474)


No need for all of these result variables, we can `AWAIT_READY(f())` 
directly.


- Benjamin Mahler


On June 23, 2016, 4:33 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48371/
> ---
> 
> (Updated June 23, 2016, 4:33 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5557
> https://issues.apache.org/jira/browse/MESOS-5557
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This component is designed to serve as a centralized component for
> allocating / deallocating Nvidia GPUs. The goal is to share an
> instance of this across both the mesos containerizer and the docker
> containerizer so they can allocate GPUs from the same shared pool.
> 
> It is overloaded to also do resource enumeration of GPUs based on
> the agent flags. This keeps all code for enumerating GPUs and the
> resources they represent in a single centralized location.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 23cac95a805e46d216e9479fea09f2c1619c45a5 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp PRE-CREATION 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> a01c88660bb3c5435802ebd1cbc7ef84323c5795 
> 
> Diff: https://reviews.apache.org/r/48371/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 48371: Added `NvidiaGpuAllocator` component.

2016-06-23 Thread Kevin Klues 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48371/
---

(Updated June 23, 2016, 4:33 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased after changes pushed to master already.


Bugs: MESOS-5557
https://issues.apache.org/jira/browse/MESOS-5557


Repository: mesos


Description
---

This component is designed to serve as a centralized component for
allocating / deallocating Nvidia GPUs. The goal is to share an
instance of this across both the mesos containerizer and the docker
containerizer so they can allocate GPUs from the same shared pool.

It is overloaded to also do resource enumeration of GPUs based on
the agent flags. This keeps all code for enumerating GPUs and the
resources they represent in a single centralized location.


Diffs (updated)
-

  src/Makefile.am 23cac95a805e46d216e9479fea09f2c1619c45a5 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp PRE-CREATION 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
a01c88660bb3c5435802ebd1cbc7ef84323c5795 

Diff: https://reviews.apache.org/r/48371/diff/


Testing
---

GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 48371: Added `NvidiaGpuAllocator` component.

2016-06-10 Thread Kevin Klues 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48371/
---

(Updated June 11, 2016, 3:04 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased for https://reviews.apache.org/r/48578


Bugs: MESOS-5557
https://issues.apache.org/jira/browse/MESOS-5557


Repository: mesos


Description
---

This component is designed to serve as a centralized component for
allocating / deallocating Nvidia GPUs. The goal is to share an
instance of this across both the mesos containerizer and the docker
containerizer so they can allocate GPUs from the same shared pool.

It is overloaded to also do resource enumeration of GPUs based on
the agent flags. This keeps all code for enumerating GPUs and the
resources they represent in a single centralized location.


Diffs (updated)
-

  src/Makefile.am b656702d918e747cbd4b3d8f2c4257f61c83b385 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp PRE-CREATION 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
65ffbafffeb5dac372b5770d2a1560a942a822e2 

Diff: https://reviews.apache.org/r/48371/diff/


Testing
---

GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 48371: Added `NvidiaGpuAllocator` component.

2016-06-07 Thread Qian Zhang 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48371/#review136575
---




src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (line 84)


Need a newline before this line.


- Qian Zhang


On June 8, 2016, 6:46 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48371/
> ---
> 
> (Updated June 8, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5557
> https://issues.apache.org/jira/browse/MESOS-5557
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This component is designed to serve as a centralized component for
> allocating / deallocating Nvidia GPUs. The goal is to share an
> instance of this across both the mesos containerizer and the docker
> containerizer so they can allocate GPUs from the same shared pool.
> 
> It is overloaded to also do resource enumeration of GPUs based on
> the agent flags. This keeps all code for enumerating GPUs and the
> resources they represent in a single centralized location.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 29525c960e8fb2448260efdd774fd8fc1d68047b 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp PRE-CREATION 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> b1e64b2fc5518463cd14e119f89e3cd298286052 
> 
> Diff: https://reviews.apache.org/r/48371/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>