Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-11 Thread Benjamin Mahler 

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


Fix it, then Ship it!




Looks good! Just some comments related to comments and naming, no code changes, 
so I can take care of these for you and get this committed!


src/master/allocator/mesos/hierarchical.cpp
Lines 1596-1602 (patched)


Looks like we could add the same structure to this comment as I suggested 
in the previous review.



src/master/allocator/mesos/hierarchical.cpp
Line 1602 (original), 1640 (patched)


a quantity?



src/master/allocator/mesos/hierarchical.cpp
Lines 1602-1608 (original), 1640-1646 (patched)


Some symmetry here would be helpful:

roleReservationQuantities vs 
roleAllocatedReservationQuantities

The maps:

reservationScalarQuantities vs
allocatedReservationScalarQuantites

Probably it makes sense to be consistent with "quantities" vs 
"scalarQuantites"?



src/master/allocator/mesos/hierarchical.cpp
Line 1606 (original), 1644 (patched)


a quantity?



src/master/allocator/mesos/hierarchical.cpp
Lines 1648-1656 (patched)


Maybe a little bit more of an explanation of why we count reservations?

```
  // We charge a role against its quota by considering its
  // allocation as well as any unallocated reservations
  // since reservations are bound to the role. In other
  // words, we always consider reservations as consuming
  // quota, regardless of whether they are allocated.
  // The equation used here is:
  //
  //   Consumed Quota = reservations + unreserved allocation
  //  = reservations + (allocation - allocated 
reservations)
  //
  // This is a __quantity__ with no meta-data.
  Resources resourcesChargedAgainstQuota =
roleReservationScalarQuantities +
  (getQuotaRoleAllocatedResources(role) -
   roleAllocatedReservationScalarQuantities);
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1658-1663 (patched)


How about?

```
  // If quota for the role is considered satisfied, then we only
  // further allocate reservations for the role.
  //
  // More precisely, we stop allocating unreserved resources if at
  // least one of the resource guarantees is considered consumed.
  // This technique prevents gaming of the quota allocation,
  // see MESOS-6432.
```

I feel like "satisfied" is explained in the second paragraph, and this 
updates that paragram to say "considered consumed" which matches up with the 
comment about what we consider consumed against quota.



src/master/allocator/mesos/hierarchical.cpp
Lines 1682-1693 (original), 1732-1743 (patched)


Looks good, thanks for updating this!



src/master/allocator/mesos/hierarchical.cpp
Lines 1791 (patched)


// Update the tracking of allocated reservations.

Seems less brittle to becoming stale if the variable name changes.



src/master/allocator/mesos/hierarchical.cpp
Lines 1800-1802 (patched)


Just an observation, unlike the tracked map, this map will contain role 
entries with empty resources. Don't think we need to write this down, just an 
inconsistency between the two that may crop up later (e.g. if this map gets 
moved to a allocator global map).


- Benjamin Mahler


On Dec. 11, 2017, 4:29 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64304/
> ---
> 
> (Updated Dec. 11, 2017, 4:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
> Park.
> 
> 
> Bugs: MESOS-4527
> https://issues.apache.org/jira/browse/MESOS-4527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enforced quota limit in the presence of unallocated reservations.
> Also modify the allocation process such that the first stage allocation
> is solely for quota roles while the second stage is solely for
> non-quota roles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9d8b6714d060f67d570c5653798e705248781869 
> 
> 
> Diff: https://reviews.apache.org/r/64304/diff/6/
> 
> 
>

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-10 Thread Meng Zhu 

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

(Updated Dec. 10, 2017, 8:29 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
Park.


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


Repository: mesos


Description
---

Enforced quota limit in the presence of unallocated reservations.
Also modify the allocation process such that the first stage allocation
is solely for quota roles while the second stage is solely for
non-quota roles.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
9d8b6714d060f67d570c5653798e705248781869 


Diff: https://reviews.apache.org/r/64304/diff/6/

Changes: https://reviews.apache.org/r/64304/diff/5-6/


Testing
---

Introduced two dedicated tests.
make check.


Thanks,

Meng Zhu

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-08 Thread Benjamin Mahler 

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



Great to see this getting fixed!

First pass over just the non-testing code. Can you split out the test into a 
separate patch?


src/master/allocator/mesos/hierarchical.cpp
Lines 1578-1579 (patched)


s/every allocation/every allocation run/

You might want to clarify that given the other TODO below this one, the 
plan is to remove the discrepancy between what the sorter considered allocated 
and what we'd like to count as allocated for fair sharing and quota. i.e. we 
should be able to simplify this and just retrieve what the role's "allocation" 
is in the sorter



src/master/allocator/mesos/hierarchical.cpp
Lines 1584-1597 (patched)


does this need to be a function? Can you just imbed it in your loop below 
over each quota role? you also wouldn't need the CHECK anymore?



src/master/allocator/mesos/hierarchical.cpp
Lines 1587-1588 (patched)


s/&//



src/master/allocator/mesos/hierarchical.cpp
Lines 1587-1588 (patched)


2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1590 (patched)


maybe allocatedReservedQuantity?



src/master/allocator/mesos/hierarchical.cpp
Lines 1591 (patched)


s/(/ (/

.values() will copy (currently), can you use foreachvalue?



src/master/allocator/mesos/hierarchical.cpp
Lines 1592-1593 (patched)


A note on why we need toUnreserved would be helpful for the reader.



src/master/allocator/mesos/hierarchical.cpp
Lines 1592-1593 (patched)


2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1599-1601 (patched)


Let's call it quantities then?

roleAllocatedReservedQuantity

(not sure you need "roles" here)



src/master/allocator/mesos/hierarchical.cpp
Lines 1611 (patched)


slave



src/master/allocator/mesos/hierarchical.cpp
Lines 1618-1621 (patched)


s/name/role/ and 2 space indents



src/master/allocator/mesos/hierarchical.cpp
Lines 1602-1609 (original), 1649-1667 (patched)


2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1663 (patched)


What goes wrong without this being fixed? Is it fixed later in your chain? 
Seems like you wrote a test for it?



src/master/allocator/mesos/hierarchical.cpp
Lines 1666-1667 (patched)


How about (x - y) + z?



src/master/allocator/mesos/hierarchical.cpp
Lines 1673 (patched)


unreserved



src/master/allocator/mesos/hierarchical.cpp
Lines 1808 (patched)


2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1808-1810 (patched)


you can take the slaveId by reference to avoid copying it?



src/master/allocator/mesos/hierarchical.cpp
Lines 1811-1813 (patched)


rolesAllocatedReservedResources[role] +=
  (resources.reserved(role).nonShared() + newShared)
.createStrippedScalarQuantity().toUnreserved();



src/master/allocator/mesos/hierarchical.cpp
Lines 1813 (patched)


a comment here as well about why we need toUnreserved would be helpful


- Benjamin Mahler


On Dec. 8, 2017, 9:51 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64304/
> ---
> 
> (Updated Dec. 8, 2017, 9:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
> Park.
> 
> 
> Bugs: MESOS-4527
> https://issues.apache.org/jira/browse/MESOS-4527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enforced quota limit in the presence of unallocated reservations.
> Also modify the allocation process such that the first stage allocation
> is solely for quota roles while the second stage is solely for
> non-quota roles.
> 
> 
> Diffs
> -
> 
>   src/ma

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-08 Thread Meng Zhu 

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

(Updated Dec. 8, 2017, 1:51 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
Park.


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


Repository: mesos


Description
---

Enforced quota limit in the presence of unallocated reservations.
Also modify the allocation process such that the first stage allocation
is solely for quota roles while the second stage is solely for
non-quota roles.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
9d8b6714d060f67d570c5653798e705248781869 
  src/tests/hierarchical_allocator_tests.cpp 
862f4683da04d37d9fe9f471d6ec9cd7751f39ec 


Diff: https://reviews.apache.org/r/64304/diff/5/

Changes: https://reviews.apache.org/r/64304/diff/4-5/


Testing
---

Introduced two dedicated tests.
make check.


Thanks,

Meng Zhu

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-05 Thread Meng Zhu 

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

(Updated Dec. 5, 2017, 2:42 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
Park.


Changes
---

Patch updated. Thanks for the review!


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


Repository: mesos


Description
---

Enforced quota limit in the presence of unallocated reservations.
Also modify the allocation process such that the first stage allocation
is solely for quota roles while the second stage is solely for
non-quota roles.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
4bc9fb6f787224114f1285937cf979ace46d8ea3 
  src/tests/hierarchical_allocator_tests.cpp 
154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


Diff: https://reviews.apache.org/r/64304/diff/4/

Changes: https://reviews.apache.org/r/64304/diff/3-4/


Testing (updated)
---

Introduced two dedicated tests.
make check.


Thanks,

Meng Zhu

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-05 Thread Meng Zhu 


> On Dec. 4, 2017, 5:08 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1272 (patched)
> > 
> >
> > After reading through the test, this is the dynamic reservation case 
> > only? Should we call this test `QuotaLimitWithDynamicReservation` and have 
> > another test for static reservations? Or do you want to test both cases in 
> > this test?

Tests parameterized.


> On Dec. 4, 2017, 5:08 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1363 (patched)
> > 
> >
> > This test is also just dynamic reservations, so the same comment above 
> > applies. Maybe we need to parameterize these tests based on whether to use 
> > static or dynamic reservation?
> > 
> > It's also not clear to me why we need both tests, instead of just a 
> > single test.
> > 
> > E.g.
> > 
> > quota = R
> > add agent 1 with R resources, reserve them, decline forever
> > add agent 2 with R resources, they should not be allocated
> > 
> > revive
> > 
> > should only get agent 1 resources offered
> > trigger another allocation cycle (to make sure it's enforced across 
> > cycles)
> > agent 2 should not be offered to the role

Parameterized the test for both dynamic reservation and static reservation.

The first test ensures that quota limit does not get exceeded in the presence 
of unallocated-reservation. (expect no pending allocation)

The second test ensures that when a role’s reserved resources gets allocated 
and (by that time) if its quota still has not been fully satisfied, it can get 
unreserved resources to meet its quota. (expect allocation of unreserved 
resources).

I do not think you proposed test checks that. I clarified the comments and test 
name.


- Meng


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


On Dec. 5, 2017, 2:42 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64304/
> ---
> 
> (Updated Dec. 5, 2017, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
> Park.
> 
> 
> Bugs: MESOS-4527
> https://issues.apache.org/jira/browse/MESOS-4527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enforced quota limit in the presence of unallocated reservations.
> Also modify the allocation process such that the first stage allocation
> is solely for quota roles while the second stage is solely for
> non-quota roles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 4bc9fb6f787224114f1285937cf979ace46d8ea3 
>   src/tests/hierarchical_allocator_tests.cpp 
> 154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 
> 
> 
> Diff: https://reviews.apache.org/r/64304/diff/4/
> 
> 
> Testing
> ---
> 
> Introduced two dedicated tests.
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-04 Thread Benjamin Mahler 

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




src/master/allocator/mesos/hierarchical.cpp
Line 1492 (original), 1492 (patched)


Feel free to send a small patch for this since it's an indpendent change, I 
can get it committed quickly.



src/master/allocator/mesos/hierarchical.cpp
Line 2351 (original), 2370 (patched)


Whoops! Can you put this in the original patch?



src/tests/hierarchical_allocator_tests.cpp
Lines 1270-1271 (patched)


Reservations should be accounted towards the quota guarantee/limit even if 
they are currently unallocated.



src/tests/hierarchical_allocator_tests.cpp
Lines 1272 (patched)


After reading through the test, this is the dynamic reservation case only? 
Should we call this test `QuotaLimitWithDynamicReservation` and have another 
test for static reservations? Or do you want to test both cases in this test?



src/tests/hierarchical_allocator_tests.cpp
Lines 1310-1311 (patched)


This note seems to contradict the test? Is it a copy paste mistake?



src/tests/hierarchical_allocator_tests.cpp
Lines 1321 (patched)


As a general comment, be sure to start comments with a capital letter and 
end them with a period, please do a sweep across this patch for other 
instances. There are also some typos in the comments, so be sure to do a 
self-review before publishing a patch.



src/tests/hierarchical_allocator_tests.cpp
Lines 1330-1331 (patched)


This comment seems like a copy/paste mistake? I'm not sure you need a 
settle here.



src/tests/hierarchical_allocator_tests.cpp
Lines 1347-1348 (patched)


allocated to the role



src/tests/hierarchical_allocator_tests.cpp
Lines 1355 (patched)


What is a satisfied reservation? An allocated reservation?



src/tests/hierarchical_allocator_tests.cpp
Lines 1358 (patched)


I think you mean allocated resources + unallocated reservations?



src/tests/hierarchical_allocator_tests.cpp
Lines 1359 (patched)


the quota limit



src/tests/hierarchical_allocator_tests.cpp
Lines 1359-1361 (patched)


These last two sentences were a bit confusing, I think you can do without 
them if you update the addition above to reflect the lack of double counting?



src/tests/hierarchical_allocator_tests.cpp
Lines 1362 (patched)


Looks like this is already stated at the top, can you remove this?



src/tests/hierarchical_allocator_tests.cpp
Lines 1363 (patched)


This test is also just dynamic reservations, so the same comment above 
applies. Maybe we need to parameterize these tests based on whether to use 
static or dynamic reservation?

It's also not clear to me why we need both tests, instead of just a single 
test.

E.g.

quota = R
add agent 1 with R resources, reserve them, decline forever
add agent 2 with R resources, they should not be allocated

revive

should only get agent 1 resources offered
trigger another allocation cycle (to make sure it's enforced across cycles)
agent 2 should not be offered to the role



src/tests/hierarchical_allocator_tests.cpp
Lines 1401 (patched)


Ditto, this looks like a copy/paste mistake?



src/tests/hierarchical_allocator_tests.cpp
Lines 4737-4796 (original), 4902-4956 (patched)


Can you pull this bit out into a separate patch? We can get this committed 
independently.



src/master/allocator/mesos/hierarchical.cpp
Lines 1543-1556 (patched)


I think ultimately, the sorter needs to consider reserved resources as 
"allocated", do you have any TODO related to this or any tickets?

I guess the "unfairly satisfy guarantees" in 
[MESOS-4527](https://issues.apache.org/jira/browse/MESOS-4527) means that we 
couldn't close this ticket until we also track the reservations in the sorters, 
which will affect the code here (i.e. you might not need to track the 
reservations separately?)

Also, we would need a ticket for reservations breaking fairness

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-04 Thread Meng Zhu 

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

(Updated Dec. 4, 2017, 3:18 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
Park.


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


Repository: mesos


Description
---

Enforced quota limit in the presence of unallocated reservations.
Also modify the allocation process such that the first stage allocation
is solely for quota roles while the second stage is solely for
non-quota roles.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
20b908cc891f9f9be3045cd9f8be83a11d37ab78 
  src/tests/hierarchical_allocator_tests.cpp 
0309074bab180be122c9b0074981e6f69c97feee 


Diff: https://reviews.apache.org/r/64304/diff/3/

Changes: https://reviews.apache.org/r/64304/diff/2-3/


Testing
---

Introduced two dedicated tests.


Thanks,

Meng Zhu

Re: Review Request 64304: Enforced quota limit in the presence of unallocated reservations.

2017-12-04 Thread Meng Zhu 

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

(Updated Dec. 4, 2017, 1:48 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Summary (updated)
-

Enforced quota limit in the presence of unallocated reservations.


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


Repository: mesos


Description
---

Enforced quota limit in the presence of unallocated reservations.
Also modify the allocation process such that the first stage allocation
is solely for quota roles while the second stage is solely for
non-quota roles.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
715650ee9cb15aed1d1e58badf70fc09e26d13c1 
  src/tests/hierarchical_allocator_tests.cpp 
0309074bab180be122c9b0074981e6f69c97feee 


Diff: https://reviews.apache.org/r/64304/diff/2/

Changes: https://reviews.apache.org/r/64304/diff/1-2/


Testing (updated)
---

Introduced two dedicated tests.


Thanks,

Meng Zhu