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 2019/07/08 18:07:31 UTC
Review Request 71029: Added Role::quotaConsumption to the master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71029/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko and Meng Zhu.
Bugs: MESOS-9871
https://issues.apache.org/jira/browse/MESOS-9871
Repository: mesos
Description
-------
Quota consumption is computed as follows:
Allocation + Unallocated Reservation ==
Reservations + Unreserved Allocation
That is, reservations count towards quota regardless of whether
they're allocated. Allocation counts towards quota. Offered resources
*do not* count towards quota, this is to (1) provide stability of the
quota consumption metrics in the face of offers flowing in and out,
and (2) to ensure that we treat offers as rescindable and therefore
not yet "counting" towards quota. Also, if in the future schedulers
are offered more than their quota to improve their choices, counting
offered resources as quota consumption will be problematic.
Diffs
-----
src/master/framework.cpp c35de2574ef683a9f397a957422fdbf8fa751313
src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
Diff: https://reviews.apache.org/r/71029/diff/1/
Testing
-------
Added a test in a subsequent patch.
Thanks,
Benjamin Mahler
Re: Review Request 71029: Added Role::consumedQuota to the master.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71029/
-----------------------------------------------------------
(Updated July 10, 2019, 1:18 a.m.)
Review request for mesos, Andrei Sekretenko and Meng Zhu.
Changes
-------
- Updated to correctly handle hierarchical accounting (by looping over all frameworks)
- Updated to avoid Resources arithemtic per mzhu's suggestion
Summary (updated)
-----------------
Added Role::consumedQuota to the master.
Bugs: MESOS-9871
https://issues.apache.org/jira/browse/MESOS-9871
Repository: mesos
Description (updated)
-------
Quota consumption is computed as follows:
Allocation + Unallocated Reservation ==
Reservations + Unreserved Allocation
That is, reservations count towards quota regardless of whether
they're allocated. Allocation counts towards quota. Offered resources
*do not* count towards quota, this is to (1) provide stability of the
quota consumption metrics in the face of offers flowing in and out,
and (2) to ensure that we treat offers as rescindable and therefore
not yet "counting" towards quota. Also, if in the future schedulers
are offered more than their quota to improve their choices, counting
offered resources as consumed quota will be problematic.
Diffs (updated)
-----
src/master/framework.cpp c35de2574ef683a9f397a957422fdbf8fa751313
src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
Diff: https://reviews.apache.org/r/71029/diff/2/
Changes: https://reviews.apache.org/r/71029/diff/1-2/
Testing
-------
Added a test in a subsequent patch.
Thanks,
Benjamin Mahler
Re: Review Request 71029: Added Role::quotaConsumption to the master.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71029/#review216436
-----------------------------------------------------------
src/master/master.hpp
Lines 2714-2715 (patched)
<https://reviews.apache.org/r/71029/#comment303674>
If we want to expose the composition, reserved unused, unreserved used and etc. We probably want to split this into several functions? e.g. allocation(), reservation(), used_reservaiton() and then combine as needed in the endpoint handler.
- Meng Zhu
On July 8, 2019, 11:07 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71029/
> -----------------------------------------------------------
>
> (Updated July 8, 2019, 11:07 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-9871
> https://issues.apache.org/jira/browse/MESOS-9871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota consumption is computed as follows:
>
> Allocation + Unallocated Reservation ==
> Reservations + Unreserved Allocation
>
> That is, reservations count towards quota regardless of whether
> they're allocated. Allocation counts towards quota. Offered resources
> *do not* count towards quota, this is to (1) provide stability of the
> quota consumption metrics in the face of offers flowing in and out,
> and (2) to ensure that we treat offers as rescindable and therefore
> not yet "counting" towards quota. Also, if in the future schedulers
> are offered more than their quota to improve their choices, counting
> offered resources as quota consumption will be problematic.
>
>
> Diffs
> -----
>
> src/master/framework.cpp c35de2574ef683a9f397a957422fdbf8fa751313
> src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
>
>
> Diff: https://reviews.apache.org/r/71029/diff/1/
>
>
> Testing
> -------
>
> Added a test in a subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71029: Added Role::consumedQuota to the master.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 9, 2019, 12:10 a.m., Meng Zhu wrote:
> > src/master/master.hpp
> > Lines 2730 (patched)
> > <https://reviews.apache.org/r/71029/diff/1/?file=2153963#file2153963line2730>
> >
> > `totalUsedResources` here also includes the executor resources which are not managed by the allocator. Do we want to include that in the consumption?
> >
> > If no, then the consumption is not going to be consistent with framework stats. But if yes, since the consumption will be compared with quota (both from the user perspective as well as internally in the future), that means when operator configures the quota, they need to take executor resources into account. I am not sure if that's good user experience.
> >
> > For simplicity, I am OK with leaving it as is now. But let's document the rationale and/or any todos.
> >
> > Also please update the description for this.
> totalUsedResources here also includes the executor resources which are not managed by the allocator. Do we want to include that in the consumption?
Per offline discussion, this was the command executor case (where the agent generates the ExecutorInfo and adds resources to it). The master doesn't store command executors, so it doesn't have the same overcommit visible.
> On July 9, 2019, 12:10 a.m., Meng Zhu wrote:
> > src/master/master.hpp
> > Lines 2743-2752 (patched)
> > <https://reviews.apache.org/r/71029/diff/1/?file=2153963#file2153963line2743>
> >
> > how about total_reservaiton - used_reservation:
> >
> > ```
> > ResourceQuantities unallocatedReservation;
> >
> > foreachvalue (Slave* slave, master->slaves.registered) {
> > ResourceQuantities totalReservation =
> > ResourceQuantities::fromScalarResources(
> > slave->totalResources.filter(reservedToRoleSubtree).scalars());
> >
> > ResourceQuantities usedReservation =
> > ResourceQuantities::fromScalarResources(
> > slave->usedResources.filter(reservedToRoleSubtree).scalars());
> >
> > unallocatedReservation += totalReservation - usedReservation;
> > }
> >
> > ```
> >
> > Should also be more performant to use ResourceQuantities and no need to clear the allocation info.
Great suggestion, thanks! This is much better without the Resources arithmetic
(note that the `slave->usedResources` and needs to be looped over)
> On July 9, 2019, 12:10 a.m., Meng Zhu wrote:
> > src/master/master.hpp
> > Lines 2758 (patched)
> > <https://reviews.apache.org/r/71029/diff/1/?file=2153963#file2153963line2758>
> >
> > Nits, how about avoiding the addition and the two variables by just having a `consumption`:
> >
> > ```
> > // Quota consumption = allocation + unallocated reservation
> >
> > ResourceQuantities consumption;
> >
> > // Add allocation
> >
> > ....
> >
> >
> > // Add unallocated reservation
> > ...
> >
> > return consumption;
> >
> > ```
Hm.. showing the computation of 'allocation' and 'unallocated reservation' by having them in variables and then showing the final addition (which is the same as the comment's addition) seems easier to follow?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71029/#review216433
-----------------------------------------------------------
On July 10, 2019, 1:18 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71029/
> -----------------------------------------------------------
>
> (Updated July 10, 2019, 1:18 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-9871
> https://issues.apache.org/jira/browse/MESOS-9871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota consumption is computed as follows:
>
> Allocation + Unallocated Reservation ==
> Reservations + Unreserved Allocation
>
> That is, reservations count towards quota regardless of whether
> they're allocated. Allocation counts towards quota. Offered resources
> *do not* count towards quota, this is to (1) provide stability of the
> quota consumption metrics in the face of offers flowing in and out,
> and (2) to ensure that we treat offers as rescindable and therefore
> not yet "counting" towards quota. Also, if in the future schedulers
> are offered more than their quota to improve their choices, counting
> offered resources as consumed quota will be problematic.
>
>
> Diffs
> -----
>
> src/master/framework.cpp c35de2574ef683a9f397a957422fdbf8fa751313
> src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
>
>
> Diff: https://reviews.apache.org/r/71029/diff/2/
>
>
> Testing
> -------
>
> Added a test in a subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71029: Added Role::quotaConsumption to the master.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71029/#review216433
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.hpp
Lines 2730 (patched)
<https://reviews.apache.org/r/71029/#comment303668>
`totalUsedResources` here also includes the executor resources which are not managed by the allocator. Do we want to include that in the consumption?
If no, then the consumption is not going to be consistent with framework stats. But if yes, since the consumption will be compared with quota (both from the user perspective as well as internally in the future), that means when operator configures the quota, they need to take executor resources into account. I am not sure if that's good user experience.
For simplicity, I am OK with leaving it as is now. But let's document the rationale and/or any todos.
Also please update the description for this.
src/master/master.hpp
Lines 2743-2752 (patched)
<https://reviews.apache.org/r/71029/#comment303671>
how about total_reservaiton - used_reservation:
```
ResourceQuantities unallocatedReservation;
foreachvalue (Slave* slave, master->slaves.registered) {
ResourceQuantities totalReservation =
ResourceQuantities::fromScalarResources(
slave->totalResources.filter(reservedToRoleSubtree).scalars());
ResourceQuantities usedReservation =
ResourceQuantities::fromScalarResources(
slave->usedResources.filter(reservedToRoleSubtree).scalars());
unallocatedReservation += totalReservation - usedReservation;
}
```
Should also be more performant to use ResourceQuantities and no need to clear the allocation info.
src/master/master.hpp
Lines 2758 (patched)
<https://reviews.apache.org/r/71029/#comment303672>
Nits, how about avoiding the addition and the two variables by just having a `consumption`:
```
// Quota consumption = allocation + unallocated reservation
ResourceQuantities consumption;
// Add allocation
....
// Add unallocated reservation
...
return consumption;
```
- Meng Zhu
On July 8, 2019, 11:07 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71029/
> -----------------------------------------------------------
>
> (Updated July 8, 2019, 11:07 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-9871
> https://issues.apache.org/jira/browse/MESOS-9871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota consumption is computed as follows:
>
> Allocation + Unallocated Reservation ==
> Reservations + Unreserved Allocation
>
> That is, reservations count towards quota regardless of whether
> they're allocated. Allocation counts towards quota. Offered resources
> *do not* count towards quota, this is to (1) provide stability of the
> quota consumption metrics in the face of offers flowing in and out,
> and (2) to ensure that we treat offers as rescindable and therefore
> not yet "counting" towards quota. Also, if in the future schedulers
> are offered more than their quota to improve their choices, counting
> offered resources as quota consumption will be problematic.
>
>
> Diffs
> -----
>
> src/master/framework.cpp c35de2574ef683a9f397a957422fdbf8fa751313
> src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
>
>
> Diff: https://reviews.apache.org/r/71029/diff/1/
>
>
> Testing
> -------
>
> Added a test in a subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 71029: Added Role::quotaConsumption to the master.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71029/#review216422
-----------------------------------------------------------
src/master/master.hpp
Lines 2728 (patched)
<https://reviews.apache.org/r/71029/#comment303649>
If we want this to accurately capture the hierarchical consumption, this needs to loop over all frameworks (i.e. `master->frameworks`) instead.
- Benjamin Mahler
On July 8, 2019, 6:07 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71029/
> -----------------------------------------------------------
>
> (Updated July 8, 2019, 6:07 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
>
>
> Bugs: MESOS-9871
> https://issues.apache.org/jira/browse/MESOS-9871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota consumption is computed as follows:
>
> Allocation + Unallocated Reservation ==
> Reservations + Unreserved Allocation
>
> That is, reservations count towards quota regardless of whether
> they're allocated. Allocation counts towards quota. Offered resources
> *do not* count towards quota, this is to (1) provide stability of the
> quota consumption metrics in the face of offers flowing in and out,
> and (2) to ensure that we treat offers as rescindable and therefore
> not yet "counting" towards quota. Also, if in the future schedulers
> are offered more than their quota to improve their choices, counting
> offered resources as quota consumption will be problematic.
>
>
> Diffs
> -----
>
> src/master/framework.cpp c35de2574ef683a9f397a957422fdbf8fa751313
> src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8
>
>
> Diff: https://reviews.apache.org/r/71029/diff/1/
>
>
> Testing
> -------
>
> Added a test in a subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>