You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2017/12/11 04:32:05 UTC
Review Request 64493: Added tests for quota enforcement with
unallocated reservations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64493/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Michael Park.
Repository: mesos
Description
-------
Added tests for quota enforcement with unallocated reservations.
Diffs
-----
src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec
Diff: https://reviews.apache.org/r/64493/diff/1/
Testing
-------
maek check
Thanks,
Meng Zhu
Re: Review Request 64493: Added tests for quota enforcement with
unallocated reservations.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1274-1280 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1274>
> >
> > Just curious, is it possible to template it by the `Resource::ReservationInfo::Type` enum?
> >
> > That might read a little more easily in the test output and code.
> >
> > In terms of naming, I think we tend to avoid naming these `SomeTestWithParam` (I only see the one named like this in this file). Naming this way means that if we add another parameterized test, we then need to add what it is parameterized by in the name:
> >
> > ```
> > // Before:
> > HierarchicalAllocatorTestWithParam ... bool (for quota)
> >
> > HierarchicalAllocatorTestWithQuotaParam ... bool (for quota)
> > HierarchicalAllocatorTestWithReservationParam ... bool or Resource::ReservationInfo::Type (for reservation)
> > ```
> >
> > Then I think it can get confusing because one's test interpretation of a "quota parameter" might be different than another's and you may then need to split the names further to distinguish?
> >
> > Maybe we could just name this fixture `HierarchicalAllocatorTestLimitWithReservations` and have the following?
> >
> > ```
> > TEST_P(HierarchicalAllocatorLimitWithReservationsTest, Unallocated)
> > TEST_P(HierarchicalAllocatorTestWithReservationParam, Allocated)
> > ```
Good suggestion. Done.
> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1314-1319 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1314>
> >
> > Is it possible just to inline this?
> >
> > ```
> > AWAIT_READY(allocator->updateAvailable(agent1.id(), {RESERVE(reserved)}));
> > ```
Done.
> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1323 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1323>
> >
> > Can you stringify QUOTA_ROLE instead of burning it in here?
> >
> > Alternatively, I think you can pass `Resources` in?
> >
> > ```
> > Resources resources = Resources::parse("cpus:1;mem:1024).get();
> >
> > resources.push_reservation(a static reservation)
> >
> > agent1 = createSlaveInfo(resources);
> > ```
> >
> > Hm.. this makes me wonder why we couldn't just do the following with the parameter?
> >
> > ```
> > Resource::ReservationInfo reservation;
> > reservation.set_type(GetParam());
> > reservation.set_role(QUOTA_ROLE);
> >
> > Resources resources = Resources::parse("cpus:1;mem:1024").get();
> > resources = resources.pushReservation(reservation);
> >
> > agent1 = createSlaveInfo(resources);
> >
> > ...
> > ```
> >
> > No need for an if condition?
Great suggestion! Done, much concise.
> On Dec. 11, 2017, 6:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp
> > Lines 1471-1473 (patched)
> > <https://reviews.apache.org/r/64493/diff/1/?file=1912389#file1912389line1471>
> >
> > Hm.. what is this testing? I was expecting to see that you couldn't go over your limit when you have [un]allocated resesrvations. This test being the allocated case, the above test being the unallocate case?
The first test checks you couldn't go over your limit when you have unallocated reservation.
The second test checks that you can properly allocated up to your limit when you have partial reservation.
The case of all allocated reservation should be no different from the case where there is no reservations in the sense that they are just allocated resources. There are already test for that.
The second test is needed to check against the tricky case of double counting of allocated-reservations. (Hence the comment above the second test).
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64493/#review193481
-----------------------------------------------------------
On Dec. 12, 2017, 1:45 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64493/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2017, 1:45 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for quota enforcement with unallocated reservations.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec
>
>
> Diff: https://reviews.apache.org/r/64493/diff/2/
>
>
> Testing
> -------
>
> maek check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 64493: Added tests for quota enforcement with
unallocated reservations.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64493/#review193481
-----------------------------------------------------------
Thanks for testing this! I left comments in the first test that also apply to the second test, so be sure to update both tests :)
src/tests/hierarchical_allocator_tests.cpp
Lines 1274-1280 (patched)
<https://reviews.apache.org/r/64493/#comment272039>
Just curious, is it possible to template it by the `Resource::ReservationInfo::Type` enum?
That might read a little more easily in the test output and code.
In terms of naming, I think we tend to avoid naming these `SomeTestWithParam` (I only see the one named like this in this file). Naming this way means that if we add another parameterized test, we then need to add what it is parameterized by in the name:
```
// Before:
HierarchicalAllocatorTestWithParam ... bool (for quota)
HierarchicalAllocatorTestWithQuotaParam ... bool (for quota)
HierarchicalAllocatorTestWithReservationParam ... bool or Resource::ReservationInfo::Type (for reservation)
```
Then I think it can get confusing because one's test interpretation of a "quota parameter" might be different than another's and you may then need to split the names further to distinguish?
Maybe we could just name this fixture `HierarchicalAllocatorTestLimitWithReservations` and have the following?
```
TEST_P(HierarchicalAllocatorLimitWithReservationsTest, Unallocated)
TEST_P(HierarchicalAllocatorTestWithReservationParam, Allocated)
```
src/tests/hierarchical_allocator_tests.cpp
Lines 1312 (patched)
<https://reviews.apache.org/r/64493/#comment272041>
What is "ops"? If it's the principal I think the test would be more readable with s/"ops"/"principal"/
src/tests/hierarchical_allocator_tests.cpp
Lines 1314-1319 (patched)
<https://reviews.apache.org/r/64493/#comment272043>
Is it possible just to inline this?
```
AWAIT_READY(allocator->updateAvailable(agent1.id(), {RESERVE(reserved)}));
```
src/tests/hierarchical_allocator_tests.cpp
Lines 1323 (patched)
<https://reviews.apache.org/r/64493/#comment272044>
Can you stringify QUOTA_ROLE instead of burning it in here?
Alternatively, I think you can pass `Resources` in?
```
Resources resources = Resources::parse("cpus:1;mem:1024).get();
resources.push_reservation(a static reservation)
agent1 = createSlaveInfo(resources);
```
Hm.. this makes me wonder why we couldn't just do the following with the parameter?
```
Resource::ReservationInfo reservation;
reservation.set_type(GetParam());
reservation.set_role(QUOTA_ROLE);
Resources resources = Resources::parse("cpus:1;mem:1024").get();
resources = resources.pushReservation(reservation);
agent1 = createSlaveInfo(resources);
...
```
No need for an if condition?
src/tests/hierarchical_allocator_tests.cpp
Lines 1339-1340 (patched)
<https://reviews.apache.org/r/64493/#comment272045>
Can you set quota first? Right now there's a race in this test between the framework being allocated resources vs the quota getting set.
src/tests/hierarchical_allocator_tests.cpp
Lines 1382-1386 (patched)
<https://reviews.apache.org/r/64493/#comment272046>
Shouldn't this description be nearly identical to the one above except for unallocated being allocated? It was a little surprising that this is phrased differently
src/tests/hierarchical_allocator_tests.cpp
Lines 1471-1473 (patched)
<https://reviews.apache.org/r/64493/#comment272047>
Hm.. what is this testing? I was expecting to see that you couldn't go over your limit when you have [un]allocated resesrvations. This test being the allocated case, the above test being the unallocate case?
- Benjamin Mahler
On Dec. 11, 2017, 4:32 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64493/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2017, 4:32 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for quota enforcement with unallocated reservations.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec
>
>
> Diff: https://reviews.apache.org/r/64493/diff/1/
>
>
> Testing
> -------
>
> maek check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 64493: Added tests for quota enforcement with
unallocated reservations.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64493/#review193728
-----------------------------------------------------------
Fix it, then Ship it!
Just some minor suggestions below, I will take care of these and get this committed shortly!
src/tests/hierarchical_allocator_tests.cpp
Lines 1274-1276 (patched)
<https://reviews.apache.org/r/64493/#comment272333>
This comment is stale?
src/tests/hierarchical_allocator_tests.cpp
Lines 1288-1289 (patched)
<https://reviews.apache.org/r/64493/#comment272335>
Fits on one line?
src/tests/hierarchical_allocator_tests.cpp
Lines 1355-1356 (patched)
<https://reviews.apache.org/r/64493/#comment272338>
Maybe a little more explanation here?
```
// Agent2's resources should not be allocated to `framework1` under
// `QUOTA_ROLE` because the reserved resources should be accounted
// towards the quota and allocating this agent will exceed the
// quota limit.
```
src/tests/hierarchical_allocator_tests.cpp
Lines 1367-1368 (patched)
<https://reviews.apache.org/r/64493/#comment272336>
Fits on one line?
src/tests/hierarchical_allocator_tests.cpp
Lines 1394 (patched)
<https://reviews.apache.org/r/64493/#comment272340>
This might be helpful to the reader?
```
// Set a quota for 2x agent resources.
```
src/tests/hierarchical_allocator_tests.cpp
Lines 1429-1431 (patched)
<https://reviews.apache.org/r/64493/#comment272341>
I thought we also wanted to then add a third agent and ensure it's not allocated to the role?
- Benjamin Mahler
On Dec. 13, 2017, 2:53 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64493/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2017, 2:53 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Michael Park.
>
>
> Bugs: MESOS-4527
> https://issues.apache.org/jira/browse/MESOS-4527
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for quota enforcement with unallocated reservations.
>
>
> Diffs
> -----
>
> src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec
>
>
> Diff: https://reviews.apache.org/r/64493/diff/3/
>
>
> Testing
> -------
>
> maek check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 64493: Added tests for quota enforcement with
unallocated reservations.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64493/
-----------------------------------------------------------
(Updated Dec. 12, 2017, 6:53 p.m.)
Review request for mesos, Benjamin Mahler and Michael Park.
Repository: mesos
Description
-------
Added tests for quota enforcement with unallocated reservations.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec
Diff: https://reviews.apache.org/r/64493/diff/3/
Changes: https://reviews.apache.org/r/64493/diff/2-3/
Testing
-------
maek check
Thanks,
Meng Zhu
Re: Review Request 64493: Added tests for quota enforcement with
unallocated reservations.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64493/
-----------------------------------------------------------
(Updated Dec. 12, 2017, 1:45 p.m.)
Review request for mesos, Benjamin Mahler and Michael Park.
Changes
-------
Thanks! Patch updated based on the comments.
Repository: mesos
Description
-------
Added tests for quota enforcement with unallocated reservations.
Diffs (updated)
-----
src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec
Diff: https://reviews.apache.org/r/64493/diff/2/
Changes: https://reviews.apache.org/r/64493/diff/1-2/
Testing
-------
maek check
Thanks,
Meng Zhu