You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anindya Sinha <an...@apple.com> on 2017/01/09 22:37:08 UTC
Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444
Repository: mesos
Description
-------
We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.
Diffs
-----
src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
Diff: https://reviews.apache.org/r/55359/diff/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/#review164578
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
On Feb. 6, 2017, 9:02 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2017, 9:02 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
>
> Diff: https://reviews.apache.org/r/55359/diff/
>
>
> Testing
> -------
>
> Tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 5:02 a.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
Addressed comment and rebased.
Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444
Repository: mesos
Description
-------
We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363
Diff: https://reviews.apache.org/r/55359/diff/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/#review164101
-----------------------------------------------------------
LGTM. Just need to take another look after rebasing to ship it.
src/master/allocator/mesos/hierarchical.cpp (lines 682 - 683)
<https://reviews.apache.org/r/55359/#comment236137>
"contained" is ambiguous, I think it's better if we convey the idea of "at least one copy".
How about:
```
Ensure that offered resources contain at least one copy of the consumed shared resource (guaranteed by master validation).
```
src/master/allocator/mesos/hierarchical.cpp (lines 696 - 697)
<https://reviews.apache.org/r/55359/#comment236123>
So `additional` now encompasses multiple launch operations but it feels like it's still valuable to log the taskIds in this call for debugging?
src/master/allocator/mesos/hierarchical.cpp (lines 732 - 733)
<https://reviews.apache.org/r/55359/#comment236135>
So now we have two reasons:
```
The agent's total resources shouldn't contain:
1. The additionally allocated shared resources.
2. `AllocationInfo` as set in `updatedOfferedResources`.
```
Maybe spell them out like this?
- Jiang Yan Xu
On Feb. 3, 2017, 2:01 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2017, 2:01 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 5f540569043df9d2bb75416c8c36bb4dd7bd68a1
>
> Diff: https://reviews.apache.org/r/55359/diff/
>
>
> Testing
> -------
>
> Tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------
(Updated Feb. 3, 2017, 10:01 p.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444
Repository: mesos
Description
-------
We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp 5f540569043df9d2bb75416c8c36bb4dd7bd68a1
Diff: https://reviews.apache.org/r/55359/diff/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------
(Updated Feb. 1, 2017, 10:45 p.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
Rebased after batch allocation patch was merged.
Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444
Repository: mesos
Description
-------
We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp ffa07087f9ed8d28b99cc4cde7c739cfd7edb1e1
Diff: https://reviews.apache.org/r/55359/diff/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 5:59 a.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
Addressed review comments.
Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444
Repository: mesos
Description
-------
We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2
Diff: https://reviews.apache.org/r/55359/diff/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------
(Updated Jan. 26, 2017, 10:06 p.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
Rebased.
Bugs: MESOS-6444
https://issues.apache.org/jira/browse/MESOS-6444
Repository: mesos
Description
-------
We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.
Diffs (updated)
-----
src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
Diff: https://reviews.apache.org/r/55359/diff/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Anindya Sinha <an...@apple.com>.
> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > My feeling is that we were using too many variables for the same thing and after this review there are still more than we need.
> >
> >
> > Feels like there should just be these variables to capture the allocation change:
> >
> > `offeredResources`: passed down from the master, this is the "original".
> > `updatedOfferedResources`: the version with all the operations applied, either in `Resources::apply()`, or within this method for `LAUNCH`. This is the "updated", but more clear, since we are dealing with both total resources and allocations.
> >
> > Then we can update all the allocation related varaibles using these two.
> >
> > Then, next, we update the agent total resources. Why updating the totals in the method `updateAllocation()`? We can clarify this in the method (see my comments there).
As discussed, the flow in `updateAllocation()` is now as follows:
```
// (1) Make a copy of `offeredResources` (called `updatedOfferedResources`) to make changes
// to original offered resources based on offer operations.
// (2) Iterate thorugh each of the operations as follows:
// (2a) For all operations, `updatedOfferedResources = updatedOfferedResources.apply()`
// (2b) For LAUNCH, accumulate all of task resources for all tasks in `consumed`.
// (3) Determine additional copies of shared resources to be added to allocations based
// on `consumed` and `updatedOfferedResources`.
// (4) Update the allocations in the allocator and the sorters.
// (5) Update the total resources so that they are consistent with the updated allocation.
```
The main change is to process all operastions first and then update the allocations followed by total resources together (instead of doing it per operation).
> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 640-652
> > <https://reviews.apache.org/r/55359/diff/1/?file=1600298#file1600298line640>
> >
> > To unify these variables, perhaps don't use `_offeredResources`. To be more consistent with the naming pattern within this method (`something` and `updatedSomething`), perhaps:
> >
> > ```
> > Resources updatedOfferedResources = offeredResources;
> >
> > // (The objective of this loop is to obtain the correct `updatedOfferedResources`.)
> > foreach (const Offer::Operation& operation, operations) {
> > if (operation.type() == Offer::Operation::LAUNCH) {
> > // LAUNCH custom logic.
> > // (Here everything we do is essentially also udpate `updatedOfferedResources`.)
> > // ...
> > } else {
> > // (Here we invoke `Resources::apply()` to update `updatedOfferedResources`.)
> > Try<Resources> _updatedOfferedResources = updatedOfferedResources.apply(operation);
> > CHECK_SOME(_updatedOfferedResources);
> > updatedOfferedResources = _updatedOfferedResources.get();
> > }
> > }
> >
> > // At this point, we are outside the loop and we have "fully" processed the offered
> > // resources to obtain `updatedOfferedResources`.
> > // Now use it to update allocator and sorter variables.
> >
> > // ...
> > ```
> >
> > We don't need `updated` to capture the updated offered resources again.
Addressed based on the top comment.
> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 645
> > <https://reviews.apache.org/r/55359/diff/1/?file=1600298#file1600298line645>
> >
> > `original` is updated in every iteration?
> >
> > Isn't original offered resources the variable `offeredResources`?
The updated flow removes the need for `original` and `updated`.
> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 786-787
> > <https://reviews.apache.org/r/55359/diff/1/?file=1600298#file1600298line786>
> >
> > After all operations perhaps it's valuable to CHECK that we indeed didn't change the quantities. So for this we can save the framework's allocation at the very beginning and compare with it at the very end.
> >
> > ```
> > void HierarchicalAllocatorProcess::updateAllocation(
> > const FrameworkID& frameworkId,
> > const SlaveID& slaveId,
> > const Resources& offeredResources,
> > const vector<Offer::Operation>& operations)
> > {
> > // ... some initial checks.
> >
> > // (Save `frameworkAllocation`.)
> > const Resources frameworkAllocation =
> > frameworkSorter->allocation(frameworkId.value(), slaveId);
> >
> > // Do the work.
> >
> > // Check that the quantities are not changed.
> > const Resources updatedFrameworkAllocation = frameworkSorter->allocation(frameworkId.value(), slaveId);
> > CHECK_EQ(frameworkAllocation.createStrippedScalarQuantity(),
> > updatedFrameworkAllocation.createStrippedScalarQuantity());
> >
> > LOG(INFO) << "Updated allocation of framework " << frameworkId
> > << " on agent " << slaveId
> > << " from " << frameworkAllocation
> > << " to " << updatedFrameworkAllocation;
> > }
> > ```
We need to `flatten()` the `frameworkAllocation` and `updatedFrameworkAllocation` since the roles may change as a result of a `RESERVE` or `UNRESERVE` offer operations.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/#review163031
-----------------------------------------------------------
On Jan. 26, 2017, 10:06 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 10:06 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55359/diff/
>
>
> Testing
> -------
>
> Tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 55359: Consolidate update of allocations in
`updateAllocation()`.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/#review163031
-----------------------------------------------------------
My feeling is that we were using too many variables for the same thing and after this review there are still more than we need.
Feels like there should just be these variables to capture the allocation change:
`offeredResources`: passed down from the master, this is the "original".
`updatedOfferedResources`: the version with all the operations applied, either in `Resources::apply()`, or within this method for `LAUNCH`. This is the "updated", but more clear, since we are dealing with both total resources and allocations.
Then we can update all the allocation related varaibles using these two.
Then, next, we update the agent total resources. Why updating the totals in the method `updateAllocation()`? We can clarify this in the method (see my comments there).
src/master/allocator/mesos/hierarchical.cpp (lines 640 - 652)
<https://reviews.apache.org/r/55359/#comment234467>
To unify these variables, perhaps don't use `_offeredResources`. To be more consistent with the naming pattern within this method (`something` and `updatedSomething`), perhaps:
```
Resources updatedOfferedResources = offeredResources;
// (The objective of this loop is to obtain the correct `updatedOfferedResources`.)
foreach (const Offer::Operation& operation, operations) {
if (operation.type() == Offer::Operation::LAUNCH) {
// LAUNCH custom logic.
// (Here everything we do is essentially also udpate `updatedOfferedResources`.)
// ...
} else {
// (Here we invoke `Resources::apply()` to update `updatedOfferedResources`.)
Try<Resources> _updatedOfferedResources = updatedOfferedResources.apply(operation);
CHECK_SOME(_updatedOfferedResources);
updatedOfferedResources = _updatedOfferedResources.get();
}
}
// At this point, we are outside the loop and we have "fully" processed the offered
// resources to obtain `updatedOfferedResources`.
// Now use it to update allocator and sorter variables.
// ...
```
We don't need `updated` to capture the updated offered resources again.
src/master/allocator/mesos/hierarchical.cpp (line 645)
<https://reviews.apache.org/r/55359/#comment234431>
`original` is updated in every iteration?
Isn't original offered resources the variable `offeredResources`?
src/master/allocator/mesos/hierarchical.cpp (lines 697 - 699)
<https://reviews.apache.org/r/55359/#comment234469>
I see you've pulled this up so we don't have to get the same `frameworkAllocation` in every iteration.
However perhaps we can do it with just `offeredResources`:
```
foreach (const Resource& resource, additional) {
CHECK(offeredResources.contains(resource));
}
```
Indeed, the rule is more precisely "we allow tasks to request additional allocations if the (original) offered resources already contain the shared resources", right?
src/master/allocator/mesos/hierarchical.cpp (lines 704 - 712)
<https://reviews.apache.org/r/55359/#comment234470>
This doesn't seem to need to go inside the `foreach (const Offer::Operation& operation, operations)` loop? The launch operation is a no-op with LAUNCH. If outside the loop, we can just do
```
// ... update allocation-related variables first.
// Update the agent total resources so they are consistent with the updated allocation.
// We don't directly use `updatedOfferedResources` here because we don't count
// additionally allocated resources in the agent total resources. (See comments in `updateSlaveTotal`.)
Try<Resources> updatedTotal = slaves[slaveId].total.apply(operations);
CHECK_SOME(updatedTotal);
updateSlaveTotal(slaveId, updatedTotal.get());
```
src/master/allocator/mesos/hierarchical.cpp (lines 748 - 749)
<https://reviews.apache.org/r/55359/#comment234476>
After all operations perhaps it's valuable to CHECK that we indeed didn't change the quantities. So for this we can save the framework's allocation at the very beginning and compare with it at the very end.
```
void HierarchicalAllocatorProcess::updateAllocation(
const FrameworkID& frameworkId,
const SlaveID& slaveId,
const Resources& offeredResources,
const vector<Offer::Operation>& operations)
{
// ... some initial checks.
// (Save `frameworkAllocation`.)
const Resources frameworkAllocation =
frameworkSorter->allocation(frameworkId.value(), slaveId);
// Do the work.
// Check that the quantities are not changed.
const Resources updatedFrameworkAllocation = frameworkSorter->allocation(frameworkId.value(), slaveId);
CHECK_EQ(frameworkAllocation.createStrippedScalarQuantity(),
updatedFrameworkAllocation.createStrippedScalarQuantity());
LOG(INFO) << "Updated allocation of framework " << frameworkId
<< " on agent " << slaveId
<< " from " << frameworkAllocation
<< " to " << updatedFrameworkAllocation;
}
```
- Jiang Yan Xu
On Jan. 9, 2017, 2:37 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2017, 2:37 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
>
>
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca
>
> Diff: https://reviews.apache.org/r/55359/diff/
>
>
> Testing
> -------
>
> Tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>