mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.
Date Sat, 04 Feb 2017 03:02:18 GMT


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> >

Thanks!


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > include/mesos/allocator/allocator.hpp, lines 89-92
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615402#file1615402line89>
> >
> >     What is the reason for changing this interface? We should think hard before
making it too specific to requirements from single features such as `MULTI_ROLE`.
> >     
> >     Propagating internal assumptions such as "only allocations towards the same
role in one `Resources`" across module boundaries really limits room for future developments.
In this case both our allocator and the master rely on above assumption; it should be possible
for them to adjust passed resources on interfaces, e.g., by creating datastructure like you
proposed here internally themself (they could use the same helper function). That way we could
avoid tech debt spilling into interfaces.
> 
> Michael Park wrote:
>     I can sympathize with this position, although it doesn't seem terrible to me that
>     the notion of multi-role which is being introduced to Mesos (rather than a specific
allocator)
>     changes the allocator API.
>     
>     But it doesn't seem obvious to me why it should be `hashmap<string, hashmap<SlaveID,
Resources>>`,
>     as opposed to `hashmap<SlaveID, hashmap<string, Resources>>`. I tried
to look through the code
>     briefly to see if it happens to make the code a lot simpler or something, but I don't
really see one..
>     
>     It seems to me like the pattern I see in a few places could be used here without
changing the API also.
>     
>     ```cpp
>     const hashmap<SlaveID, Resources>& resources;
>     
>     foreachpair (const SlaveID& slaveId, const Resources& resources) {
>       foreachpair (const string& role,
>                    const Resources& resources,
>                    resources.allocation()) {
>         // ...
>       }
>     }
>     ```
>     
>     I'm more-so trying to understand what the reasoning is here.

A couple of thoughts here:

Allocations occur to roles. Frameworks that are subscribed to the role can receive the allocations.
A historical artifact is that we have a second level of implicitly weighted fair sharing between
frameworks in a role to maintain our "pessimistic" offer technique, and so the allocator has
knowlege of the frameworks. This approach was unfortunate since we have the framework id acting
as a pseudo role but there is no control over the weights or quota for this pseudo role. The
vision is that we should just perform allocations to roles, what this would mean is that if
there are multiple frameworks listening to a role, they compete in a first come first serve
fashion (if this is not desired, sub-roles can be used to impose sharing and quota constraints
between the frameworks).

Given this, I suspect we could design the allocator to have no knowledge of what a framework
is, and rather have the allocator operate purely on roles. This would lead to a "role" centric
API. In this model, the master understands how to deliver the allocations to the roles based
on which frameworks are subscribed to the role. The only wrinkle here that I see is that framework
capabilities have an influence on allocation.

This is why I made the roles explicit, to reflect that allocations are occuring to roles.
Thoughts?


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > include/mesos/allocator/allocator.hpp, line 264
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615402#file1615402line264>
> >
> >     Do we need this detail of our allocator implementation in the generic interface?

Hm.. I'm not sure I follow how this is an implementation detail, it's currently an invariant
we impose on the caller, so the caller should know about it, no?


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > include/mesos/allocator/allocator.hpp, lines 333-335
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615402#file1615402line333>
> >
> >     Internal detail?

Hm.. I'm not sure I follow how this is an implementation detail, it's currently an invariant
we impose on the caller, so the caller should know about it, no?


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 691-696
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line691>
> >
> >     Is this detailed knowledge of `Resource` internals necessary here? Below we
use the same adjusted `operation` regardless of its type.

I'll remove the injection and leave the validation TODO.


> On Jan. 30, 2017, 4:12 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 709-710
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615405#file1615405line709>
> >
> >     Lets try to put this code into the caller (e.g., `Master::_accept`). It seems
that would introduce less current and future breakage in custom allocator modules.

I'll remove the injection and leave the validation TODO.


- Benjamin


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


On Jan. 25, 2017, 9:55 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55870/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2017, 9:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6635
>     https://issues.apache.org/jira/browse/MESOS-6635
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The allocator now sets `Resource.AllocationInfo` when performing
> an allocation, and the interface is updated to expose allocations
> for each role within a framework.
> 
> The allocator also assumes that `Resource.AllocationInfo` is set
> for inbound resources that are allocated (e.g. when adding an agent,
> when adding a framework, when recovering resources). Note however,
> that the necessary changes to the master and agent to enforce this
> will be done via separate patches.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp 558594983beb6ff49c1fddf875ba29c1983b1288 
>   src/master/allocator/mesos/allocator.hpp 8e0f37a5eedd4d71d765991c0039b49c96f0ca53 
>   src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d

>   src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a

>   src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b 
>   src/master/master.cpp 73159328ce3fd838e02eba0e6a30cf69efc319ba 
>   src/tests/allocator.hpp 1f9261d1e7afa027bebc07915ba3e871a58e6558 
> 
> Diff: https://reviews.apache.org/r/55870/diff/
> 
> 
> Testing
> -------
> 
> Adjustments to existing tests are split out into a subsequent patch.
> 
> New tests for frameworks having multiple roles will be added as a follow up.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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