mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.
Date Tue, 31 Jan 2017 05:59:08 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message