Re: Review Request 71020: Added overcommit and hierarchical inclusion check for `UPDATE_QUOTA`.

[Meng Zhu]

> On July 7, 2019, 3:50 p.m., Benjamin Mahler wrote:
> > I assume we'll track the missing pieces with additional tickets or leave 
> > the existing ticket open until we have them?

Yes, let's keep the ticket open.


> On July 7, 2019, 3:50 p.m., Benjamin Mahler wrote:
> > src/master/quota_handler.cpp
> > Lines 94 (patched)
> > 
> >
> > This seems like a transitive property that doesn't need to be spelled 
> > out?
> > 
> > sum of children's guarantees <=
> > parent's guarantee <=
> > parent's limit
> > 
> > It seems somewhat unnatural to directly check:
> > 
> > sum of children's guarantees <=
> > parent's limit
> > 
> > rather than check:
> > 
> > sum of children's guarantees <=
> > parent's limit
> > 
> > &&
> > 
> > guarantee <= limit

Good point! Updated the comment, commit description and related tickets.


> On July 7, 2019, 3:50 p.m., Benjamin Mahler wrote:
> > src/master/quota_handler.cpp
> > Lines 467-515 (patched)
> > 
> >
> > Hm.. is this code copied from the original quota handler? Is your plan 
> > to have this be the single path and the old api goes through this?
> > 
> > if so, can you clarify with a TODO that this is duplicated from the 
> > original quota handler and the plan is to have that go through this path?

The logic here is a bit different (supposedly more performant). Added a TODO to 
pull this out as a standalone function to be shared by the old and the new.


> On July 7, 2019, 3:50 p.m., Benjamin Mahler wrote:
> > src/master/quota_handler.cpp
> > Lines 485 (patched)
> > 
> >
> > BadRequest vs Conflict below seems a bit inconsitent? They're both 
> > violations of the constraints?

Changed to return `BadRequest` in both cases.


- Meng


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


On July 7, 2019, 3:12 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71020/
> ---
> 
> (Updated July 7, 2019, 3:12 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9812
> https://issues.apache.org/jira/browse/MESOS-9812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The overcommit check validates that the total quota guarantees in
> the cluster is contained by the cluster capacity.
> 
> The hierarchical inclusion check validates that the sum of
> children's  guarantees is contained by the parent guarantee.
> 
> Further validation is needed for:
> 
> - Check a role's limit is less than its current consumption.
> - Check a role's limit is less than its parent's limit.
> - Check the sum of children's guarantees are less than its
> parent's limit.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp f9c743170461d8d83180db20c917d3842241d4df 
> 
> 
> Diff: https://reviews.apache.org/r/71020/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> tests in r/71022/
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71020: Added overcommit and hierarchical inclusion check for `UPDATE_QUOTA`.

[Benjamin Mahler]

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


Fix it, then Ship it!




I assume we'll track the missing pieces with additional tickets or leave the 
existing ticket open until we have them?


src/master/quota_handler.cpp
Lines 94 (patched)


This seems like a transitive property that doesn't need to be spelled out?

sum of children's guarantees <=
parent's guarantee <=
parent's limit

It seems somewhat unnatural to directly check:

sum of children's guarantees <=
parent's limit

rather than check:

sum of children's guarantees <=
parent's limit

&&

guarantee <= limit



src/master/quota_handler.cpp
Lines 467-515 (patched)


Hm.. is this code copied from the original quota handler? Is your plan to 
have this be the single path and the old api goes through this?

if so, can you clarify with a TODO that this is duplicated from the 
original quota handler and the plan is to have that go through this path?



src/master/quota_handler.cpp
Lines 472 (patched)


up-to-date s/the//



src/master/quota_handler.cpp
Lines 485 (patched)


BadRequest vs Conflict below seems a bit inconsitent? They're both 
violations of the constraints?


- Benjamin Mahler


On July 7, 2019, 10:12 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71020/
> ---
> 
> (Updated July 7, 2019, 10:12 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9812
> https://issues.apache.org/jira/browse/MESOS-9812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The overcommit check validates that the total quota guarantees in
> the cluster is contained by the cluster capacity.
> 
> The hierarchical inclusion check validates that the sum of
> children's  guarantees is contained by the parent guarantee.
> 
> Further validation is needed for:
> 
> - Check a role's limit is less than its current consumption.
> - Check a role's limit is less than its parent's limit.
> - Check the sum of children's guarantees are less than its
> parent's limit.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp f9c743170461d8d83180db20c917d3842241d4df 
> 
> 
> Diff: https://reviews.apache.org/r/71020/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> tests in r/71022/
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 71020: Added overcommit and hierarchical inclusion check for `UPDATE_QUOTA`.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

The overcommit check validates that the total quota guarantees in
the cluster is contained by the cluster capacity.

The hierarchical inclusion check validates that the sum of
children's  guarantees is contained by the parent guarantee.

Further validation is needed for:

- Check a role's limit is less than its current consumption.
- Check a role's limit is less than its parent's limit.
- Check the sum of children's guarantees are less than its
parent's limit.


Diffs
-

  src/master/quota_handler.cpp f9c743170461d8d83180db20c917d3842241d4df 


Diff: https://reviews.apache.org/r/71020/diff/1/


Testing
---

make check

tests in r/71022/


Thanks,

Meng Zhu