mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yubo Li <liyub...@cn.ibm.com>
Subject Re: Review Request 50841: Added GPU scheduling logic to docker containerizer process.
Date Mon, 19 Sep 2016 06:22:54 GMT


> On 八月 25, 2016, 12:32 a.m., Kevin Klues wrote:
> > src/slave/containerizer/docker.cpp, lines 666-668
> > <https://reviews.apache.org/r/50841/diff/6/?file=1480595#file1480595line666>
> >
> >     I would do this as the first check in this function.  If we don't have an allocator
set, then we really shouldn't even be calling this function regardless of anything else that
is going on.
> >     
> >     Also, the string should read:
> >     ```
> >     "The `allocateNvidiaGpu` function was called without an `NvidiaGpuAllocator`
set".
> >     ```
> 
> Yubo Li wrote:
>     If we put `nvidiaGpuAllocator` check in top of this function, we have to check `requested==0`
outside the function otherwise `nvidiaGpuAllocator` check will be failed if GPU feature is
not enabled. But I think move `requested==0` outside `nvidiaGpuAllocator` is reasonable if
we use temp `Future()`. That is something logic like
>     ```
>     Future<Nothing> allocateGpus = Nothing();
>     ......
>     if (gpus.isSome()) {
>       // Make sure that the `gpus` resource is not fractional.
>       // We rely on scala resources only have 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");
>       }
>     
>       const size_t requested = static_cast<size_t>(gpus.getOrElse(0.0));
>     
>       if (requested > 0) {
>         allocateGpus = allocateNvidiaGpus(requested, containerId);
>       }
>     }
>     ```
>     Make sense?
> 
> Kevin Klues wrote:
>     Yeah, that looks good to me. Except now you don't need `getOrElse()`, just `get()`.

Sure, I changed to `get()`.


> On 八月 25, 2016, 12:32 a.m., Kevin Klues wrote:
> > src/slave/containerizer/docker.cpp, lines 694-696
> > <https://reviews.apache.org/r/50841/diff/6/?file=1480595#file1480595line694>
> >
> >     Why do you need this level of indirection? Why not just pass `containers_[containerId]->gpuAllocated`
directly to `nvidiaGpuAllocator->deallocate()`?
> 
> Yubo Li wrote:
>     `containers_[containerId]->gpuAllocated` is a list but `nvidiaGpuAllocator->deallocate()`
accepts set.
> 
> Kevin Klues wrote:
>     Can we  make `containers_[containerId]->gpuAllocated` a `set`?

Yes, changed it to `set`.


- Yubo


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


On 八月 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 八月 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message