You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2018/07/05 21:22:09 UTC
Re: Review Request 67444: Persisted role consumed quota info in the
allocator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67444/#review205762
-----------------------------------------------------------
Looks like a great cleanup!
Persist tends to carry the connotation of writing something to durable storage. How about:
```
Made quota consumption tracking event-driven in the allocator.
The allocator needs to keep track of role consumed quota
to maintain quota headroom. Currently this info is
constructed on the fly prior to each allocation cycle.
This patch lets the allocator track quota consumption
across allocation cycles in an event-driven manner to improve
performance and reduce code complexity.
```
src/master/allocator/mesos/hierarchical.hpp
Lines 509-512 (patched)
<https://reviews.apache.org/r/67444/#comment288630>
```
// To enforce quota, we keep track of how much quota is consumed by each role.
// Quota consumption always includes the role's reservations (since they cannot
// be allocated to other roles) as well as any allocated resources for the role.
//
// This is only tracked for roles with non-default quota.
```
src/master/allocator/mesos/hierarchical.hpp
Lines 526 (patched)
<https://reviews.apache.org/r/67444/#comment288632>
How about `consumedQuotaScalarQuantities`? We don't call it `rolesQuota` above for example.
src/master/allocator/mesos/hierarchical.cpp
Lines 1387-1390 (patched)
<https://reviews.apache.org/r/67444/#comment288631>
I don't think we need to repeat ourselves with the "in other words" part. I think we just need to explain why reservations are included in one place (in the header):
```
// Track quota consumption.
//
// A role's quota consumption includes its allocation as well as any
// unallocated reservations:
//
// Consumed Quota = reservations + unreserved allocation
// = reservations + allocation - allocated reservations
```
src/master/allocator/mesos/hierarchical.cpp
Lines 1404 (patched)
<https://reviews.apache.org/r/67444/#comment288636>
It wasn't clear to me when reading this why it needs to be done on each agent, so we should probably clarify that?
```
// Lastly, subtract allocated reservations. This needs to be done on a per-agent basis because ...
```
src/master/allocator/mesos/hierarchical.cpp
Lines 1405-1406 (patched)
<https://reviews.apache.org/r/67444/#comment288637>
We probably want a TODO to optimize this by avoiding the copy by returning a const reference to the map?
src/master/allocator/mesos/hierarchical.cpp
Lines 1385-1390 (original), 1413-1418 (patched)
<https://reviews.apache.org/r/67444/#comment288638>
Just an unrelated observation, this note looks misleading, since the master does try to rescind offers to re-balance.
src/master/allocator/mesos/hierarchical.cpp
Lines 1439-1440 (patched)
<https://reviews.apache.org/r/67444/#comment288639>
It looks like it follows the same order as operations in setQuota, so I would expect this to be done after metrics.removeQuota. Any reason not to?
Actually, it seems like the metric should get added after quota tracking and removed before quota tracking is removed, since I would imagine the metrics look at the tracking information. It looks like we currently only expose 'allocated' instead of 'consumed' for now, but we probably want to expose 'consumed' soon, is there a ticket?
src/master/allocator/mesos/hierarchical.cpp
Lines 2613-2614 (original), 2576-2577 (patched)
<https://reviews.apache.org/r/67444/#comment288640>
Hm.. an aside, but I was puzzled about this function. Since it takes role->reservations it loses information for non-scalar resources.
Fortunately, this function only uses the scalar quantities anyway, but we should make the interface clearly take quantities to clarify the assumption.
src/master/allocator/mesos/hierarchical.cpp
Lines 2586-2588 (patched)
<https://reviews.apache.org/r/67444/#comment288642>
It's pretty hard to reason from here about whether this is correct. For example, how do we know that these quantities were not already tracked because they were allocated prior to becoming reserved? If that invariant doesn't hold we'll double count?
src/master/allocator/mesos/hierarchical.cpp
Lines 2607-2611 (patched)
<https://reviews.apache.org/r/67444/#comment288643>
Ditto here, it's hard to reason about why we can remove it here, what if the resources are unreserved but remain allocated?
src/master/allocator/mesos/hierarchical.cpp
Lines 2727-2728 (patched)
<https://reviews.apache.org/r/67444/#comment288644>
Hm.. there seem to be some invariants here in how the tracking functions are called but I can't quite figure them out. Is there a way to enforce them?
Let's say I have allocated resoures, these should be tracked. Then I make them reserved, are they somehow untracked as allocated, the reservations get tracked, then the updated allocation gets re-tracked? Is that why we skip the reserved resources here?
We probably need to spell this out more clearly for the reader.
src/master/allocator/mesos/hierarchical.cpp
Lines 2768-2771 (patched)
<https://reviews.apache.org/r/67444/#comment288645>
Ditto here.
- Benjamin Mahler
On June 28, 2018, 8:55 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67444/
> -----------------------------------------------------------
>
> (Updated June 28, 2018, 8:55 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8802
> https://issues.apache.org/jira/browse/MESOS-8802
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The allocator needs to keep track of role consumed quota
> to maintain quota headroom. Currently this info is
> constructed on the fly prior to each allocation cycle.
>
> This patch lets the allocator buildup and persist this info
> across allocation cycles to improve performance and reduce
> code complexity.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26
> src/master/allocator/mesos/hierarchical.cpp cbdfb2ba9c25755ac631557e0e7dbd721f861a4d
>
>
> Diff: https://reviews.apache.org/r/67444/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67444: Made quota consumption tracking
event-driven in the allocator.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > Looks like a great cleanup!
> >
> > Persist tends to carry the connotation of writing something to durable storage. How about:
> >
> > ```
> > Made quota consumption tracking event-driven in the allocator.
> >
> > The allocator needs to keep track of role consumed quota
> > to maintain quota headroom. Currently this info is
> > constructed on the fly prior to each allocation cycle.
> >
> > This patch lets the allocator track quota consumption
> > across allocation cycles in an event-driven manner to improve
> > performance and reduce code complexity.
> > ```
Sounds good. Thanks!
> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1404 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1404>
> >
> > It wasn't clear to me when reading this why it needs to be done on each agent, so we should probably clarify that?
> >
> > ```
> > // Lastly, subtract allocated reservations. This needs to be done on a per-agent basis because ...
> > ```
I do not think we have anything interesting to say here. We subtract on a per-agent basis because currently there is no aggregated bookkeeping info. In the future, it is possible for us to track aggregated allocated reservations and subtract all at once. Dropping.
> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1405-1406 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1405>
> >
> > We probably want a TODO to optimize this by avoiding the copy by returning a const reference to the map?
Looks like the `quotaRoleSorter->allocation(role)` is already returning a const ref, must be a slip earlier. Will update these in an immediate followup patch.
> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1385-1390 (original), 1413-1418 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1413>
> >
> > Just an unrelated observation, this note looks misleading, since the master does try to rescind offers to re-balance.
"Re-balancing" means a change of allocation? Anyway, added a todo here:
// TODO(mzhu): Re-evaluate the above assumption and consider triggering an
// allocation here.
> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1439-1440 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1439>
> >
> > It looks like it follows the same order as operations in setQuota, so I would expect this to be done after metrics.removeQuota. Any reason not to?
> >
> > Actually, it seems like the metric should get added after quota tracking and removed before quota tracking is removed, since I would imagine the metrics look at the tracking information. It looks like we currently only expose 'allocated' instead of 'consumed' for now, but we probably want to expose 'consumed' soon, is there a ticket?
Filed MESOS-9123 for exposing role consumed quota.
Updated `metrics.setQuota` to be done after the tracking info update. But for `metrics.removeQuota`, it does not depend on any of the tracking info since it is all about removing the metric entries. I think it can be done either before or after the tracking info update. For consistency, let's update the metrics after the tracking info update in both add and remove.
> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2586-2588 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line2654>
> >
> > It's pretty hard to reason from here about whether this is correct. For example, how do we know that these quantities were not already tracked because they were allocated prior to becoming reserved? If that invariant doesn't hold we'll double count?
>
> Meng Zhu wrote:
> Thanks for catching this!
>
> The invariant should be:
> (1) when tracking reservation, only track unallocated reservations
> (2) when track allocation, only track unreserved allocations
>
> (2) is already being done. (1) is hard to do given the current abstraction, we will need more than raw "reservations". Will need more refactoring on the Slave struct in the allocator to make this work and readable. Will come back to this later.
>
> In addition, this reminds me that we do not have tests for the above scenario yet. Will add some.
Now fixed.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67444/#review205762
-----------------------------------------------------------
On July 31, 2018, 4:56 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67444/
> -----------------------------------------------------------
>
> (Updated July 31, 2018, 4:56 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8802
> https://issues.apache.org/jira/browse/MESOS-8802
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The allocator needs to keep track of role consumed quota
> to maintain quota headroom. Currently this info is
> constructed on the fly prior to each allocation cycle.
>
> This patch lets the allocator track quota consumption
> across allocation cycles in an event-driven manner to improve
> performance and reduce code complexity.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0cd2fac17f09110c46b8540523a9c935f156f858
> src/master/allocator/mesos/hierarchical.cpp 35992474eacb8b14ae57e1dc23307e1542f63cb5
>
>
> Diff: https://reviews.apache.org/r/67444/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67444: Persisted role consumed quota info in the
allocator.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2586-2588 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line2654>
> >
> > It's pretty hard to reason from here about whether this is correct. For example, how do we know that these quantities were not already tracked because they were allocated prior to becoming reserved? If that invariant doesn't hold we'll double count?
Thanks for catching this!
The invariant should be:
(1) when tracking reservation, only track unallocated reservations
(2) when track allocation, only track unreserved allocations
(2) is already being done. (1) is hard to do given the current abstraction, we will need more than raw "reservations". Will need more refactoring on the Slave struct in the allocator to make this work and readable. Will come back to this later.
In addition, this reminds me that we do not have tests for the above scenario yet. Will add some.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67444/#review205762
-----------------------------------------------------------
On June 28, 2018, 1:55 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67444/
> -----------------------------------------------------------
>
> (Updated June 28, 2018, 1:55 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8802
> https://issues.apache.org/jira/browse/MESOS-8802
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The allocator needs to keep track of role consumed quota
> to maintain quota headroom. Currently this info is
> constructed on the fly prior to each allocation cycle.
>
> This patch lets the allocator buildup and persist this info
> across allocation cycles to improve performance and reduce
> code complexity.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26
> src/master/allocator/mesos/hierarchical.cpp cbdfb2ba9c25755ac631557e0e7dbd721f861a4d
>
>
> Diff: https://reviews.apache.org/r/67444/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>