Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote: > > src/master/allocator/sorter/drf/sorter.hpp > > Lines 347 (patched) > > <https://reviews.apache.org/r/57935/diff/3/?file=1716055#file1716055line347> > > > > Why is `flatten` necessary here? We are already comparing two scalar > > quantities, so is the flatten required? > > Anindya Sinha wrote: > In the case of a `RESERVE` or `UNRESERVE` operation, the `newAllocation` > and `oldAllocation` varies in distribution of resources across roles. > > eg. For `RESERVE`, oldAllocation = `disk(*):100`, and newAllocation = > `disk(*):90; disk(role1):10`. > > So, `createStrippedScalarQuantities()` on these `Resources` shall drop > the `ReservationInfo` but the roles will remain intact. > Without `flatten()`, `oldAllocationQuantity` and `newAllocationQuantity` > will not be the same (due to different roles) and hence `dirty` shall be set. > But I do not think we need to recalculate shares since the total resources > have not changed (only the distribution has changed in terms of roles). That > is the reason for the comparison having the `flatten()`. > > Looking at when `dirty` is true: We update `totals` (which is hashmap > `<ResourceName, ScalarValue>` in `update()`. And when `calculateShare()` is > called, we calculate share based on totals in the Sorter and individual Node. > So I think we should be good to have the `flatten()` in this comparison. > > Let me know if this does not sound ok. > > Neil Conway wrote: > Sounds reasonable. So, can this be merged now? - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57935/#review176687 --- On June 2, 2017, 4:59 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57935/ > --- > > (Updated June 2, 2017, 4:59 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7138 > https://issues.apache.org/jira/browse/MESOS-7138 > > > Repository: mesos > > > Description > --- > > In `DRFSorter::update`, we should set the `dirty` flag only when the > total scalar quantities have changed in any of the clients in the > hierarchy, and not always. > > > Diffs > - > > src/master/allocator/sorter/drf/sorter.hpp > 77e52dec735d276389643f7f356cd763b2f785e9 > src/master/allocator/sorter/drf/sorter.cpp > ecc5586737b6b447c5a1cf1a37037832bcbacd69 > > > Diff: https://reviews.apache.org/r/57935/diff/4/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 449-450 (original), 479-481 (patched) > > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line479> > > > > Not your bug, but it seems wrong to be activating roles here if the > > framework was deactivated. Would be good to sync with mpark on this > > original change. It seems to me that if the framework is deactivated via > > deactivateFramework, we wouldn't want to be activating things here in > > updateFrameowrk. > > Anindya Sinha wrote: > Reached out to @mpark. I kept it as-is till he has a chance to look into > it. > > Anindya Sinha wrote: > So indeed, we need to activate roles if the framework is not deactivated. > Can we push that fix in a separate JIRA/RR once this one is merged? Added a TODO for now indicating that roles should be activated in `updateFramework()` only if the framework is active. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/#review176630 ------- On June 5, 2017, 10 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57817/ > --- > > (Updated June 5, 2017, 10 p.m.) > > > Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. > > > Bugs: MESOS-7015 > https://issues.apache.org/jira/browse/MESOS-7015 > > > Repository: mesos > > > Description > --- > > If the `SUBSCRIBE` indicates a subset of roles to be suppressed during > framework (re)registration, the allocator does not offer resources for > those roles to such frameworks. Note that this functionality is added > for `v1::SUBSCRIBE` only (and not for scheduler driver API). > > In addition, the master validates the suppressed roles to be a subset > of all the roles being (re)registered by the framework. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 > src/master/allocator/mesos/allocator.hpp > b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 > src/master/allocator/mesos/hierarchical.hpp > 5e7c3068061012c51d4b9220dedf476408016a12 > src/master/allocator/mesos/hierarchical.cpp > 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 > src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe > src/tests/hierarchical_allocator_tests.cpp > eb2b647d3247a85e8b9b82e5589232c74ad8570f > src/tests/master_allocator_tests.cpp > 0082045ccaebe015725c5e178aa89dac32b1c50c > src/tests/persistent_volume_endpoints_tests.cpp > 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c > src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 > src/tests/resource_offers_tests.cpp > 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 > src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 > > > Diff: https://reviews.apache.org/r/57817/diff/15/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated June 5, 2017, 10:01 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/15/ Changes: https://reviews.apache.org/r/57818/diff/14-15/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated June 5, 2017, 10 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Added a TODO indicating that roles should be activated in `updateFramework()` only if the framework is active. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the `SUBSCRIBE` indicates a subset of roles to be suppressed during framework (re)registration, the allocator does not offer resources for those roles to such frameworks. Note that this functionality is added for `v1::SUBSCRIBE` only (and not for scheduler driver API). In addition, the master validates the suppressed roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - include/mesos/allocator/allocator.hpp b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 src/master/allocator/mesos/allocator.hpp b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 src/master/allocator/mesos/hierarchical.hpp 5e7c3068061012c51d4b9220dedf476408016a12 src/master/allocator/mesos/hierarchical.cpp 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe src/tests/hierarchical_allocator_tests.cpp eb2b647d3247a85e8b9b82e5589232c74ad8570f src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c src/tests/persistent_volume_endpoints_tests.cpp 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 Diff: https://reviews.apache.org/r/57817/diff/15/ Changes: https://reviews.apache.org/r/57817/diff/14-15/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated June 2, 2017, 9:29 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- rebased. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/14/ Changes: https://reviews.apache.org/r/57818/diff/13-14/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
- Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/#review176833 --- On June 2, 2017, 9:29 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57817/ > --- > > (Updated June 2, 2017, 9:29 p.m.) > > > Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. > > > Bugs: MESOS-7015 > https://issues.apache.org/jira/browse/MESOS-7015 > > > Repository: mesos > > > Description > --- > > If the `SUBSCRIBE` indicates a subset of roles to be suppressed during > framework (re)registration, the allocator does not offer resources for > those roles to such frameworks. Note that this functionality is added > for `v1::SUBSCRIBE` only (and not for scheduler driver API). > > In addition, the master validates the suppressed roles to be a subset > of all the roles being (re)registered by the framework. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 > src/master/allocator/mesos/allocator.hpp > b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 > src/master/allocator/mesos/hierarchical.hpp > 5e7c3068061012c51d4b9220dedf476408016a12 > src/master/allocator/mesos/hierarchical.cpp > 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 > src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe > src/tests/hierarchical_allocator_tests.cpp > eb2b647d3247a85e8b9b82e5589232c74ad8570f > src/tests/master_allocator_tests.cpp > 0082045ccaebe015725c5e178aa89dac32b1c50c > src/tests/persistent_volume_endpoints_tests.cpp > 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c > src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 > src/tests/resource_offers_tests.cpp > 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 > src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 > > > Diff: https://reviews.apache.org/r/57817/diff/14/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
> On June 2, 2017, 7:51 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 463 (patched)
> > <https://reviews.apache.org/r/57817/diff/12-13/?file=1736091#file1736091line474>
> >
> > what about new roles that are also suppressed?
>
> Anindya Sinha wrote:
> We do not activate new roles that are suppressed. We only activate roles
> if they are not suppressed. That is handled here:
>
> ```
> if (!suppressedRoles.count(role)) {
> frameworkSorters.at(role)->activate(frameworkId.value());
> }
> ```
Consolidated this within `rolesToActivate`.
- Anindya
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57817/#review176833
-------
On June 2, 2017, 9:29 p.m., Anindya Sinha wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
>
> (Updated June 2, 2017, 9:29 p.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> ---
>
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
>
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
>
>
> Diffs
> -
>
> include/mesos/allocator/allocator.hpp
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4
> src/master/allocator/mesos/allocator.hpp
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733
> src/master/allocator/mesos/hierarchical.hpp
> 5e7c3068061012c51d4b9220dedf476408016a12
> src/master/allocator/mesos/hierarchical.cpp
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270
> src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732
> src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9
> src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe
> src/tests/hierarchical_allocator_tests.cpp
> eb2b647d3247a85e8b9b82e5589232c74ad8570f
> src/tests/master_allocator_tests.cpp
> 0082045ccaebe015725c5e178aa89dac32b1c50c
> src/tests/persistent_volume_endpoints_tests.cpp
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c
> src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14
> src/tests/resource_offers_tests.cpp
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8
> src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866
>
>
> Diff: https://reviews.apache.org/r/57817/diff/14/
>
>
> Testing
> ---
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 449-450 (original), 479-481 (patched) > > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line479> > > > > Not your bug, but it seems wrong to be activating roles here if the > > framework was deactivated. Would be good to sync with mpark on this > > original change. It seems to me that if the framework is deactivated via > > deactivateFramework, we wouldn't want to be activating things here in > > updateFrameowrk. > > Anindya Sinha wrote: > Reached out to @mpark. I kept it as-is till he has a chance to look into > it. So indeed, we need to activate roles if the framework is not deactivated. Can we push that fix in a separate JIRA/RR once this one is merged? - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/#review176630 ----------- On June 2, 2017, 5:49 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57817/ > --- > > (Updated June 2, 2017, 5:49 p.m.) > > > Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. > > > Bugs: MESOS-7015 > https://issues.apache.org/jira/browse/MESOS-7015 > > > Repository: mesos > > > Description > --- > > If the `SUBSCRIBE` indicates a subset of roles to be suppressed during > framework (re)registration, the allocator does not offer resources for > those roles to such frameworks. Note that this functionality is added > for `v1::SUBSCRIBE` only (and not for scheduler driver API). > > In addition, the master validates the suppressed roles to be a subset > of all the roles being (re)registered by the framework. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 > src/master/allocator/mesos/allocator.hpp > b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 > src/master/allocator/mesos/hierarchical.hpp > 5e7c3068061012c51d4b9220dedf476408016a12 > src/master/allocator/mesos/hierarchical.cpp > 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 > src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe > src/tests/hierarchical_allocator_tests.cpp > eb2b647d3247a85e8b9b82e5589232c74ad8570f > src/tests/master_allocator_tests.cpp > 0082045ccaebe015725c5e178aa89dac32b1c50c > src/tests/persistent_volume_endpoints_tests.cpp > 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c > src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 > src/tests/resource_offers_tests.cpp > 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 > src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 > > > Diff: https://reviews.apache.org/r/57817/diff/13/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
> On June 2, 2017, 7:51 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 430-431 (patched)
> > <https://reviews.apache.org/r/57817/diff/12-13/?file=1736091#file1736091line438>
> >
> > what about roles that are suppressed in old state and new state?
If a specific role was suppressed before, and is also supressed now, then that
role is already deactivated. I do not think we need to deactivate them again.
Similarly, if a role is non-suppressed before and after, we do not need to
activate them since they are already activated. Or am I missing something?
> On June 2, 2017, 7:51 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 463 (patched)
> > <https://reviews.apache.org/r/57817/diff/12-13/?file=1736091#file1736091line474>
> >
> > what about new roles that are also suppressed?
We do not activate new roles that are suppressed. We only activate roles if
they are not suppressed. That is handled here:
```
if (!suppressedRoles.count(role)) {
frameworkSorters.at(role)->activate(frameworkId.value());
}
```
- Anindya
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57817/#review176833
-------
On June 2, 2017, 5:49 p.m., Anindya Sinha wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
>
> (Updated June 2, 2017, 5:49 p.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> ---
>
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
>
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
>
>
> Diffs
> -
>
> include/mesos/allocator/allocator.hpp
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4
> src/master/allocator/mesos/allocator.hpp
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733
> src/master/allocator/mesos/hierarchical.hpp
> 5e7c3068061012c51d4b9220dedf476408016a12
> src/master/allocator/mesos/hierarchical.cpp
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270
> src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732
> src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9
> src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe
> src/tests/hierarchical_allocator_tests.cpp
> eb2b647d3247a85e8b9b82e5589232c74ad8570f
> src/tests/master_allocator_tests.cpp
> 0082045ccaebe015725c5e178aa89dac32b1c50c
> src/tests/persistent_volume_endpoints_tests.cpp
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c
> src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14
> src/tests/resource_offers_tests.cpp
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8
> src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866
>
>
> Diff: https://reviews.apache.org/r/57817/diff/13/
>
>
> Testing
> ---
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated June 2, 2017, 5:50 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/13/ Changes: https://reviews.apache.org/r/57818/diff/12-13/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote: > > src/common/protobuf_utils.hpp > > Lines 359-361 (patched) > > <https://reviews.apache.org/r/57817/diff/11/?file=1735873#file1735873line359> > > > > This looks to be just a generic conversion from repeated ptr field to > > set, not something role specific? > > > > This means we should either inline the set construction (using the > > iterator approach), or expose a generic conversion (would we want to return > > an Error if there are duplicates?) Agreed. Removed the function and calling it inline instead. > On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 391-395 (patched) > > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line391> > > > > Is this stale? Seems to refer to "deactivated" roles whereas the > > variables are "suppressed" roles Since `deactivate()` is idempotent (as in next comment), there is no need to add the `if (!framework.suppressedRoles.count(role))` condition, and hence I do not think the comment is needed here. > On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 449-450 (original), 479-481 (patched) > > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line479> > > > > Not your bug, but it seems wrong to be activating roles here if the > > framework was deactivated. Would be good to sync with mpark on this > > original change. It seems to me that if the framework is deactivated via > > deactivateFramework, we wouldn't want to be activating things here in > > updateFrameowrk. Reached out to @mpark. I kept it as-is till he has a chance to look into it. > On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 485 (patched) > > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line485> > > > > Seems there is a bug here? What about existing roles (not covered by > > "removed roles" or "added roles" loops above) that have now become > > suppressed? For these, we want to deactivate them. Yes, that is correct. So, the roles that need to be deactivated are the roles which have been removed, as well as the roles which have moved from non-suppressed mode to suppressed mode. Similarly, the roles that need to be activated are the roles that have been added, as well as the roles which have moved from suppressed to non-suppressed mode. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/#review176630 --- On June 1, 2017, 12:38 a.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57817/ > --- > > (Updated June 1, 2017, 12:38 a.m.) > > > Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. > > > Bugs: MESOS-7015 > https://issues.apache.org/jira/browse/MESOS-7015 > > > Repository: mesos > > > Description > --- > > If the `SUBSCRIBE` indicates a subset of roles to be suppressed during > framework (re)registration, the allocator does not offer resources for > those roles to such frameworks. Note that this functionality is added > for `v1::SUBSCRIBE` only (and not for scheduler driver API). > > In addition, the master validates the suppressed roles to be a subset > of all the roles being (re)registered by the framework. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 > src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 > src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d > src/master/allocator/mesos/allocator.hpp > b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 > src/master/allocator/mesos/hierarchical.hpp > 5e7c3068061012c51d4b9220dedf476408016a12 > src/master/allocator/mesos/hierarchical.cpp > 6679d47ea53bbcd68d375654edf6e85890e5772a > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 > src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe > src/tests/hierarchical_allocator_tests.cpp > eb2b647d3247a85e8b9b82e5589232c74ad8570f > src/tests/master_allocat
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated June 2, 2017, 5:49 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Addressed comments. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the `SUBSCRIBE` indicates a subset of roles to be suppressed during framework (re)registration, the allocator does not offer resources for those roles to such frameworks. Note that this functionality is added for `v1::SUBSCRIBE` only (and not for scheduler driver API). In addition, the master validates the suppressed roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - include/mesos/allocator/allocator.hpp b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 src/master/allocator/mesos/allocator.hpp b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 src/master/allocator/mesos/hierarchical.hpp 5e7c3068061012c51d4b9220dedf476408016a12 src/master/allocator/mesos/hierarchical.cpp 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe src/tests/hierarchical_allocator_tests.cpp eb2b647d3247a85e8b9b82e5589232c74ad8570f src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c src/tests/persistent_volume_endpoints_tests.cpp 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 Diff: https://reviews.apache.org/r/57817/diff/13/ Changes: https://reviews.apache.org/r/57817/diff/12-13/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote: > > src/master/allocator/sorter/drf/sorter.hpp > > Lines 347 (patched) > > <https://reviews.apache.org/r/57935/diff/3/?file=1716055#file1716055line347> > > > > Why is `flatten` necessary here? We are already comparing two scalar > > quantities, so is the flatten required? In the case of a `RESERVE` or `UNRESERVE` operation, the `newAllocation` and `oldAllocation` varies in distribution of resources across roles. eg. For `RESERVE`, oldAllocation = `disk(*):100`, and newAllocation = `disk(*):90; disk(role1):10`. So, `createStrippedScalarQuantities()` on these `Resources` shall drop the `ReservationInfo` but the roles will remain intact. Without `flatten()`, `oldAllocationQuantity` and `newAllocationQuantity` will not be the same (due to different roles) and hence `dirty` shall be set. But I do not think we need to recalculate shares since the total resources have not changed (only the distribution has changed in terms of roles). That is the reason for the comparison having the `flatten()`. Looking at when `dirty` is true: We update `totals` (which is hashmap `<ResourceName, ScalarValue>` in `update()`. And when `calculateShare()` is called, we calculate share based on totals in the Sorter and individual Node. So I think we should be good to have the `flatten()` in this comparison. Let me know if this does not sound ok. > On June 2, 2017, 4:26 p.m., Neil Conway wrote: > > src/master/allocator/sorter/drf/sorter.cpp > > Lines 307 (patched) > > <https://reviews.apache.org/r/57935/diff/3/?file=1716056#file1716056line312> > > > > Dropping this as there is no comment here. > On June 2, 2017, 4:26 p.m., Neil Conway wrote: > > src/master/allocator/sorter/drf/sorter.cpp > > Lines 307 (patched) > > <https://reviews.apache.org/r/57935/diff/3/?file=1716056#file1716056line312> > > > > Dropping this as there is no comment here. > On June 2, 2017, 4:26 p.m., Neil Conway wrote: > > src/master/allocator/sorter/drf/sorter.cpp > > Lines 307 (patched) > > <https://reviews.apache.org/r/57935/diff/3/?file=1716056#file1716056line312> > > > > The return value of `update` depends only on the parameters > > `oldAllocation` and `newAllocation`, right? In which case, the way this is > > written seems confusing to me (e.g., it implies it would be possible for > > `dirty` to remain false when we start at a leaf node, but then for `dirty` > > to be true for an ancestor node). > > > > An improvement would be to `CHECK` that `update()` returns the same > > value for all the nodes in the path from leaf -> root. > > > > We could alternatively check whether the total allocation has changed > > outside `update` and only update `dirty` once. We could conceivably compute > > `oldAllocationQuantity` and `newAllocationQuantity` in `allocated` and pass > > them into `update` (for efficiency), but that is a bit ugly. > > > > Let me know what you think. SGTM. I modified based on the 1st recommendation. I do a `CHECK` to ensure that `update()` returns the same value for all nodes in the path from leaf -> root. - Anindya ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57935/#review176687 --- On June 2, 2017, 4:59 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57935/ > --- > > (Updated June 2, 2017, 4:59 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7138 > https://issues.apache.org/jira/browse/MESOS-7138 > > > Repository: mesos > > > Description > --- > > In `DRFSorter::update`, we should set the `dirty` flag only when the > total scalar quantities have changed in any of the clients in the > hierarchy, and not always. > > > Diffs > - > > src/master/allocator/sorter/drf/sorter.hpp > 77e52dec735d276389643f7f356cd763b2f785e9 > src/master/allocator/sorter/drf/sorter.cpp > ecc5586737b6b447c5a1cf1a37037832bcbacd69 > > > Diff: https://reviews.apache.org/r/57935/diff/4/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57935/ --- (Updated June 2, 2017, 4:59 p.m.) Review request for mesos and Neil Conway. Changes --- Addressed review comments. Bugs: MESOS-7138 https://issues.apache.org/jira/browse/MESOS-7138 Repository: mesos Description (updated) --- In `DRFSorter::update`, we should set the `dirty` flag only when the total scalar quantities have changed in any of the clients in the hierarchy, and not always. Diffs (updated) - src/master/allocator/sorter/drf/sorter.hpp 77e52dec735d276389643f7f356cd763b2f785e9 src/master/allocator/sorter/drf/sorter.cpp ecc5586737b6b447c5a1cf1a37037832bcbacd69 Diff: https://reviews.apache.org/r/57935/diff/4/ Changes: https://reviews.apache.org/r/57935/diff/3-4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52071/ --- (Updated June 1, 2017, 7:16 p.m.) Review request for mesos and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-6062 https://issues.apache.org/jira/browse/MESOS-6062 Repository: mesos Description --- When a resource with no size is specified in `--resources` startup flag of mesos agent, the size is auto detected by the agent. This is enabled for all known resource types (except gpus) only when the resources are specified in JSON format. For disk resources, this is done for both root disks as well as MOUNT and PATH disks. Diffs (updated) - docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa Diff: https://reviews.apache.org/r/52071/diff/13/ Changes: https://reviews.apache.org/r/52071/diff/12-13/ Testing --- Documentation change only. All tests passed. Thanks, Anindya Sinha
Re: Review Request 51879: Autodetect value of resource when not specified in static resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51879/ --- (Updated June 1, 2017, 7:16 p.m.) Review request for mesos and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-6062 https://issues.apache.org/jira/browse/MESOS-6062 Repository: mesos Description --- When static resources indicate resources with a positive size, we use that for the resources on the agent. However, --resources can include resources with no size, which indicates that mesos agent determine the size of those resources from the agent and uses that information. Note that auto-detection of resources is allowed for all known resource types represented in JSON format only. Auto-detection is not done when the resources are represented in text format. With this change, JSON representation for disk resources that do not specify any value would not result in an error, but those resources will not be accounted for until a valid size is determined for such resources. A scalar value of -1 still results in an invalid resource. Diffs (updated) - include/mesos/resources.hpp 5d16a80a0d4fc386ad62c7dffe96b68d11ebddb1 include/mesos/v1/resources.hpp e1c35777227dd709f90b934859eb946daffbcd97 src/common/resources.cpp 97c00b204b17fa6b47face6873eea9751978f52c src/slave/containerizer/containerizer.cpp 15ae0b33a5ea8f73a9a84081d9c310c1d59c3141 src/tests/persistent_volume_endpoints_tests.cpp 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c src/tests/reservation_endpoints_tests.cpp 505c5421e95378177a7a09f462e5625ffa75cd37 src/v1/resources.cpp 559b048b65cce2da2e75abd740fc857f35280143 Diff: https://reviews.apache.org/r/51879/diff/14/ Changes: https://reviews.apache.org/r/51879/diff/13-14/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 51879: Autodetect value of resource when not specified in static resources.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731951#file1731951line175>
> >
> > Sorry I know you had https://reviews.apache.org/r/52002 which was
> > originally intended for all disk types and later evolved into a single
> > `bool isRootDisk()` but given this is the only use of the helper can we
> > just simply do the following here?
> >
> > ```
> > bool isRootDisk = !resource.has_disk();
> > ```
> >
> > Now I think a proper follow up would be to actually to give the root
> > disk a type which is set by default.
> >
> > e.g.,
> >
> > ```
> > message Source {
> > enum Type {
> > UNKNOWN = 0;
> > PATH = 1;
> > MOUNT = 2;
> > ROOT = 3;
> > }
> > ...
> > optional Source source = 3 [default = ROOT];
> > }
> > ```
> >
> > Which would have made things much simpler in many places.
Dropped the RR https://reviews.apache.org/r/52002, and moved the other change
in that RR to https://reviews.apache.org/r/59724/.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
> > Lines 182-190 (original), 192-196 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731952#file1731952line192>
> >
> > So if we are not auot-detecting for "gpus:" (which I think is fine for
> > now as an incremental step), are the changes in
> > "src/slave/containerizer/mesos/isolators/gpu/allocator.cpp" just a
> > follow-up for https://reviews.apache.org/r/55761/?
> >
> > Then can we separate it out to simplify this patch?
Moved this part to a separate RR (https://reviews.apache.org/r/59723/) in this
chain.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_endpoints_tests.cpp
> > Lines 2180-2195 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731953#file1731953line2196>
> >
> > Are these just reordering? Why do it?
> >
> > Same for other changes in this file?
In `Try Containerizer::resources(const Flags& flags)`, we use a
`const hashset predefined = {"cpus", "mem", "disk", "ports", "gpus"}`
(since we call a `contains()`). As a result, the order in which
`vector result` and hence the `resources` returned from this function
varies in order from before.
The test uses `JSON::Value` which should be enhanced to do a comparison using
`Resources` which we can do later. For now, I changed the order of the
`JSON::Value` in the test to satisfy the condition.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 1784-1791 (original)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731954#file1731954line1784>
> >
> > Ditto. Why reordering?
Same reason as in previous comment.
- Anindya
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review176411
---
On May 24, 2017, 7:56 p.m., Anindya Sinha wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
>
> (Updated May 24, 2017, 7:56 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
>
>
> Repository: mesos
>
>
> Description
> ---
>
> When static resources indicate resources with a positive size, we use
> that for the resources on the agent. However, --resources can include
> resources with no size, which indicates that mesos agent determine the
> size of those resources from the agent and uses that information. Note
> that auto-detection of resources is allowed for all known resource
> types represented in JSON format only. Auto-detection is not done when
> the resources are represented in text format.
>
> With this change, JSON representation for disk resources that do not
> specify any value would not result in an error, but those resources
> will not be accounted for until a valid size is determined for su
Review Request 59723: Fixed matching for `gpus` resource.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59723/ --- Review request for mesos and Jiang Yan Xu. Repository: mesos Description --- This is a followup for RR 55761 (for `gpus` resource). Diffs - src/slave/containerizer/mesos/isolators/gpu/allocator.cpp c288ad634b856702483b9751f41445308babd0c9 Diff: https://reviews.apache.org/r/59723/diff/1/ Testing --- Tests passed. Thanks, Anindya Sinha
Review Request 59724: Check for `disk` resource in determination for persistent volume.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59724/ --- Review request for mesos and Jiang Yan Xu. Repository: mesos Description --- Check for `disk` resource in determination for persistent volume. Diffs - src/common/resources.cpp 97c00b204b17fa6b47face6873eea9751978f52c src/v1/resources.cpp 559b048b65cce2da2e75abd740fc857f35280143 Diff: https://reviews.apache.org/r/59724/diff/1/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated June 1, 2017, 12:38 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Fixed merge conflict. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the `SUBSCRIBE` indicates a subset of roles to be suppressed during framework (re)registration, the allocator does not offer resources for those roles to such frameworks. Note that this functionality is added for `v1::SUBSCRIBE` only (and not for scheduler driver API). In addition, the master validates the suppressed roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - include/mesos/allocator/allocator.hpp b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d src/master/allocator/mesos/allocator.hpp b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 src/master/allocator/mesos/hierarchical.hpp 5e7c3068061012c51d4b9220dedf476408016a12 src/master/allocator/mesos/hierarchical.cpp 6679d47ea53bbcd68d375654edf6e85890e5772a src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe src/tests/hierarchical_allocator_tests.cpp eb2b647d3247a85e8b9b82e5589232c74ad8570f src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c src/tests/persistent_volume_endpoints_tests.cpp 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 src/tests/slave_recovery_tests.cpp e140f4d902a43b8fb2390541a357a217896b6228 Diff: https://reviews.apache.org/r/57817/diff/12/ Changes: https://reviews.apache.org/r/57817/diff/11-12/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated June 1, 2017, 12:38 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Fixed merge conflict. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/12/ Changes: https://reviews.apache.org/r/57818/diff/11-12/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated May 31, 2017, 2:53 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- rebase Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/11/ Changes: https://reviews.apache.org/r/57818/diff/10-11/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated May 31, 2017, 2:52 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Fix compilation for tests. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the `SUBSCRIBE` indicates a subset of roles to be suppressed during framework (re)registration, the allocator does not offer resources for those roles to such frameworks. Note that this functionality is added for `v1::SUBSCRIBE` only (and not for scheduler driver API). In addition, the master validates the suppressed roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 82fea40ad09c95f0096934c4210a39a6f0638ed9 src/master/allocator/mesos/hierarchical.cpp ca368bb5ef0dcfe83672004d58a4de6f85d6b4bc src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp eb2b647d3247a85e8b9b82e5589232c74ad8570f src/tests/master_allocator_tests.cpp b9e04226a1688499847bc25c7c220c0b82ba8db7 src/tests/persistent_volume_endpoints_tests.cpp 21136b29d724959527b889f52611baee0668 src/tests/reservation_tests.cpp 47ccf7f6f6ee48236f6794ce3084ce4f425c93c5 src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c src/tests/slave_recovery_tests.cpp df0c5c88786190be06df7ef3602834aa8985cefe Diff: https://reviews.apache.org/r/57817/diff/11/ Changes: https://reviews.apache.org/r/57817/diff/10-11/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated May 31, 2017, 8 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/10/ Changes: https://reviews.apache.org/r/57818/diff/9-10/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
> On May 23, 2017, 9:01 p.m., Vinod Kone wrote: > > src/master/allocator/mesos/hierarchical.hpp > > Line 309 (original), 309 (patched) > > <https://reviews.apache.org/r/57817/diff/4-8/?file=1689440#file1689440line312> > > > > I don't quite follow why you need to have this variable here? Looks > > like the sorter can be the source of truth of whether a role is activated > > or not? > > Anindya Sinha wrote: > `FrameworkInfo::roles` indicates all the roles the framework registers > as, and `Call::Subscribe::suppressed_roles` is a subset of roles for which > the master would not offer resources. The effective `suppressed_roles` can > change (after the framework is (re)registered) with `SUPPRESS` and `REVIVE` > calls, which means the roles for which offers are sent to frameworks can > change based on roles contained in these messages. > > Now, the existing `activateFramework()` (and `deactivateFramework()`) > activates (and deactivates) all roles of the framework. I think that should > not be the case any longer. We should activate (and deactivate) roles of the > framework which are not contained in `suppressed_roles` at any given point of > time. To achieve this, we keep track of > `HierarchicalAllocatorProcess::Framework::Framework::suppressed_roles` to > ensure we keep track of `suppressed_roles` based on `SUPPRESS` and `REVIVE` > calls during the life cycle of the framework. This would help us determine if > we activate (or deactivate) for a specific role based on whether the role is > a `suppressed_roles`. > > Vinod Kone wrote: > I see. Makes sense. Can you add a comment to > activateFramework/deactivateFramework about the new semantics for posterity? Added a comment in `activateFramework()` and `deactivateFramework()`. > On May 23, 2017, 9:01 p.m., Vinod Kone wrote: > > src/master/master.cpp > > Lines 2804-2808 (patched) > > <https://reviews.apache.org/r/57817/diff/4-8/?file=1689443#file1689443line2809> > > > > why do you need this check? is the below one not sufficient? > > Anindya Sinha wrote: > Just an optimization to not loop through the roles in `suppressedRoles` > incase the sizes of `frameworkRoles` and `suppressedRoles` differ. But I > think it should be fine to loop through since we do not anticipate the number > of roles to be too many. > > Vinod Kone wrote: > looks like you fixed it, not sure why you marked it as "dropped". Sorry, my mistake. Updated it. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/#review175846 --- On May 23, 2017, 10:19 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57817/ > --- > > (Updated May 23, 2017, 10:19 p.m.) > > > Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. > > > Bugs: MESOS-7015 > https://issues.apache.org/jira/browse/MESOS-7015 > > > Repository: mesos > > > Description > --- > > If the `SUBSCRIBE` indicates a subset of roles to be suppressed during > framework (re)registration, the allocator does not offer resources for > those roles to such frameworks. Note that this functionality is added > for `v1::SUBSCRIBE` only (and not for scheduler driver API). > > In addition, the master validates the suppressed roles to be a subset > of all the roles being (re)registered by the framework. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 > src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 > src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d > src/master/allocator/mesos/allocator.hpp > 119b461136123229f85ed3c3cfd41137974a6b9b > src/master/allocator/mesos/hierarchical.hpp > 123f97cf495bff0f822838e09df0d88818f04da6 > src/master/allocator/mesos/hierarchical.cpp > b75ed9a20a9a42f958cebbacd91e5e15b0d21394 > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/master/master.cpp 1c89a1159d8ac01a40f567beb14ed424ac613912 > src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 > src/tests/hierarchical_allocator_tests.cpp > 6dee2296d5a14185dbf7eee17968b20148839bfd > src/tests/master_allocator_tests.cpp > d05ee441a5120144aff42d78c095e1ce5051a6ac > src/tests/persistent_volume_endpoints_te
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated May 31, 2017, 8 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the `SUBSCRIBE` indicates a subset of roles to be suppressed during framework (re)registration, the allocator does not offer resources for those roles to such frameworks. Note that this functionality is added for `v1::SUBSCRIBE` only (and not for scheduler driver API). In addition, the master validates the suppressed roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 82fea40ad09c95f0096934c4210a39a6f0638ed9 src/master/allocator/mesos/hierarchical.cpp ca368bb5ef0dcfe83672004d58a4de6f85d6b4bc src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp eb2b647d3247a85e8b9b82e5589232c74ad8570f src/tests/master_allocator_tests.cpp b9e04226a1688499847bc25c7c220c0b82ba8db7 src/tests/persistent_volume_endpoints_tests.cpp 21136b29d724959527b889f52611baee0668 src/tests/reservation_tests.cpp 47ccf7f6f6ee48236f6794ce3084ce4f425c93c5 src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c src/tests/slave_recovery_tests.cpp df0c5c88786190be06df7ef3602834aa8985cefe Diff: https://reviews.apache.org/r/57817/diff/10/ Changes: https://reviews.apache.org/r/57817/diff/9-10/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53841: Added metrics for sorting in the role and quota sorters.
> On May 26, 2017, 4:56 p.m., Jiang Yan Xu wrote: > > For this and the next review, could you summarize how these metrics can be > > used to reason about the allocator/sorter's performance? > > > > I agree that conceptually we'd like something that tells us how well the > > (dirty or overall) sort performs but it's not immediately clear how to > > derive that from the provided metrics because `sort` on each sorter is > > called many times during one allocation, multiple times per agent. The > > three sorters are of the same implementation, how to interpret the metrics > > from each? The amount of work split between each sorter seems to be pretty > > dynamic? > > > > Also given the frequency that the timer (relatively expensive) is invoked, > > how much overhead would it cost the sort()? This is probably worth > > measuring if we add these. The latency in role (and quota) level sorts indicate how much time in the allocate cycle is being spent on the sorts. This can be significant if: i) A very high level of roles/quotas are being sorted; and ii) Events that make the `dirty` flag `false` occuring significant number of times (determined from the number of sorts being performed indicated by `allocator/mesos/roles/sort_runs` or `allocator/mesos/quotas/sort_runs`) leading to a significant number of times the sorts are actually happening. These stats by themselves are indicative of an actual problem but should help in diagnosing such conditions. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53841/#review176110 ----------- On May 24, 2017, 4:23 a.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53841/ > --- > > (Updated May 24, 2017, 4:23 a.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-6579 > https://issues.apache.org/jira/browse/MESOS-6579 > > > Repository: mesos > > > Description > --- > > Following metrics have been added: > a) allocator/mesos/roles/sort_runs: Number of role level sorts. > b) allocator/mesos/roles/sort_run: Latency in role level sorts. > c) allocator/mesos/quotas/sort_runs: Number of quota level sorts. > d) allocator/mesos/quotas/sort_run: Latency in quota level sorts. > > > Diffs > - > > docs/monitoring.md cb2833642e7e41c03c98ea92f7300d156a216a2e > src/master/allocator/mesos/hierarchical.hpp > 123f97cf495bff0f822838e09df0d88818f04da6 > src/master/allocator/sorter/drf/metrics.hpp > 61568cb520826ab59d675824b212e0d3deb63764 > src/master/allocator/sorter/drf/metrics.cpp > ff63fbac5bbcf54e1ae39c3b650c0dafe7ea46d4 > src/master/allocator/sorter/drf/sorter.hpp > fee58d6d1f08163e2a06a4a20c891fe535c3dcff > src/master/allocator/sorter/drf/sorter.cpp > 26b77f578f3235a8792c72d4575d607cdb2c7de7 > src/tests/hierarchical_allocator_tests.cpp > f90068a50c822aa90b864329ae87c9b5f8bb > > > Diff: https://reviews.apache.org/r/53841/diff/7/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 49571: Added a benchmark test for allocations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49571/ --- (Updated May 25, 2017, 11:30 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-5771 https://issues.apache.org/jira/browse/MESOS-5771 Repository: mesos Description --- Allocations test has the following resource configurations: (1) REGULAR: Offers from every slave have regular resources. (2) SHARED: Offers from every slave include a shared resource. (3) REGULAR: Offers from every alternate slave contain only regular resources; and offers from every other alternate slave contains a shared resource. This test is parameterized based on number of agents, number of frameworks and resource configuration. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp eb2b647d3247a85e8b9b82e5589232c74ad8570f src/tests/resources_utils.hpp 1f41f02babce5c8174ea2223f4dc7470452fbaf1 src/tests/resources_utils.cpp 2cef55f7312d671307e097c2c4960c8dcf45c1ff Diff: https://reviews.apache.org/r/49571/diff/36/ Changes: https://reviews.apache.org/r/49571/diff/35-36/ Testing --- All tests passed. Allocations benchmark test results == There is no visible impact in performance when shared resources are added in the allocations. The numbers for HEAD are prior to shared resources support (mid 2016) and the numbers indicate improvements in allocations during this timeframe. Following is a snapshot with 1000 agents and 200 frameworks. With the patch (and no shared resources) [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9 Using 1000 agents and 200 frameworks with resource type 0 Added 200 frameworks in 6588us Added 1000 agents in 1.567347secs round 0 allocate() took 1.15531secs to make 1000 offers round 10 allocate() took 1.152876secs to make 1000 offers round 20 allocate() took 1.15661secs to make 1000 offers round 30 allocate() took 1.117733secs to make 1000 offers round 40 allocate() took 1.118754secs to make 1000 offers round 50 allocate() took 1.11169secs to make 1000 offers With the patch (and shared resources on all agents) --- [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10 Using 1000 agents and 200 frameworks with resource type 1 Added 200 frameworks in 6064us Added 1000 agents in 1.627008secs round 0 allocate() took 1.168253secs to make 1000 offers round 10 allocate() took 1.146421secs to make 1000 offers round 20 allocate() took 1.16416secs to make 1000 offers round 30 allocate() took 1.210476secs to make 1000 offers round 40 allocate() took 1.194251secs to make 1000 offers round 50 allocate() took 1.17789secs to make 1000 offers With the patch (and shared resources on alternate agents) - [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11 Using 1000 agents and 200 frameworks with resource type 2 Added 200 frameworks in 6466us Added 1000 agents in 1.568717secs round 0 allocate() took 1.153005secs to make 1000 offers round 10 allocate() took 1.168169secs to make 1000 offers round 20 allocate() took 1.156774secs to make 1000 offers round 30 allocate() took 1.183112secs to make 1000 offers round 40 allocate() took 1.202452secs to make 1000 offers round 50 allocate() took 1.198918secs to make 1000 offers Based on HEAD, with all regular resources (no shared resources in HEAD supported) - [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9 Using 1000 agents and 200 frameworks with resource type 0 Added 200 frameworks in 6801us Added 1000 agents in 1.721447secs round 0 allocate() took 1.502953secs to make 1000 offers round 50 allocate() took 1.520157secs to make 1000 offers round 100 allocate() took 1.517221secs to make 1000 offers round 150 allocate() took 1.526446secs to make 1000 offers round 199 allocate() took 1.538005secs to make 1000 offers Thanks, Anindya Sinha
Re: Review Request 49571: Added a benchmark test for allocations.
> On May 24, 2017, 11:35 p.m., James Peach wrote: > > src/tests/hierarchical_allocator_tests.cpp > > Lines 4972 (patched) > > <https://reviews.apache.org/r/49571/diff/35/?file=1730051#file1730051line4972> > > > > Why do we need to loop that many times? I don't think you'd expect the > > performance to vary much over this range because tou are always recovering > > the resources so the allocation state doesn't change substantially. I now run the loop 6 times to ensure we cover each framework which should be `max(number_of_frameworks)/min(number_of_agents)`, i.e. `6000/1000 = 6`. > On May 24, 2017, 11:35 p.m., James Peach wrote: > > src/tests/hierarchical_allocator_tests.cpp > > Lines 4995 (patched) > > <https://reviews.apache.org/r/49571/diff/35/?file=1730051#file1730051line4995> > > > > I found that this output wasn't very helpful. How about running through > > the offer cycle N times and collecting the results in a > > `process::TimeSeries`, then showing the `process::Statistics` once at the > > end of the test? I think we should defer this to a future commit to do this for all benchmarks. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49571/#review176019 --- On May 25, 2017, 11:30 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49571/ > --- > > (Updated May 25, 2017, 11:30 p.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-5771 > https://issues.apache.org/jira/browse/MESOS-5771 > > > Repository: mesos > > > Description > --- > > Allocations test has the following resource configurations: > (1) REGULAR: Offers from every slave have regular resources. > (2) SHARED: Offers from every slave include a shared resource. > (3) REGULAR: Offers from every alternate slave contain only regular > resources; and offers from every other alternate slave contains > a shared resource. > > This test is parameterized based on number of agents, number of > frameworks and resource configuration. > > > Diffs > - > > src/tests/hierarchical_allocator_tests.cpp > eb2b647d3247a85e8b9b82e5589232c74ad8570f > src/tests/resources_utils.hpp 1f41f02babce5c8174ea2223f4dc7470452fbaf1 > src/tests/resources_utils.cpp 2cef55f7312d671307e097c2c4960c8dcf45c1ff > > > Diff: https://reviews.apache.org/r/49571/diff/36/ > > > Testing > --- > > All tests passed. > > Allocations benchmark test results > == > There is no visible impact in performance when shared resources are added in > the allocations. The numbers for HEAD are prior to shared resources support > (mid 2016) and the numbers indicate improvements in allocations during this > timeframe. > > Following is a snapshot with 1000 agents and 200 frameworks. > > With the patch (and no shared resources) > > [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9 > Using 1000 agents and 200 frameworks with resource type 0 > Added 200 frameworks in 6588us > Added 1000 agents in 1.567347secs > round 0 allocate() took 1.15531secs to make 1000 offers > round 10 allocate() took 1.152876secs to make 1000 offers > round 20 allocate() took 1.15661secs to make 1000 offers > round 30 allocate() took 1.117733secs to make 1000 offers > round 40 allocate() took 1.118754secs to make 1000 offers > round 50 allocate() took 1.11169secs to make 1000 offers > > With the patch (and shared resources on all agents) > --- > [ RUN ] > AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10 > Using 1000 agents and 200 frameworks with resource type 1 > Added 200 frameworks in 6064us > Added 1000 agents in 1.627008secs > round 0 allocate() took 1.168253secs to make 1000 offers > round 10 allocate() took 1.146421secs to make 1000 offers > round 20 allocate() took 1.16416secs to make 1000 offers > round 30 allocate() took 1.210476secs to make 1000 offers > round 40 allocate() took 1.194251secs to make 1000 offers > round 50 allocate() took 1.17789secs to make 1000 offers > > With the patch (and shared resources on alternate agents) > - > [ RUN ] > AllReso
Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52071/ --- (Updated May 24, 2017, 7:56 p.m.) Review request for mesos and Jiang Yan Xu. Bugs: MESOS-6062 https://issues.apache.org/jira/browse/MESOS-6062 Repository: mesos Description --- When a resource with no size is specified in `--resources` startup flag of mesos agent, the size is auto detected by the agent. This is enabled for all known resource types (except gpus) only when the resources are specified in JSON format. For disk resources, this is done for both root disks as well as MOUNT and PATH disks. Diffs (updated) - docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa Diff: https://reviews.apache.org/r/52071/diff/12/ Changes: https://reviews.apache.org/r/52071/diff/11-12/ Testing --- Documentation change only. All tests passed. Thanks, Anindya Sinha
Re: Review Request 51880: Added unit tests to determine disk size for MOUNT or PATH disks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51880/ --- (Updated May 24, 2017, 7:56 p.m.) Review request for mesos and Jiang Yan Xu. Bugs: MESOS-6062 https://issues.apache.org/jira/browse/MESOS-6062 Repository: mesos Description --- Added unit tests to determine disk size for MOUNT or PATH disks. Diffs (updated) - src/Makefile.am 7e4ce8532ec2c1b4216c2aa8d4262b2091e21c4a src/tests/containerizer/resources_containerizer_tests.cpp PRE-CREATION src/tests/resources_tests.cpp 5dcbce2dd4944194b59790551929d6d75b805ba3 Diff: https://reviews.apache.org/r/51880/diff/16/ Changes: https://reviews.apache.org/r/51880/diff/15-16/ Testing --- All tests including the additional tests in this RR passed. Thanks, Anindya Sinha
Re: Review Request 52002: Added helper methods to determine types of disk resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52002/ --- (Updated May 24, 2017, 7:56 p.m.) Review request for mesos and Jiang Yan Xu. Bugs: MESOS-6062 https://issues.apache.org/jira/browse/MESOS-6062 Repository: mesos Description --- Added helper methods to determine types of disk resources. Diffs (updated) - include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d include/mesos/v1/resources.hpp 132ef3eb03d335774ba13ecb39045128d0476954 src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c src/tests/resources_tests.cpp 5dcbce2dd4944194b59790551929d6d75b805ba3 src/v1/resources.cpp 1bb5d0741c9f9ea59f34e92159a335661019444d Diff: https://reviews.apache.org/r/52002/diff/13/ Changes: https://reviews.apache.org/r/52002/diff/12-13/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 51879: Autodetect value of resource when not specified in static resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51879/ --- (Updated May 24, 2017, 7:56 p.m.) Review request for mesos and Jiang Yan Xu. Bugs: MESOS-6062 https://issues.apache.org/jira/browse/MESOS-6062 Repository: mesos Description --- When static resources indicate resources with a positive size, we use that for the resources on the agent. However, --resources can include resources with no size, which indicates that mesos agent determine the size of those resources from the agent and uses that information. Note that auto-detection of resources is allowed for all known resource types represented in JSON format only. Auto-detection is not done when the resources are represented in text format. With this change, JSON representation for disk resources that do not specify any value would not result in an error, but those resources will not be accounted for until a valid size is determined for such resources. A scalar value of -1 still results in an invalid resource. Diffs (updated) - include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d include/mesos/v1/resources.hpp 132ef3eb03d335774ba13ecb39045128d0476954 src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c src/slave/containerizer/containerizer.cpp 15ae0b33a5ea8f73a9a84081d9c310c1d59c3141 src/slave/containerizer/mesos/isolators/gpu/allocator.cpp c288ad634b856702483b9751f41445308babd0c9 src/tests/persistent_volume_endpoints_tests.cpp 8c54372b7f6d94f0335561086f2a8cb90373e285 src/tests/reservation_endpoints_tests.cpp 8c195eb7d788fbca8722d66d81c77d399702160a src/v1/resources.cpp 1bb5d0741c9f9ea59f34e92159a335661019444d Diff: https://reviews.apache.org/r/51879/diff/13/ Changes: https://reviews.apache.org/r/51879/diff/12-13/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 53842: Add specific metrics for sorting runs across frameworks of a role.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53842/ --- (Updated May 24, 2017, 4:26 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased after the 1st patch in the chain was pushed. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description (updated) --- Added the following 2 metrics which is maintained on a role level (as long as there is at least one framework of that role): a) allocator/mesos/frameworks/role/sort_run_ms: Number of framework level sorts (based on role) in DRF Sorter. b) allocator/mesos/frameworks/role/sort_run_ms: Latency in framework level sorts (based on role) in DRF Sorter. Diffs (updated) - docs/monitoring.md cb2833642e7e41c03c98ea92f7300d156a216a2e src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp 5511bf6ce8c866c8a8436595f5b3eb1ef81c999f src/tests/hierarchical_allocator_tests.cpp f90068a50c822aa90b864329ae87c9b5f8bb Diff: https://reviews.apache.org/r/53842/diff/8/ Changes: https://reviews.apache.org/r/53842/diff/7-8/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53842: Add specific metrics for sorting runs across frameworks of a role.
> On April 25, 2017, 5:31 p.m., James Peach wrote: > > The code looks fine. As per the other revires this needs documentation. > > > > Please update the metric names in the commit. The metrics are > > `allocator/mesos/frameworks/` not > > `allocator/mesos/frameworks/role/`. Added documentation, and fixed the commit message. Thanks for catching that. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53842/#review172952 --- On May 23, 2017, 4:30 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53842/ > --- > > (Updated May 23, 2017, 4:30 p.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-6579 > https://issues.apache.org/jira/browse/MESOS-6579 > > > Repository: mesos > > > Description > --- > > Added the following 2 metrics which is maintained on a role level > (as long as there is at least one framework of that role): > a) allocator/mesos/frameworks/role//sort_runs: Number of framework >level sorts (based on role) in DRF Sorter. > b) allocator/mesos/frameworks/role//sort_run: Latency in framework >level sorts (based on role) in DRF Sorter. > > > Diffs > - > > docs/monitoring.md a027f4905a0e6e41ff4e1348d34fd7aa5f1cbe61 > src/master/allocator/mesos/hierarchical.hpp > 123f97cf495bff0f822838e09df0d88818f04da6 > src/master/allocator/mesos/hierarchical.cpp > b75ed9a20a9a42f958cebbacd91e5e15b0d21394 > src/tests/hierarchical_allocator_tests.cpp > 6dee2296d5a14185dbf7eee17968b20148839bfd > > > Diff: https://reviews.apache.org/r/53842/diff/7/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 53841: Added metrics for sorting in the role and quota sorters.
> On April 20, 2017, 9:15 p.m., James Peach wrote: > > This also needs documentation added to the `docs/monitoring.md`. Documentation added. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53841/#review172540 --- On May 24, 2017, 4:23 a.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53841/ > --- > > (Updated May 24, 2017, 4:23 a.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-6579 > https://issues.apache.org/jira/browse/MESOS-6579 > > > Repository: mesos > > > Description > --- > > Following metrics have been added: > a) allocator/mesos/roles/sort_runs: Number of role level sorts. > b) allocator/mesos/roles/sort_run: Latency in role level sorts. > c) allocator/mesos/quotas/sort_runs: Number of quota level sorts. > d) allocator/mesos/quotas/sort_run: Latency in quota level sorts. > > > Diffs > - > > docs/monitoring.md cb2833642e7e41c03c98ea92f7300d156a216a2e > src/master/allocator/mesos/hierarchical.hpp > 123f97cf495bff0f822838e09df0d88818f04da6 > src/master/allocator/sorter/drf/metrics.hpp > 61568cb520826ab59d675824b212e0d3deb63764 > src/master/allocator/sorter/drf/metrics.cpp > ff63fbac5bbcf54e1ae39c3b650c0dafe7ea46d4 > src/master/allocator/sorter/drf/sorter.hpp > fee58d6d1f08163e2a06a4a20c891fe535c3dcff > src/master/allocator/sorter/drf/sorter.cpp > 26b77f578f3235a8792c72d4575d607cdb2c7de7 > src/tests/hierarchical_allocator_tests.cpp > f90068a50c822aa90b864329ae87c9b5f8bb > > > Diff: https://reviews.apache.org/r/53841/diff/7/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 53841: Added metrics for sorting in the role and quota sorters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53841/ --- (Updated May 24, 2017, 4:23 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased after the 1st patch in the chain was pushed. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Following metrics have been added: a) allocator/mesos/roles/sort_runs: Number of role level sorts. b) allocator/mesos/roles/sort_run: Latency in role level sorts. c) allocator/mesos/quotas/sort_runs: Number of quota level sorts. d) allocator/mesos/quotas/sort_run: Latency in quota level sorts. Diffs (updated) - docs/monitoring.md cb2833642e7e41c03c98ea92f7300d156a216a2e src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/sorter/drf/metrics.hpp 61568cb520826ab59d675824b212e0d3deb63764 src/master/allocator/sorter/drf/metrics.cpp ff63fbac5bbcf54e1ae39c3b650c0dafe7ea46d4 src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7 src/tests/hierarchical_allocator_tests.cpp f90068a50c822aa90b864329ae87c9b5f8bb Diff: https://reviews.apache.org/r/53841/diff/7/ Changes: https://reviews.apache.org/r/53841/diff/6-7/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated May 23, 2017, 10:20 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/9/ Changes: https://reviews.apache.org/r/57818/diff/8-9/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated May 23, 2017, 10:19 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the `SUBSCRIBE` indicates a subset of roles to be suppressed during framework (re)registration, the allocator does not offer resources for those roles to such frameworks. Note that this functionality is added for `v1::SUBSCRIBE` only (and not for scheduler driver API). In addition, the master validates the suppressed roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 1c89a1159d8ac01a40f567beb14ed424ac613912 src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp 6dee2296d5a14185dbf7eee17968b20148839bfd src/tests/master_allocator_tests.cpp d05ee441a5120144aff42d78c095e1ce5051a6ac src/tests/persistent_volume_endpoints_tests.cpp 8c54372b7f6d94f0335561086f2a8cb90373e285 src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c src/tests/slave_recovery_tests.cpp 52e78b6b6280a159233b402ce2849448204d4f11 Diff: https://reviews.apache.org/r/57817/diff/9/ Changes: https://reviews.apache.org/r/57817/diff/8-9/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
> On May 23, 2017, 9:01 p.m., Vinod Kone wrote: > > src/master/allocator/mesos/hierarchical.hpp > > Line 309 (original), 309 (patched) > > <https://reviews.apache.org/r/57817/diff/4-8/?file=1689440#file1689440line312> > > > > I don't quite follow why you need to have this variable here? Looks > > like the sorter can be the source of truth of whether a role is activated > > or not? `FrameworkInfo::roles` indicates all the roles the framework registers as, and `Call::Subscribe::suppressed_roles` is a subset of roles for which the master would not offer resources. The effective `suppressed_roles` can change (after the framework is (re)registered) with `SUPPRESS` and `REVIVE` calls, which means the roles for which offers are sent to frameworks can change based on roles contained in these messages. Now, the existing `activateFramework()` (and `deactivateFramework()`) activates (and deactivates) all roles of the framework. I think that should not be the case any longer. We should activate (and deactivate) roles of the framework which are not contained in `suppressed_roles` at any given point of time. To achieve this, we keep track of `HierarchicalAllocatorProcess::Framework::Framework::suppressed_roles` to ensure we keep track of `suppressed_roles` based on `SUPPRESS` and `REVIVE` calls during the life cycle of the framework. This would help us determine if we activate (or deactivate) for a specific role based on whether the role is a `suppressed_roles`. > On May 23, 2017, 9:01 p.m., Vinod Kone wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 348 (patched) > > <https://reviews.apache.org/r/57817/diff/4-8/?file=1689441#file1689441line352> > > > > why the if? is the `sorter::activate` not idempotent? See response to the first comment. > On May 23, 2017, 9:01 p.m., Vinod Kone wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 370 (patched) > > <https://reviews.apache.org/r/57817/diff/4-8/?file=1689441#file1689441line374> > > > > ditto. Same as the response to the first comment. > On May 23, 2017, 9:01 p.m., Vinod Kone wrote: > > src/master/master.cpp > > Lines 2804-2808 (patched) > > <https://reviews.apache.org/r/57817/diff/4-8/?file=1689443#file1689443line2809> > > > > why do you need this check? is the below one not sufficient? Just an optimization to not loop through the roles in `suppressedRoles` incase the sizes of `frameworkRoles` and `suppressedRoles` differ. But I think it should be fine to loop through since we do not anticipate the number of roles to be too many. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/#review175846 --- On May 23, 2017, 10:19 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57817/ > --- > > (Updated May 23, 2017, 10:19 p.m.) > > > Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. > > > Bugs: MESOS-7015 > https://issues.apache.org/jira/browse/MESOS-7015 > > > Repository: mesos > > > Description > --- > > If the `SUBSCRIBE` indicates a subset of roles to be suppressed during > framework (re)registration, the allocator does not offer resources for > those roles to such frameworks. Note that this functionality is added > for `v1::SUBSCRIBE` only (and not for scheduler driver API). > > In addition, the master validates the suppressed roles to be a subset > of all the roles being (re)registered by the framework. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 > src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 > src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d > src/master/allocator/mesos/allocator.hpp > 119b461136123229f85ed3c3cfd41137974a6b9b > src/master/allocator/mesos/hierarchical.hpp > 123f97cf495bff0f822838e09df0d88818f04da6 > src/master/allocator/mesos/hierarchical.cpp > b75ed9a20a9a42f958cebbacd91e5e15b0d21394 > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/master/master.cpp 1c89a1159d8ac01a40f567beb14ed424ac613912 > src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 > src/tests/hierarchical_allocator_tests.cpp > 6dee2296d5a14185dbf7eee17968b20148839bfd > src/tests/master_allocator_tests.cpp
Re: Review Request 53666: Refactor cgroups cleanup to allow cleanup to continue on an error.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53666/ --- (Updated May 23, 2017, 8:59 p.m.) Review request for mesos, Jie Yu and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-6489 https://issues.apache.org/jira/browse/MESOS-6489 Repository: mesos Description --- Added a `continueOnError` attribute to cgroups `Destroyer` to allow cgroups cleanup to continue incase of any error. The linux launcher continues cleanup of cgroups even if there is an error. However, if the top level cgroup is actually removed, it treats it as a success. Diffs (updated) - src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 src/slave/containerizer/mesos/linux_launcher.cpp 1cea04edac8e0c4aea8c1c7d946b5065f3eac931 Diff: https://reviews.apache.org/r/53666/diff/3/ Changes: https://reviews.apache.org/r/53666/diff/2-3/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 49571: Added a benchmark test for allocations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49571/ --- (Updated May 23, 2017, 6:59 p.m.) Review request for mesos and Jiang Yan Xu. Changes --- Rebased to incorporate `AllocationInfo`. Bugs: MESOS-5771 https://issues.apache.org/jira/browse/MESOS-5771 Repository: mesos Description --- Allocations test has the following resource configurations: (1) REGULAR: Offers from every slave have regular resources. (2) SHARED: Offers from every slave include a shared resource. (3) REGULAR: Offers from every alternate slave contain only regular resources; and offers from every other alternate slave contains a shared resource. This test is parameterized based on number of agents, number of frameworks and resource configuration. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 6dee2296d5a14185dbf7eee17968b20148839bfd src/tests/resources_utils.hpp 1f41f02babce5c8174ea2223f4dc7470452fbaf1 src/tests/resources_utils.cpp 2cef55f7312d671307e097c2c4960c8dcf45c1ff Diff: https://reviews.apache.org/r/49571/diff/35/ Changes: https://reviews.apache.org/r/49571/diff/34-35/ Testing (updated) --- All tests passed. Allocations benchmark test results == There is no visible impact in performance when shared resources are added in the allocations. The numbers for HEAD are prior to shared resources support (mid 2016) and the numbers indicate improvements in allocations during this timeframe. Following is a snapshot with 1000 agents and 200 frameworks. With the patch (and no shared resources) [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9 Using 1000 agents and 200 frameworks with resource type 0 Added 200 frameworks in 6588us Added 1000 agents in 1.567347secs round 0 allocate() took 1.15531secs to make 1000 offers round 10 allocate() took 1.152876secs to make 1000 offers round 20 allocate() took 1.15661secs to make 1000 offers round 30 allocate() took 1.117733secs to make 1000 offers round 40 allocate() took 1.118754secs to make 1000 offers round 50 allocate() took 1.11169secs to make 1000 offers With the patch (and shared resources on all agents) --- [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10 Using 1000 agents and 200 frameworks with resource type 1 Added 200 frameworks in 6064us Added 1000 agents in 1.627008secs round 0 allocate() took 1.168253secs to make 1000 offers round 10 allocate() took 1.146421secs to make 1000 offers round 20 allocate() took 1.16416secs to make 1000 offers round 30 allocate() took 1.210476secs to make 1000 offers round 40 allocate() took 1.194251secs to make 1000 offers round 50 allocate() took 1.17789secs to make 1000 offers With the patch (and shared resources on alternate agents) - [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11 Using 1000 agents and 200 frameworks with resource type 2 Added 200 frameworks in 6466us Added 1000 agents in 1.568717secs round 0 allocate() took 1.153005secs to make 1000 offers round 10 allocate() took 1.168169secs to make 1000 offers round 20 allocate() took 1.156774secs to make 1000 offers round 30 allocate() took 1.183112secs to make 1000 offers round 40 allocate() took 1.202452secs to make 1000 offers round 50 allocate() took 1.198918secs to make 1000 offers Based on HEAD, with all regular resources (no shared resources in HEAD supported) - [ RUN ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9 Using 1000 agents and 200 frameworks with resource type 0 Added 200 frameworks in 6801us Added 1000 agents in 1.721447secs round 0 allocate() took 1.502953secs to make 1000 offers round 50 allocate() took 1.520157secs to make 1000 offers round 100 allocate() took 1.517221secs to make 1000 offers round 150 allocate() took 1.526446secs to make 1000 offers round 199 allocate() took 1.538005secs to make 1000 offers Thanks, Anindya Sinha
Re: Review Request 53841: Added metrics for sorting in the role and quota sorters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53841/ --- (Updated May 23, 2017, 4:30 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebase. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Following metrics have been added: a) allocator/mesos/roles/sort_runs: Number of role level sorts. b) allocator/mesos/roles/sort_run: Latency in role level sorts. c) allocator/mesos/quotas/sort_runs: Number of quota level sorts. d) allocator/mesos/quotas/sort_run: Latency in quota level sorts. Diffs (updated) - docs/monitoring.md a027f4905a0e6e41ff4e1348d34fd7aa5f1cbe61 src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/sorter/drf/metrics.hpp 61568cb520826ab59d675824b212e0d3deb63764 src/master/allocator/sorter/drf/metrics.cpp ff63fbac5bbcf54e1ae39c3b650c0dafe7ea46d4 src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7 src/tests/hierarchical_allocator_tests.cpp 6dee2296d5a14185dbf7eee17968b20148839bfd Diff: https://reviews.apache.org/r/53841/diff/6/ Changes: https://reviews.apache.org/r/53841/diff/5-6/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53842: Add specific metrics for sorting runs across frameworks of a role.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53842/ --- (Updated May 23, 2017, 4:30 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebase. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Added the following 2 metrics which is maintained on a role level (as long as there is at least one framework of that role): a) allocator/mesos/frameworks/role//sort_runs: Number of framework level sorts (based on role) in DRF Sorter. b) allocator/mesos/frameworks/role//sort_run: Latency in framework level sorts (based on role) in DRF Sorter. Diffs (updated) - docs/monitoring.md a027f4905a0e6e41ff4e1348d34fd7aa5f1cbe61 src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/hierarchical_allocator_tests.cpp 6dee2296d5a14185dbf7eee17968b20148839bfd Diff: https://reviews.apache.org/r/53842/diff/7/ Changes: https://reviews.apache.org/r/53842/diff/6-7/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53840: Metric in the allocator to track latency in running allocations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53840/ --- (Updated May 23, 2017, 4:30 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebase. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Added a metric 'allocator/mesos/allocation_run_interval_ms' which tracks the latency between scheduling an allocation to an actual allocation run. Diffs (updated) - docs/monitoring.md a027f4905a0e6e41ff4e1348d34fd7aa5f1cbe61 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/master/allocator/mesos/metrics.hpp ce486eb079dc44bf0bbfad8668426c7ae87c97b3 src/master/allocator/mesos/metrics.cpp ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 src/tests/hierarchical_allocator_tests.cpp 6dee2296d5a14185dbf7eee17968b20148839bfd Diff: https://reviews.apache.org/r/53840/diff/7/ Changes: https://reviews.apache.org/r/53840/diff/6-7/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57815: Added `suppressed_roles` field in `SUBSCRIBE`.
> On April 17, 2017, 6:28 p.m., Vinod Kone wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 249-250 (original), 249-250 (patched)
> > <https://reviews.apache.org/r/57815/diff/1-3/?file=1670985#file1670985line249>
> >
> > Hmm, I was hoping that we could re-use the `Call::Suppress` message
> > here instead of re-defining it. That way any changes to suppress would be
> > in sync.
>
> Anindya Sinha wrote:
> Currently, `Call::Suppress` is:
> ```
> message Suppress {
> optional string role = 1;
> }
> ```
>
> So, the `SUPPRESS` call currently allows suppressing offers for a
> framework on a per-role basis (or all roles if `role` is not set). Since we
> now support multiple roles for a frameework, and if we want to suppress
> offers for a subset of roles at the time of `SUBSCRIBE`, using
> `Call::Suppress` would not satisfy that use case. It would only satisfy
> suppressing for all roles (i.e. `Suppress::role` is not set) or for a
> specific role.
>
> Is there any plans of extending `SUPPRESS` to suppress offers for a
> subset of roles (>0, and < all roles) in a single call? Do you think we
> should consider that also?
>
> One such approach could be to have the following:
> ```
> message Suppress {
> repeated string roles = 1;
> }
>
> optional Suppress suppress;
> ```
>
> If suppress is not set, then we do not suppress for any roles. Otherwise,
> we suppress for the set of roles specified in `suppress.roles`. If `suppress`
> is set but `suppress.roles` is empty, we suppress for all roles of the
> framework.
>
> Thoughts?
>
> Anindya Sinha wrote:
> So based on discussion in the Slack channel, this modified review chain
> does the following:
>
> 1. Add `repeated string deactivated_roles` to `FrameworkInfo` which
> represents a subset of roles which are deactivated. Offers pertaining to the
> deactivated roles shall not be sent out.
> 2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of
> the roles.
> 3. Allocator's `activateFramework()` call will activate all roles of the
> framework which are not in `FrameworkInfo::deactivated+roles`. Similarly, in
> `deactivateFramework()` call will deactivate all roles that are not
> deactivated already.
Based on discussion on Slack, this is what was decided:
1. Add `repeated string suppressed_roles` to `Call::Subscribe` which represents
a subset of roles which are suppressed. Offers pertaining to the suppressed
roles shall not be sent out.
2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of the
roles.
3. Allocator's `activateFramework()` call will activate all roles of the
framework which are not in suppressed_roles. Similarly, in
`deactivateFramework()` call will deactivate all roles that are not suppressed
already.
Note that this functionality shall not be added for scheduler driver in this
review chain (for MESOS-7015). I opened
https://issues.apache.org/jira/browse/MESOS-7526 to address that.
- Anindya
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/#review172115
---
On May 19, 2017, 6:26 p.m., Anindya Sinha wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> ---
>
> (Updated May 19, 2017, 6:26 p.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This field is a subset of roles the framework registered as for which
> the framework does not want any resources offere to.
>
>
> Diffs
> -
>
> include/mesos/scheduler/scheduler.proto
> f83b2ce7e88e83abc4e523b06333c448a3dcfd01
> include/mesos/v1/scheduler/scheduler.proto
> d923cb9dc4205e4b601feebba84e3b30091ea3e0
>
>
> Diff: https://reviews.apache.org/r/57815/diff/6/
>
>
> Testing
> ---
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated May 19, 2017, 6:26 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Updates based on agreed approach. Summary (updated) - Offers not sent for suppressed roles as indicated in `SUBSCRIBE`. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description (updated) --- If the `SUBSCRIBE` indicates a subset of roles to be suppressed during framework (re)registration, the allocator does not offer resources for those roles to such frameworks. Note that this functionality is added for `v1::SUBSCRIBE` only (and not for scheduler driver API). In addition, the master validates the suppressed roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 02affe2d6dc76ef91363df04d8d8cbed3beaf34f src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp 6dee2296d5a14185dbf7eee17968b20148839bfd src/tests/master_allocator_tests.cpp d05ee441a5120144aff42d78c095e1ce5051a6ac src/tests/persistent_volume_endpoints_tests.cpp 8c54372b7f6d94f0335561086f2a8cb90373e285 src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c src/tests/slave_recovery_tests.cpp 52e78b6b6280a159233b402ce2849448204d4f11 Diff: https://reviews.apache.org/r/57817/diff/8/ Changes: https://reviews.apache.org/r/57817/diff/7-8/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated May 19, 2017, 6:26 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Updates based on agreed approach. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 Diff: https://reviews.apache.org/r/57818/diff/8/ Changes: https://reviews.apache.org/r/57818/diff/7-8/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57815: Added `suppressed_roles` field in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57815/ --- (Updated May 19, 2017, 6:26 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Updates based on agreed approach. Summary (updated) - Added `suppressed_roles` field in `SUBSCRIBE`. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- This field is a subset of roles the framework registered as for which the framework does not want any resources offere to. Diffs (updated) - include/mesos/scheduler/scheduler.proto f83b2ce7e88e83abc4e523b06333c448a3dcfd01 include/mesos/v1/scheduler/scheduler.proto d923cb9dc4205e4b601feebba84e3b30091ea3e0 Diff: https://reviews.apache.org/r/57815/diff/6/ Changes: https://reviews.apache.org/r/57815/diff/5-6/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 58486: Update the allocator on a per offer operation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58486/ --- (Updated May 17, 2017, 4:50 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Do not recalculate resources to be recovered if there is no error in `updateAllocation()`. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- The allocator is updated for each offer operation in the order specified in the `ACCEPT` call. Once the allocator is updated successfully, we handle subsequent operations as follows: 1) Launch or Launch Group: We launch the tasks or task group on the agent. 2) (Un)reservation or Create/Destroy of Persistent Volumes: We send the `CheckPointResourcesMessage` to the agent. If allocation for any of the operations fail, those resources are not recovered from the allocator. Diffs (updated) - src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad Diff: https://reviews.apache.org/r/58486/diff/8/ Changes: https://reviews.apache.org/r/58486/diff/7-8/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 58486: Update the allocator on a per offer operation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58486/ --- (Updated May 17, 2017, 2 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Validate that the valid operations can be processed in `Master::__accept()`. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- The allocator is updated for each offer operation in the order specified in the `ACCEPT` call. Once the allocator is updated successfully, we handle subsequent operations as follows: 1) Launch or Launch Group: We launch the tasks or task group on the agent. 2) (Un)reservation or Create/Destroy of Persistent Volumes: We send the `CheckPointResourcesMessage` to the agent. If allocation for any of the operations fail, those resources are not recovered from the allocator. Diffs (updated) - src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad Diff: https://reviews.apache.org/r/58486/diff/7/ Changes: https://reviews.apache.org/r/58486/diff/6-7/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 59195: Ensure that allocator can be updated before committing changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59195/ --- (Updated May 17, 2017, 2 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Check if `slave.allocated.apply()` can be done before iterating through individual operations. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description (updated) --- In addition to `apply()` on `offeredResources`, we ensure all operations can be applied on `slave.allocated`. It is is so because certain operations require that they are applicable to the agent's allocated resources as a whole. Note that we return a failed future on any error.We do not modify the state of the allocator if any of the operations fail. Diffs (updated) - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/59195/diff/4/ Changes: https://reviews.apache.org/r/59195/diff/3-4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 59300: Updated documentation for `registrar/log/network_size`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59300/#review175144 --- Ship it! Ship It! - Anindya Sinha On May 15, 2017, 11:08 p.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59300/ > --- > > (Updated May 15, 2017, 11:08 p.m.) > > > Review request for mesos, Anindya Sinha and Benjamin Mahler. > > > Bugs: MESOS-7507 > https://issues.apache.org/jira/browse/MESOS-7507 > > > Repository: mesos > > > Description > --- > > Updated documentation for `registrar/log/network_size`. > > > Diffs > - > > docs/monitoring.md c42afce0a26dc15e033ee1375b7cb23d55be40ab > > > Diff: https://reviews.apache.org/r/59300/diff/1/ > > > Testing > --- > > Markdown viewer. > > > Thanks, > > Jiang Yan Xu > >
Re: Review Request 59290: Added '/log/network_size' metric.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59290/#review175143 --- Ship it! Ship It! - Anindya Sinha On May 15, 2017, 10:47 p.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59290/ > --- > > (Updated May 15, 2017, 10:47 p.m.) > > > Review request for mesos, Anindya Sinha and Benjamin Mahler. > > > Bugs: MESOS-7507 > https://issues.apache.org/jira/browse/MESOS-7507 > > > Repository: mesos > > > Description > --- > > Added '/log/network_size' metric. > > Will update monitoring.md next. > > > Diffs > - > > src/log/log.hpp a600025f4ca38e3fad0f64f48b007138da8e22d4 > src/log/metrics.hpp PRE-CREATION > src/log/metrics.cpp PRE-CREATION > src/tests/log_tests.cpp 52b1bfedd8d3f8f2ab3308c4be9f167126ee694b > > > Diff: https://reviews.apache.org/r/59290/diff/1/ > > > Testing > --- > > make check. > > > Thanks, > > Jiang Yan Xu > >
Re: Review Request 59289: Refactored log Metrics into separate files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59289/#review175133 --- Ship it! Ship It! - Anindya Sinha On May 15, 2017, 10:46 p.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59289/ > --- > > (Updated May 15, 2017, 10:46 p.m.) > > > Review request for mesos, Anindya Sinha and Jie Yu. > > > Bugs: MESOS-7507 > https://issues.apache.org/jira/browse/MESOS-7507 > > > Repository: mesos > > > Description > --- > > A separate cpp file is more appropriate as we add more metrics and > make the struct more complex. > > No functional change. > > > Diffs > - > > src/CMakeLists.txt eef718d95b5d8e051a5094369dc9b4532bc307ff > src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a > src/log/log.hpp a600025f4ca38e3fad0f64f48b007138da8e22d4 > src/log/log.cpp 2301eef564f2a42958c6c2c8eef0cc4b2fd76353 > src/log/metrics.hpp PRE-CREATION > src/log/metrics.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/59289/diff/1/ > > > Testing > --- > > make check. > > > Thanks, > > Jiang Yan Xu > >
Re: Review Request 53842: Add specific metrics for sorting runs across frameworks of a role.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53842/ --- (Updated May 16, 2017, 6:27 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Fixed the view aspect of docs/monitoring.md. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Added the following 2 metrics which is maintained on a role level (as long as there is at least one framework of that role): a) allocator/mesos/frameworks/role//sort_runs: Number of framework level sorts (based on role) in DRF Sorter. b) allocator/mesos/frameworks/role//sort_run: Latency in framework level sorts (based on role) in DRF Sorter. Diffs (updated) - docs/monitoring.md c42afce0a26dc15e033ee1375b7cb23d55be40ab src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/53842/diff/6/ Changes: https://reviews.apache.org/r/53842/diff/5-6/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53842: Add specific metrics for sorting runs across frameworks of a role.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53842/ --- (Updated May 16, 2017, 7:22 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Updated docs. Summary (updated) - Add specific metrics for sorting runs across frameworks of a role. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Added the following 2 metrics which is maintained on a role level (as long as there is at least one framework of that role): a) allocator/mesos/frameworks/role//sort_runs: Number of framework level sorts (based on role) in DRF Sorter. b) allocator/mesos/frameworks/role//sort_run: Latency in framework level sorts (based on role) in DRF Sorter. Diffs (updated) - docs/monitoring.md c42afce0a26dc15e033ee1375b7cb23d55be40ab src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/53842/diff/5/ Changes: https://reviews.apache.org/r/53842/diff/4-5/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53841: Added metrics for sorting in the role and quota sorters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53841/ --- (Updated May 16, 2017, 7:22 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Updated docs. Summary (updated) - Added metrics for sorting in the role and quota sorters. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Following metrics have been added: a) allocator/mesos/roles/sort_runs: Number of role level sorts. b) allocator/mesos/roles/sort_run: Latency in role level sorts. c) allocator/mesos/quotas/sort_runs: Number of quota level sorts. d) allocator/mesos/quotas/sort_run: Latency in quota level sorts. Diffs (updated) - docs/monitoring.md c42afce0a26dc15e033ee1375b7cb23d55be40ab src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/sorter/drf/metrics.hpp 61568cb520826ab59d675824b212e0d3deb63764 src/master/allocator/sorter/drf/metrics.cpp ff63fbac5bbcf54e1ae39c3b650c0dafe7ea46d4 src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/53841/diff/5/ Changes: https://reviews.apache.org/r/53841/diff/4-5/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53840: Metric in the allocator to track latency in running allocations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53840/ --- (Updated May 16, 2017, 7:22 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Updated docs. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Added a metric 'allocator/mesos/allocation_run_interval_ms' which tracks the latency between scheduling an allocation to an actual allocation run. Diffs (updated) - docs/monitoring.md c42afce0a26dc15e033ee1375b7cb23d55be40ab src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/master/allocator/mesos/metrics.hpp ce486eb079dc44bf0bbfad8668426c7ae87c97b3 src/master/allocator/mesos/metrics.cpp ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/53840/diff/6/ Changes: https://reviews.apache.org/r/53840/diff/5-6/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53842: Add role specific metrics for sorting runs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53842/ --- (Updated May 16, 2017, 6:24 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Added the following 2 metrics which is maintained on a role level (as long as there is at least one framework of that role): a) allocator/mesos/frameworks/role//sort_runs: Number of framework level sorts (based on role) in DRF Sorter. b) allocator/mesos/frameworks/role//sort_run: Latency in framework level sorts (based on role) in DRF Sorter. Diffs (updated) - src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/53842/diff/4/ Changes: https://reviews.apache.org/r/53842/diff/3-4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53841: Added metrics for sorting of the sorters in the allocator.
> On April 20, 2017, 9:15 p.m., James Peach wrote: > > src/master/allocator/sorter/drf/metrics.hpp > > Lines 59 (patched) > > <https://reviews.apache.org/r/53841/diff/3/?file=1658621#file1658621line59> > > > > Why is this necessary? Is there a reason that publishing the dominant > > resources and shares for quotas doesn't make sense? The reason publishing the dominant resources and shares for quotas is disabled because that is what was originally. The flag `includeClientMetrics` is most useful for framework sorters since we keep it disabled because of the possibility of high number of framework sorters (https://reviews.apache.org/r/53842/). However, I think it makes sense to enable this for quotas though. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53841/#review172540 ------- On May 16, 2017, 6:24 a.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53841/ > --- > > (Updated May 16, 2017, 6:24 a.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-6579 > https://issues.apache.org/jira/browse/MESOS-6579 > > > Repository: mesos > > > Description > --- > > Following metrics have been added: > a) allocator/mesos/roles/sort_runs: Number of role level sorts. > b) allocator/mesos/roles/sort_run: Latency in role level sorts. > c) allocator/mesos/quotas/sort_runs: Number of quota level sorts. > d) allocator/mesos/quotas/sort_run: Latency in quota level sorts. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.hpp > 123f97cf495bff0f822838e09df0d88818f04da6 > src/master/allocator/sorter/drf/metrics.hpp > 61568cb520826ab59d675824b212e0d3deb63764 > src/master/allocator/sorter/drf/metrics.cpp > ff63fbac5bbcf54e1ae39c3b650c0dafe7ea46d4 > src/master/allocator/sorter/drf/sorter.hpp > fee58d6d1f08163e2a06a4a20c891fe535c3dcff > src/master/allocator/sorter/drf/sorter.cpp > 26b77f578f3235a8792c72d4575d607cdb2c7de7 > src/tests/hierarchical_allocator_tests.cpp > 08180b9975869de328f0c095dd3cddf0c84fbecf > > > Diff: https://reviews.apache.org/r/53841/diff/4/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 53841: Added metrics for sorting of the sorters in the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53841/ --- (Updated May 16, 2017, 6:24 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased and minor edits. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Following metrics have been added: a) allocator/mesos/roles/sort_runs: Number of role level sorts. b) allocator/mesos/roles/sort_run: Latency in role level sorts. c) allocator/mesos/quotas/sort_runs: Number of quota level sorts. d) allocator/mesos/quotas/sort_run: Latency in quota level sorts. Diffs (updated) - src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/sorter/drf/metrics.hpp 61568cb520826ab59d675824b212e0d3deb63764 src/master/allocator/sorter/drf/metrics.cpp ff63fbac5bbcf54e1ae39c3b650c0dafe7ea46d4 src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/53841/diff/4/ Changes: https://reviews.apache.org/r/53841/diff/3-4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53840: Metric in the allocator to track latency in running allocations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53840/ --- (Updated May 16, 2017, 6:23 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-6579 https://issues.apache.org/jira/browse/MESOS-6579 Repository: mesos Description --- Added a metric 'allocator/mesos/allocation_run_interval_ms' which tracks the latency between scheduling an allocation to an actual allocation run. Diffs (updated) - src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/master/allocator/mesos/metrics.hpp ce486eb079dc44bf0bbfad8668426c7ae87c97b3 src/master/allocator/mesos/metrics.cpp ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/53840/diff/5/ Changes: https://reviews.apache.org/r/53840/diff/4-5/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 53840: Metric in the allocator to track latency in running allocations.
> On May 15, 2017, 5:25 a.m., Jiang Yan Xu wrote: > > src/tests/hierarchical_allocator_tests.cpp > > Lines 3459-3463 (original), 3459-3462 (patched) > > <https://reviews.apache.org/r/53840/diff/4/?file=1697516#file1697516line3459> > > > > Why this change? Seems like the two ways of expressing the expectation > > are equivalent. Reinstated the original. > On May 15, 2017, 5:25 a.m., Jiang Yan Xu wrote: > > src/tests/hierarchical_allocator_tests.cpp > > Lines 3537 (patched) > > <https://reviews.apache.org/r/53840/diff/4/?file=1697516#file1697516line3539> > > > > Seems inelegant to reuse the test for `allocation_run`, for > > `allocation_run` we are storing a full list of statistics in `statistics` > > but we are not doing the same for the new metric. Can we create an > > independent test? Created a separate test for `allocation_run_latency`. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53840/#review174871 --- On May 16, 2017, 6:23 a.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53840/ > --- > > (Updated May 16, 2017, 6:23 a.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-6579 > https://issues.apache.org/jira/browse/MESOS-6579 > > > Repository: mesos > > > Description > --- > > Added a metric 'allocator/mesos/allocation_run_interval_ms' which > tracks the latency between scheduling an allocation to an actual > allocation run. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > b75ed9a20a9a42f958cebbacd91e5e15b0d21394 > src/master/allocator/mesos/metrics.hpp > ce486eb079dc44bf0bbfad8668426c7ae87c97b3 > src/master/allocator/mesos/metrics.cpp > ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 > src/tests/hierarchical_allocator_tests.cpp > 08180b9975869de328f0c095dd3cddf0c84fbecf > > > Diff: https://reviews.apache.org/r/53840/diff/5/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 53840: Metric in the allocator to track latency in running allocations.
> On April 25, 2017, 4:41 p.m., James Peach wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Line 1427 (original), 1428 (patched) > > <https://reviews.apache.org/r/53840/diff/4/?file=1697513#file1697513line1428> > > > > You also need to stop the latency metric here, otherwise you will > > capture multiple allocator dispatches. As Yan mentioned, I stop the `allocation_run_latency` as the first thing done in this function. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53840/#review172945 ------- On May 16, 2017, 6:23 a.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53840/ > --- > > (Updated May 16, 2017, 6:23 a.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-6579 > https://issues.apache.org/jira/browse/MESOS-6579 > > > Repository: mesos > > > Description > --- > > Added a metric 'allocator/mesos/allocation_run_interval_ms' which > tracks the latency between scheduling an allocation to an actual > allocation run. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > b75ed9a20a9a42f958cebbacd91e5e15b0d21394 > src/master/allocator/mesos/metrics.hpp > ce486eb079dc44bf0bbfad8668426c7ae87c97b3 > src/master/allocator/mesos/metrics.cpp > ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 > src/tests/hierarchical_allocator_tests.cpp > 08180b9975869de328f0c095dd3cddf0c84fbecf > > > Diff: https://reviews.apache.org/r/53840/diff/5/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 59195: Ensure that allocator can be updated before committing changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59195/ --- (Updated May 15, 2017, 6:01 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- In addition to `apply()` on `offeredResources`, we ensure this operation can be applied on `slave.allocated`. It is is so because certain operations require that they are applicable to the agent's allocated resources as a whole. Note that we return a failed future on the first error (and there may be other operations which might have failed). However, we do not modify the state of the allocator if any of the operations fail. Diffs (updated) - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/59195/diff/3/ Changes: https://reviews.apache.org/r/59195/diff/2-3/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 59194: Validate DESTROY operation in `Resources::apply()`.
> On May 15, 2017, 5:41 a.m., Jiang Yan Xu wrote: > > src/tests/resources_tests.cpp > > Lines 2577 (patched) > > <https://reviews.apache.org/r/59194/diff/2/?file=1717738#file1717738line2577> > > > > `__total`? That is a left over from the original version. Fixed it. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59194/#review174924 ------- On May 15, 2017, 6:01 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59194/ > --- > > (Updated May 15, 2017, 6:01 p.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-7403 > https://issues.apache.org/jira/browse/MESOS-7403 > > > Repository: mesos > > > Description > --- > > Ensure that applying `DESTROY` operation removes this volume. This is > needed to handle `DESTROY` of shared volumes to ensure that the > resultant `Resources` object has this `Resource` removed (i.e. we > allow this operation to succeed if the original `Resources` object > only had a single copy of the shared resource). > > > Diffs > - > > src/common/resources.cpp f6f02eb0a7ca05ab8fd9a670f32428862009b7d5 > src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be > src/v1/resources.cpp cad069defb34d5ccc20a0e083eb88cb80f70e415 > > > Diff: https://reviews.apache.org/r/59194/diff/3/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 58486: Update the allocator on a per offer operation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58486/ --- (Updated May 15, 2017, 6:01 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- The allocator is updated for each offer operation in the order specified in the `ACCEPT` call. Once the allocator is updated successfully, we handle subsequent operations as follows: 1) Launch or Launch Group: We launch the tasks or task group on the agent. 2) (Un)reservation or Create/Destroy of Persistent Volumes: We send the `CheckPointResourcesMessage` to the agent. If allocation for any of the operations fail, those resources are not recovered from the allocator. Diffs (updated) - src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad Diff: https://reviews.apache.org/r/58486/diff/6/ Changes: https://reviews.apache.org/r/58486/diff/5-6/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 59194: Validate DESTROY operation in `Resources::apply()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59194/ --- (Updated May 15, 2017, 6:01 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-7403 https://issues.apache.org/jira/browse/MESOS-7403 Repository: mesos Description (updated) --- Ensure that applying `DESTROY` operation removes this volume. This is needed to handle `DESTROY` of shared volumes to ensure that the resultant `Resources` object has this `Resource` removed (i.e. we allow this operation to succeed if the original `Resources` object only had a single copy of the shared resource). Diffs (updated) - src/common/resources.cpp f6f02eb0a7ca05ab8fd9a670f32428862009b7d5 src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be src/v1/resources.cpp cad069defb34d5ccc20a0e083eb88cb80f70e415 Diff: https://reviews.apache.org/r/59194/diff/3/ Changes: https://reviews.apache.org/r/59194/diff/2-3/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 59195: Ensure that allocator can be updated before committing changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59195/ --- (Updated May 13, 2017, 4:51 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- In addition to `apply()` on `offeredResources`, we ensure this operation can be applied on `slave.allocated`. It is is so because certain operations require that they are applicable to the agent's allocated resources as a whole. Note that we return a failed future on the first error (and there may be other operations which might have failed). However, we do not modify the state of the allocator if any of the operations fail. Diffs (updated) - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/59195/diff/2/ Changes: https://reviews.apache.org/r/59195/diff/1-2/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 58486: Update the allocator on a per offer operation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58486/ --- (Updated May 13, 2017, 4:51 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- The allocator is updated for each offer operation in the order specified in the `ACCEPT` call. Once the allocator is updated successfully, we handle subsequent operations as follows: 1) Launch or Launch Group: We launch the tasks or task group on the agent. 2) (Un)reservation or Create/Destroy of Persistent Volumes: We send the `CheckPointResourcesMessage` to the agent. If allocation for any of the operations fail, those resources are not recovered from the allocator. Diffs (updated) - src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad Diff: https://reviews.apache.org/r/58486/diff/5/ Changes: https://reviews.apache.org/r/58486/diff/4-5/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 59194: Validate DESTROY operation in `Resources::apply()`.
> On May 12, 2017, 6:19 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp > > Lines 1417 (patched) > > <https://reviews.apache.org/r/59194/diff/1/?file=1715965#file1715965line1417> > > > > If this check fails we would return an Error directly so doesn't look > > like we need this temp var? Yes, that is right. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59194/#review174772 ------- On May 13, 2017, 4:50 a.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59194/ > --- > > (Updated May 13, 2017, 4:50 a.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-7403 > https://issues.apache.org/jira/browse/MESOS-7403 > > > Repository: mesos > > > Description > --- > > Ensure that applying `DESTROY` operation removes this volume. This is > needed to handle `DESTROY` of shared volumes to ensure that the > resultant `Resources` object has this `Resource` removed (i.e. we > allow this operation to succeed if the original `Resources` object > had a single copy of the shared resource). > > > Diffs > - > > src/common/resources.cpp f6f02eb0a7ca05ab8fd9a670f32428862009b7d5 > src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be > src/v1/resources.cpp cad069defb34d5ccc20a0e083eb88cb80f70e415 > > > Diff: https://reviews.apache.org/r/59194/diff/2/ > > > Testing > --- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 59194: Validate DESTROY operation in `Resources::apply()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59194/ --- (Updated May 13, 2017, 4:50 a.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-7403 https://issues.apache.org/jira/browse/MESOS-7403 Repository: mesos Description (updated) --- Ensure that applying `DESTROY` operation removes this volume. This is needed to handle `DESTROY` of shared volumes to ensure that the resultant `Resources` object has this `Resource` removed (i.e. we allow this operation to succeed if the original `Resources` object had a single copy of the shared resource). Diffs (updated) - src/common/resources.cpp f6f02eb0a7ca05ab8fd9a670f32428862009b7d5 src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be src/v1/resources.cpp cad069defb34d5ccc20a0e083eb88cb80f70e415 Diff: https://reviews.apache.org/r/59194/diff/2/ Changes: https://reviews.apache.org/r/59194/diff/1-2/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57935/ --- (Updated May 11, 2017, 9:02 p.m.) Review request for mesos and Neil Conway. Changes --- Rebased. Bugs: MESOS-7138 https://issues.apache.org/jira/browse/MESOS-7138 Repository: mesos Description --- In `DRFSorter::update`, we should set the `dirty` flag only when the total scalar quantities have changed in any of the cleints in the hierarchy, and not always. Diffs (updated) - src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7 Diff: https://reviews.apache.org/r/57935/diff/3/ Changes: https://reviews.apache.org/r/57935/diff/2-3/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 58486: Update the allocator on a per offer operation.
We should capture the failure at > > the individual step and construct an error message/log that's explicit > > about it. Updated in https://reviews.apache.org/r/59195/. > On April 20, 2017, 6:17 p.m., Jiang Yan Xu wrote: > > src/master/master.cpp > > Lines 4774-4775 (patched) > > <https://reviews.apache.org/r/58486/diff/2/?file=1693992#file1693992line4775> > > > > So if the failure has occurred, the allocation in the allocator is > > already wrong. e.g., it can have both a volume of 1MB (which didn't get > > removed) and the plain disk of 1MB (which got added assuming the volume was > > removed). > > > > Ideally when a failure happens it leaves the allocator in the previous > > good state but here we are making an attempt to asynchronously compensate > > that and to correct it. Many events can be executed on the allocator > > between the two. > > > > While it's true that no new supposedly destroyed volumes can be offered > > out due to being removed from `slave.total` in the allocator, the allocator > > is itself running and the framework's incorrect allocation can cause issue, > > e.g., fairness, although there are probably more severe problems. Updated based on the description of the new revision. - Anindya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58486/#review172448 --- On May 11, 2017, 8:21 p.m., Anindya Sinha wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58486/ > --- > > (Updated May 11, 2017, 8:21 p.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-7308 > https://issues.apache.org/jira/browse/MESOS-7308 > > > Repository: mesos > > > Description > --- > > The allocator is updated for each offer operation in the order specified > in the `ACCEPT` call. Once the allocator is updated successfully, we > handle subsequent operations as follows: > > 1) Launch or Launch Group: We launch the tasks or task group on the >agent. > 2) (Un)reservation or Create/Destroy of Persistent Volumes: We send >the `CheckPointResourcesMessage` to the agent. > > If allocation for any of the operations fail, those resources are not > recovered from the allocator. > > > Diffs > - > > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad > > > Diff: https://reviews.apache.org/r/58486/diff/4/ > > > Testing > --- > > Tests passed. > > > Thanks, > > Anindya Sinha > >
Re: Review Request 58486: Update the allocator on a per offer operation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58486/ --- (Updated May 11, 2017, 8:21 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Summary (updated) - Update the allocator on a per offer operation. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description (updated) --- The allocator is updated for each offer operation in the order specified in the `ACCEPT` call. Once the allocator is updated successfully, we handle subsequent operations as follows: 1) Launch or Launch Group: We launch the tasks or task group on the agent. 2) (Un)reservation or Create/Destroy of Persistent Volumes: We send the `CheckPointResourcesMessage` to the agent. If allocation for any of the operations fail, those resources are not recovered from the allocator. Diffs (updated) - src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad Diff: https://reviews.apache.org/r/58486/diff/4/ Changes: https://reviews.apache.org/r/58486/diff/3-4/ Testing --- Tests passed. Thanks, Anindya Sinha
Review Request 59194: Validate DESTROY operation in `Resources::apply()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59194/ --- Review request for mesos, James Peach and Jiang Yan Xu. Bugs: MESOS-7403 https://issues.apache.org/jira/browse/MESOS-7403 Repository: mesos Description --- Ensure that applying `DESTROY` operation removes this volume. This is needed to handle `DESTROY` of shared volumes to ensure that the resulted `Resources` object has this `Resource` removed. Diffs - src/common/resources.cpp f6f02eb0a7ca05ab8fd9a670f32428862009b7d5 src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be src/v1/resources.cpp cad069defb34d5ccc20a0e083eb88cb80f70e415 Diff: https://reviews.apache.org/r/59194/diff/1/ Testing --- All tests passed. Thanks, Anindya Sinha
Review Request 59195: Ensure that allocator can be updated before committing changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59195/ --- Review request for mesos, James Peach and Jiang Yan Xu. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- In addition to `apply()` on `offeredResources`, we ensure this operation can be applied on `slave.allocated`. It is is so because certain operations require that they are applicable to the agent's allocated resources as a whole. Note that we return a failed future on the first error (and there may be other operations which might have failed). However, we do not modify the state of the allocator if any of the operations fail. Diffs - include/mesos/allocator/allocator.hpp dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 src/master/allocator/mesos/allocator.hpp 119b461136123229f85ed3c3cfd41137974a6b9b src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 src/tests/hierarchical_allocator_tests.cpp 08180b9975869de328f0c095dd3cddf0c84fbecf Diff: https://reviews.apache.org/r/59195/diff/1/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 58487: Fix allocation quantities when shared resources are removed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58487/ --- (Updated May 11, 2017, 6:13 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- When shared resources are removed from the `allocations` in the sorter, we remove the scalar quantity only when the shared resource is actually removed (i.e. shared count goes down to 0). Similarly, we increase the scalar quantity only when this is a new shared resource that is being added to the `allocations`. Diffs - src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff Diff: https://reviews.apache.org/r/58487/diff/3/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 57815: Added `deactivated_roles` field in `FrameworkInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57815/ --- (Updated May 6, 2017, 10:30 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- This field is a subset of roles the framework registered as for which the framework does not want any resources offere to. Diffs (updated) - include/mesos/mesos.proto 1935f47a52840f6b395ecb2d2829551fa691 include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7 Diff: https://reviews.apache.org/r/57815/diff/5/ Changes: https://reviews.apache.org/r/57815/diff/4-5/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated May 6, 2017, 10:30 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/master_tests.cpp ceee2f4a5d38e0f4200f444769e058d2173de821 src/tests/scheduler_tests.cpp 0f5d9ada6eb880379baf5f106fd2d5b12e9738db Diff: https://reviews.apache.org/r/57818/diff/7/ Changes: https://reviews.apache.org/r/57818/diff/6-7/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for deactivated roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated May 6, 2017, 10:30 p.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the framework indicates a subset of roles to be deactivated, the allocator does not offer resources for those roles to such frameworks. In addition, the master validates the deactivated roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad src/tests/hierarchical_allocator_tests.cpp 33d5c0ea0182e09b3f3f30d20a33d46c23d81697 Diff: https://reviews.apache.org/r/57817/diff/7/ Changes: https://reviews.apache.org/r/57817/diff/6-7/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 59038: Fixed flakiness in HierarchicalAllocatorTest.NestedRoleDRF.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59038/#review174119 --- Fix it, then Ship it! Ship It! src/tests/hierarchical_allocator_tests.cpp Line 745 (original), 745 (patched) <https://reviews.apache.org/r/59038/#comment247219> nitpik: Maybe change the order of the comment to reflect that the 2 frameworks are added in roles "a/c" and "d/e" respectively followed by adding of the slave. src/tests/hierarchical_allocator_tests.cpp Line 783 (original), 783 (patched) <https://reviews.apache.org/r/59038/#comment247221> nitpik: Similar comment as the previous one. - Anindya Sinha On May 6, 2017, 12:50 a.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59038/ > --- > > (Updated May 6, 2017, 12:50 a.m.) > > > Review request for mesos, Anindya Sinha and Michael Park. > > > Bugs: MESOS-7462 > https://issues.apache.org/jira/browse/MESOS-7462 > > > Repository: mesos > > > Description > --- > > Fixed flakiness in HierarchicalAllocatorTest.NestedRoleDRF. > > > Diffs > - > > src/tests/hierarchical_allocator_tests.cpp > 33d5c0ea0182e09b3f3f30d20a33d46c23d81697 > > > Diff: https://reviews.apache.org/r/59038/diff/1/ > > > Testing > --- > > `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose > --gtest_repeat=1000 --gtest_break_on_failure .` > > > Thanks, > > Neil Conway > >
Re: Review Request 57816: Add a scheduler flag `suppress_offers_on_registration`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57816/ --- (Updated May 5, 2017, 2:21 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- If the framework sets this flag to indicate that it wants offers suppressed on successful registration to master, we pass this information to the master in the `SUBSCRIBE` call. Diffs - src/sched/flags.hpp d19d20bad7dba48c8792783c73115affa1430cbe src/sched/sched.cpp ef73c1dccfd736b79f40a057951f022df7f60644 Diff: https://reviews.apache.org/r/57816/diff/4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57817: Offers not sent for deactivated roles as indicated in `SUBSCRIBE`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57817/ --- (Updated May 5, 2017, 12:34 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Modifications based on discussion in Slack. Summary (updated) - Offers not sent for deactivated roles as indicated in `SUBSCRIBE`. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description (updated) --- If the framework indicates a subset of roles to be deactivated, the allocator does not offer resources for those roles to such frameworks. In addition, the master validates the deactivated roles to be a subset of all the roles being (re)registered by the framework. Diffs (updated) - src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d src/master/allocator/mesos/hierarchical.hpp 123f97cf495bff0f822838e09df0d88818f04da6 src/master/allocator/mesos/hierarchical.cpp b75ed9a20a9a42f958cebbacd91e5e15b0d21394 src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad src/tests/hierarchical_allocator_tests.cpp ebc4868a6b7e2a04cc202a74a4633969ade40aca Diff: https://reviews.apache.org/r/57817/diff/6/ Changes: https://reviews.apache.org/r/57817/diff/5-6/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57818/ --- (Updated May 5, 2017, 12:34 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Modifications based on discussion in Slack. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description --- Added unit tests to verify offers are suppressed based on registration. Diffs (updated) - src/tests/master_tests.cpp ceee2f4a5d38e0f4200f444769e058d2173de821 src/tests/scheduler_tests.cpp 0f5d9ada6eb880379baf5f106fd2d5b12e9738db Diff: https://reviews.apache.org/r/57818/diff/6/ Changes: https://reviews.apache.org/r/57818/diff/5-6/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57815: Added `deactivated_roles` field in `FrameworkInfo`.
> On April 17, 2017, 6:28 p.m., Vinod Kone wrote:
> > include/mesos/scheduler/scheduler.proto
> > Lines 249-250 (original), 249-250 (patched)
> > <https://reviews.apache.org/r/57815/diff/1-3/?file=1670985#file1670985line249>
> >
> > Hmm, I was hoping that we could re-use the `Call::Suppress` message
> > here instead of re-defining it. That way any changes to suppress would be
> > in sync.
>
> Anindya Sinha wrote:
> Currently, `Call::Suppress` is:
> ```
> message Suppress {
> optional string role = 1;
> }
> ```
>
> So, the `SUPPRESS` call currently allows suppressing offers for a
> framework on a per-role basis (or all roles if `role` is not set). Since we
> now support multiple roles for a frameework, and if we want to suppress
> offers for a subset of roles at the time of `SUBSCRIBE`, using
> `Call::Suppress` would not satisfy that use case. It would only satisfy
> suppressing for all roles (i.e. `Suppress::role` is not set) or for a
> specific role.
>
> Is there any plans of extending `SUPPRESS` to suppress offers for a
> subset of roles (>0, and < all roles) in a single call? Do you think we
> should consider that also?
>
> One such approach could be to have the following:
> ```
> message Suppress {
> repeated string roles = 1;
> }
>
> optional Suppress suppress;
> ```
>
> If suppress is not set, then we do not suppress for any roles. Otherwise,
> we suppress for the set of roles specified in `suppress.roles`. If `suppress`
> is set but `suppress.roles` is empty, we suppress for all roles of the
> framework.
>
> Thoughts?
So based on discussion in the Slack channel, this modified review chain does
the following:
1. Add `repeated string deactivated_roles` to `FrameworkInfo` which represents
a subset of roles which are deactivated. Offers pertaining to the deactivated
roles shall not be sent out.
2. `SUPPRESS` and `REVIVE` calls will toggle the (de)activation mode of the
roles.
3. Allocator's `activateFramework()` call will activate all roles of the
framework which are not in `FrameworkInfo::deactivated+roles`. Similarly, in
`deactivateFramework()` call will deactivate all roles that are not deactivated
already.
- Anindya
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57815/#review172115
---
On May 5, 2017, 12:34 a.m., Anindya Sinha wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57815/
> ---
>
> (Updated May 5, 2017, 12:34 a.m.)
>
>
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This field is a subset of roles the framework registered as for which
> the framework does not want any resources offere to.
>
>
> Diffs
> -
>
> include/mesos/mesos.proto 1935f47a52840f6b395ecb2d2829551fa691
> include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
>
>
> Diff: https://reviews.apache.org/r/57815/diff/4/
>
>
> Testing
> ---
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57815: Added `deactivated_roles` field in `FrameworkInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57815/ --- (Updated May 5, 2017, 12:34 a.m.) Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu. Changes --- Addressed review comments based on discussion on Slack. Summary (updated) - Added `deactivated_roles` field in `FrameworkInfo`. Bugs: MESOS-7015 https://issues.apache.org/jira/browse/MESOS-7015 Repository: mesos Description (updated) --- This field is a subset of roles the framework registered as for which the framework does not want any resources offere to. Diffs (updated) - include/mesos/mesos.proto 1935f47a52840f6b395ecb2d2829551fa691 include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7 Diff: https://reviews.apache.org/r/57815/diff/4/ Changes: https://reviews.apache.org/r/57815/diff/3-4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57964: Added a test to verify metrics when shared resources are present.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57964/ --- (Updated May 2, 2017, 5:54 p.m.) Review request for mesos and Jiang Yan Xu. Changes --- Addressed comments. Bugs: MESOS-7186 https://issues.apache.org/jira/browse/MESOS-7186 Repository: mesos Description --- Added a test to verify metrics when shared resources are present. Diffs (updated) - src/tests/persistent_volume_tests.cpp d42fd18fd5b36f5970e91e60b84e839aeedfc34b Diff: https://reviews.apache.org/r/57964/diff/4/ Changes: https://reviews.apache.org/r/57964/diff/3-4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 57963: Metrics for used resources should incorporate shared resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57963/ --- (Updated May 2, 2017, 5:53 p.m.) Review request for mesos and Jiang Yan Xu. Changes --- Addressed comments. Bugs: MESOS-7186 https://issues.apache.org/jira/browse/MESOS-7186 Repository: mesos Description --- The following metrics take the shared counts of shared resources into account in determination of the actual amount of used resources: a) `master/_used` b) `master/_revocable_used` c) `slave/_used` d) `slave/_revocable_used` Diffs (updated) - src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a Diff: https://reviews.apache.org/r/57963/diff/4/ Changes: https://reviews.apache.org/r/57963/diff/3-4/ Testing --- All tests passed. Thanks, Anindya Sinha
Re: Review Request 58487: Fix allocation quantities when shared resources are removed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58487/ --- (Updated April 28, 2017, 3:42 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- When shared resources are removed from the `allocations` in the sorter, we remove the scalar quantity only when the shared resource is actually removed (i.e. shared count goes down to 0). Similarly, we increase the scalar quantity only when this is a new shared resource that is being added to the `allocations`. Diffs (updated) - src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff Diff: https://reviews.apache.org/r/58487/diff/3/ Changes: https://reviews.apache.org/r/58487/diff/2-3/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 58486: Fixed a race in `updateAllocation()` on DESTORY of a shared volume.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58486/ --- (Updated April 28, 2017, 3:42 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Yet to address the review comments. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- In `updateAllocation()`, it is possible for the flattened scalar quantites of original framework allocation to differ from the updated framework allocation. This can happen when an offer with a shared persistent volume is sent out after a DESTROY has been received. If that is the case, we rescind the pending offers from all frameworks which contain the volumes to be destroyed. Diffs (updated) - include/mesos/allocator/allocator.hpp 6eda1b8619269c1501a935045b18b1deaf845b33 src/master/allocator/mesos/allocator.hpp 57b54b86c43c7731e64d422d285c4b8ca7e27a60 src/master/allocator/mesos/hierarchical.hpp 219f508dacb3b372c12da3f15e07a3bf5f27e6e6 src/master/allocator/mesos/hierarchical.cpp 84dc31dd6e567e9316a73c61e03a1daf6f556829 src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 src/master/master.cpp e8c2a96ff3407fb429e60cd9e66a8c4dc52b391b src/tests/allocator.hpp 6b71c574e0e4facd1795ef50ee0869c03b87833d src/tests/hierarchical_allocator_tests.cpp 84bb6f302f6ff6f8a93fa891795df9ef7380629f Diff: https://reviews.apache.org/r/58486/diff/3/ Changes: https://reviews.apache.org/r/58486/diff/2-3/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 58485: Avoid a corruption while rescinding offers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58485/ --- (Updated April 28, 2017, 3:42 p.m.) Review request for mesos, James Peach and Jiang Yan Xu. Changes --- Rebased. Bugs: MESOS-7308 https://issues.apache.org/jira/browse/MESOS-7308 Repository: mesos Description --- When a `DESTROY` is processed, we rescind all pending offers to which persistent volumes have been offered. As soon as we rescind an offer, the `Offer` object is freed; and hence, we should move on to the next offer (even though there are multiple volumes being destroyed in the same `ACCEPT` call). Diffs (updated) - src/master/master.cpp e8c2a96ff3407fb429e60cd9e66a8c4dc52b391b src/tests/persistent_volume_tests.cpp 3eb7afe3de8e72ffb6502dfe12ef37ccd4ca2125 Diff: https://reviews.apache.org/r/58485/diff/3/ Changes: https://reviews.apache.org/r/58485/diff/2-3/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 57535: Applied RegisterAgent ACL to the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57535/#review173227 --- Ship it! Ship It! - Anindya Sinha On April 25, 2017, 5:30 p.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57535/ > --- > > (Updated April 25, 2017, 5:30 p.m.) > > > Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin > Mahler, Greg Mann, and Vinod Kone. > > > Bugs: MESOS-7097 > https://issues.apache.org/jira/browse/MESOS-7097 > > > Repository: mesos > > > Description > --- > > Applied RegisterAgent ACL to the master. > > > Diffs > - > > src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 > src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 > src/tests/master_authorization_tests.cpp > 1a0285a3f345ef21a8256d4123d8bb684f538da4 > > > Diff: https://reviews.apache.org/r/57535/diff/8/ > > > Testing > --- > > make check. > > The tests added here cover some basic scenarios, I have more tests but will > add them when MESOS-7244 is fixed. > > > Thanks, > > Jiang Yan Xu > >
Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.
> On April 26, 2017, 7:26 p.m., Neil Conway wrote:
> > Hey Anindya -- can you rebase this RR for the recent sorter changes? I
> > should be able to review and commit it shortly after that.
Rebased this patch.
btw, `SorterTest.HierarchicalAllocation()` has the following call to
`sorter.update()`:
```
sorter.update("b/c", slaveId, cResources, cNewResources);
```
which updates the sorter from `cResources` of `cpus(*):4;mem(*):4` to
`cNewResources` of `cpus(*):1;mem(*):1`. This obviously changes the `totals`
and hence the `dirty` flag needs to be set.
However, `DRFSorter::update()` is only called from
`HierarchicalAllocatorProcess::updateAllocation()` for each of the role sorters
and IIUC, there should not be a case when the `totals` would actually change.
So, I was of the opinion of putting in a check invariant in
`Node::Allocation::update()` but obviously that would be a problem for this
test. So, as of now, I added a check that if `totals` changes, we set the
`dirty` flag.
- Anindya
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review173081
---
On April 27, 2017, 5:35 p.m., Anindya Sinha wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> ---
>
> (Updated April 27, 2017, 5:35 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> ---
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the cleints in the
> hierarchy, and not always.
>
>
> Diffs
> -
>
> src/master/allocator/sorter/drf/sorter.hpp
> fee58d6d1f08163e2a06a4a20c891fe535c3dcff
> src/master/allocator/sorter/drf/sorter.cpp
> a8b35c4deac7a4f0725ccae517a8cbca5b8e1ee7
>
>
> Diff: https://reviews.apache.org/r/57935/diff/2/
>
>
> Testing
> ---
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57935/ --- (Updated April 27, 2017, 5:35 p.m.) Review request for mesos and Neil Conway. Changes --- Rebased on top of changes in hierarchical roles implementation. Summary (updated) - Recalculate shares only when total scalar quantities have changed. Bugs: MESOS-7138 https://issues.apache.org/jira/browse/MESOS-7138 Repository: mesos Description (updated) --- In `DRFSorter::update`, we should set the `dirty` flag only when the total scalar quantities have changed in any of the cleints in the hierarchy, and not always. Diffs (updated) - src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff src/master/allocator/sorter/drf/sorter.cpp a8b35c4deac7a4f0725ccae517a8cbca5b8e1ee7 Diff: https://reviews.apache.org/r/57935/diff/2/ Changes: https://reviews.apache.org/r/57935/diff/1-2/ Testing --- All tests passed. Thanks, Anindya Sinha
Review Request 58765: Fix build failure introduced in 8990ebb.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58765/ --- Review request for mesos, Vinod Kone and Jiang Yan Xu. Repository: mesos Description --- Fix build failure introduced in 8990ebb. Diffs - 3rdparty/libprocess/src/process.cpp 6d4c5022b5eef271cd40d131aeef13362209eb3e Diff: https://reviews.apache.org/r/58765/diff/1/ Testing --- Tests passed. Thanks, Anindya Sinha
Re: Review Request 58676: Logged when an agent (re-)registration request is received.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58676/#review172960 --- Ship it! Ship It! - Anindya Sinha On April 25, 2017, 5:33 p.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58676/ > --- > > (Updated April 25, 2017, 5:33 p.m.) > > > Review request for mesos, Anindya Sinha and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > - This log happens after the master has done initial validation but >before authorization, which is consistent with the (re-)register >framework code paths. > > > Diffs > - > > src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 > > > Diff: https://reviews.apache.org/r/58676/diff/1/ > > > Testing > --- > > make check. > > > Thanks, > > Jiang Yan Xu > >
Re: Review Request 57534: Added and implemented RegisterAgent ACL.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57534/#review172739 --- Ship it! Ship It! - Anindya Sinha On March 28, 2017, 8:24 a.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57534/ > --- > > (Updated March 28, 2017, 8:24 a.m.) > > > Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, and Greg > Mann. > > > Bugs: MESOS-7097 > https://issues.apache.org/jira/browse/MESOS-7097 > > > Repository: mesos > > > Description > --- > > Added and implemented RegisterAgent ACL. > > > Diffs > - > > include/mesos/authorizer/acls.proto > ae10027eb716d4dcdeddf924223bcd4faed36de9 > include/mesos/authorizer/authorizer.proto > 736f76d552956f2351ffd40fc51d088dff83f8c8 > src/authorizer/local/authorizer.cpp > 1c1f912794cfe61112a0e513b217ba3a755f35f1 > src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d > > > Diff: https://reviews.apache.org/r/57534/diff/7/ > > > Testing > --- > > make check. > > > Thanks, > > Jiang Yan Xu > >
Re: Review Request 57731: Fixed a test in MasterAuthorizationTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57731/#review172738 --- Ship it! Ship It! - Anindya Sinha On March 17, 2017, 3:55 p.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57731/ > --- > > (Updated March 17, 2017, 3:55 p.m.) > > > Review request for mesos, Anindya Sinha and Vinod Kone. > > > Bugs: MESOS-7097 > https://issues.apache.org/jira/browse/MESOS-7097 > > > Repository: mesos > > > Description > --- > > - `MasterAuthorizationTest.PendingExecutorInfoDiffersOnDifferentSlaves` > used to assume the mock authorizer is only called for tasks > authorization but with the new `regsiter_agents` ACL this is no > longer true. > > > Diffs > - > > src/tests/master_authorization_tests.cpp > 1a0285a3f345ef21a8256d4123d8bb684f538da4 > > > Diff: https://reviews.apache.org/r/57731/diff/4/ > > > Testing > --- > > make check. > > > Thanks, > > Jiang Yan Xu > >
