Review Request 73004: Documented setting offer constraints via the scheduler API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73004/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-10193 https://issues.apache.org/jira/browse/MESOS-10193 Repository: mesos Description --- Documented setting offer constraints via the scheduler API. Diffs - docs/app-framework-development-guide.md 8f30045b04d1a8f3c0d078ff79e65ba9c1e7c94a docs/scheduler-http-api.md e447ffb43253fd927bdb266ad12a4554c69e0ee8 Diff: https://reviews.apache.org/r/73004/diff/1/ Testing --- Tested manually how Github renders this: https://github.com/asekretenko/mesos/blob/offer_constraints_doc/docs/app-framework-development-guide.md https://github.com/asekretenko/mesos/blob/offer_constraints_doc/docs/scheduler-http-api.md Thanks, Andrei Sekretenko
Re: Review Request 72955: Consolidated creation and validation of `allocator::Framework` options.
> On Oct. 15, 2020, 9:27 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 2615-2616 (patched)
> > <https://reviews.apache.org/r/72955/diff/3/?file=2240753#file2240753line2627>
> >
> > It seems unfortunate that validatation of the suppressed roles is
> > buried within creating the allocator framework options, which leads to it
> > being modeled as a "failure to create the options" rather than a
> > "validation error". It's less clear if the failure is due to invalidness or
> > some kind of error on our part.
> >
> > The explicit approach makes it clear:
> >
> > ```
> > Option e = validateFramework(info);
> >
> > ...
> >
> > // Validate the other fields of the subscription:
> > e = validateSuppressedRoles(info, suppressedRoles);
> > ...
> > e = validateOfferConstraints(info, offerConstraints);
> >
> > make allocator options w/ hard failure if not valid
> > ```
> >
> > Also, by containing all of the API validation within our validation
> > headers we can unit test all of it, and for integration tests we only need
> > to test that the functions are called (rather than testing all invalid
> > cases via the integration tests). That was the idea when we started to
> > contain it all within validation headers.
> >
> > Just the thought I had while reading this, not suggesting that you
> > necessarily change this.
Left it as it is for now.
Probably it will make sense to revisit this location if, for example, the
stateless validation of `FrameworkInfo` becomes better separated from
validation against Master state.
Fully agree that the fact that **all** errors returned by
`createAllocatorFrameworkOptions()` (and by `OfferConstraintsFilter::create()`,
for that matter!) are **validation errors** is relatively non-straightforward.
Note that this approach was chosen mainly because we in any case need an
intermediate representation due to the need to construct RE2s; thus, all the
locations in the code that can encounter invalid offer constraints are
localized in the IR construction.
In this case, having a validation code that would simply mirror construction
(and will construct RE2s one more time) made no sense.
I would say that this is not the case in many other data processing paths in
Mesos, where validation doesn't really mirror any code that is using the
validated data afterwards.
In these cases, validation still needs to be kept in sync with what follows
next, but we cannot realistically avoid this, as it is either expensive, or
outright impossible to guard against an invalid input later on, or unreasonably
difficult to propagate an error backwards.
The offer constraints IR construction is not among these cases, though.
> On Oct. 15, 2020, 9:27 p.m., Benjamin Mahler wrote:
> > src/master/validation.cpp
> > Lines 692-693 (patched)
> > <https://reviews.apache.org/r/72955/diff/3/?file=2240755#file2240755line692>
> >
> > don't know if we need the quotes here, since we're not showing a string
> > field:
> >
> > `Suppressed roles {"foo", "bar"} are not ...`
We don't need them; fixed this.
- Andrei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72955/#review222060
---
On Oct. 27, 2020, 8:38 p.m., Andrei Sekretenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72955/
> ---
>
> (Updated Oct. 27, 2020, 8:38 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10176
> https://issues.apache.org/jira/browse/MESOS-10176
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This merges three near-identical pieces of scattered code in SUBSCRIBE
> and UPDATE_FRAMEWORK execution paths in the Master that validate
> and construct parts of `allocator::FrameworkOptions` (the set of
> suppressed roles and the offer constraints filter) into a single
> function.
>
> This is a prerequisite to adding validation of offer constraint roles.
>
>
> Diffs
> -
>
> src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028
> src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188
> src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208
> src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb
>
>
> Diff: https://reviews.apache.org/r/72955/diff/4/
>
>
> Testing
> ---
>
> make check
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72955: Consolidated creation and validation of `allocator::Framework` options.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72955/ --- (Updated Oct. 27, 2020, 8:38 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Amended the comment for `validateFramework`, removed redundant quotes. Bugs: MESOS-10176 https://issues.apache.org/jira/browse/MESOS-10176 Repository: mesos Description --- This merges three near-identical pieces of scattered code in SUBSCRIBE and UPDATE_FRAMEWORK execution paths in the Master that validate and construct parts of `allocator::FrameworkOptions` (the set of suppressed roles and the offer constraints filter) into a single function. This is a prerequisite to adding validation of offer constraint roles. Diffs (updated) - src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb Diff: https://reviews.apache.org/r/72955/diff/4/ Changes: https://reviews.apache.org/r/72955/diff/3-4/ Testing --- make check Thanks, Andrei Sekretenko
Re: Review Request 72956: Added validation that offer constraints are set only for existing roles.
> On Oct. 15, 2020, 11:22 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Line 2622 (original), 2622-2624 (patched)
> > <https://reviews.apache.org/r/72956/diff/2/?file=2240748#file2240748line2622>
> >
> > ```
> > Make sure the offer constraints don't use roles other than the
> > framework's roles.
> > ```
> >
> > That seems sufficient for the reader? I'm not sure they need to be
> > explained here that you could construct a "valid" OfferConstraintsFilter
> > with roles outside of the framework's roles
Fully agree that my previous version probably doesn't add much value. On the
other hand, the shorter version still has to be kept it in sync with the code
of `validateOfferConstraintsRoles()`.
Removed this comment altogether, hopefully the function name is descriptive
enough.
> On Oct. 15, 2020, 11:22 p.m., Benjamin Mahler wrote:
> > src/master/validation.hpp
> > Lines 137-139 (patched)
> > <https://reviews.apache.org/r/72956/diff/2/?file=2240749#file2240749line137>
> >
> > Similar to the last review, just want to comment on how these are unit
> > testable. In this case, I'm not sure how we could avoid the integration
> > test though since it's not a sub-case of validation where we can just
> > ensure in the integration test that overall validation is called.
> >
> > Still, might want to just add the simple unit test for this?
I'll probably send a follow-up patch with unit tests, for this, and also for
the suppressed roles validation.
> On Oct. 15, 2020, 11:22 p.m., Benjamin Mahler wrote:
> > src/master/validation.cpp
> > Lines 706 (patched)
> > <https://reviews.apache.org/r/72956/diff/2/?file=2240750#file2240750line706>
> >
> > could just use `foreachkey (`
> >
> > ```
> > foreachkey (const string& role, offerConstraints.role_constraints()) {
> >
> > }
> > ```
Unfortunatley, there is no straightforward way to use a protobuf map with
`foreachkey()`.
This macro employs `std::get<>`, which is not overloaded for protobuf's
`MapPair`.
- Andrei
-----------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72956/#review222061
---
On Oct. 14, 2020, 8:18 p.m., Andrei Sekretenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72956/
> ---
>
> (Updated Oct. 14, 2020, 8:18 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10176
> https://issues.apache.org/jira/browse/MESOS-10176
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch makes SUBSCRIBE/UPDATE_FRAMEWORK calls validate that
> the framework does not specify offer constraints for roles to which
> it is not going to be subscribed.
>
>
> Diffs
> -
>
> src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188
> src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208
> src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb
> src/tests/master/update_framework_tests.cpp
> 3f86573e8dfeea63fe99064f2137c61d901f4fc7
>
>
> Diff: https://reviews.apache.org/r/72956/diff/3/
>
>
> Testing
> ---
>
> `make check`
> `src/mesos-tests
> --gtest_filter='*UpdateFrameworkTest.OfferConstraintsForUnsubscribedRole*'
> --gtest_break_on_failure --gtest_repeat=1000`
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On Sept. 25, 2020, 10:26 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 422-426 (patched)
> > <https://reviews.apache.org/r/72448/diff/3/?file=2236142#file2236142line423>
> >
> > Just `return approved(...)` ?
>
> Dong Zhu wrote:
> I got totally confused here, I have no idea what you mean. Can you
> develop a patch and post here that I can make a reference ?
I mean, why define a variable when you can just return the result of the
function call?
Instead of
```
T f(){
const T result = g();
return result;
}
```
you can just write
```
T f() {
return g();
}
```
?
- Andrei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72448/#review221963
---
On July 23, 2020, 11:03 a.m., Dong Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
>
> (Updated July 23, 2020, 11:03 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch intends to fix issue MESOS-10085.
>
> When the authorization failed happens master return nothing to the
> subscriber, subscriber isn't aware of what is happening, this issue
> can lead to inconsistencies in Event stream.
>
>
> Diffs
> -
>
> include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29
> include/mesos/v1/master/master.proto
> 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d
> src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641
> src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c
> src/tests/master/mock_master_api_subscriber.cpp
> 893d3e366164ccebd2847ed4c2874ab00e0e5b7b
>
>
> Diff: https://reviews.apache.org/r/72448/diff/3/
>
>
> Testing
> ---
>
> - Manually tested
> - make check
>
>
> Thanks,
>
> Dong Zhu
>
>
Re: Review Request 72956: Added validation that offer constraints are set only for existing roles.
> On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Line 2635 (original), 2635-2642 (patched) > > <https://reviews.apache.org/r/72956/diff/1/?file=2240634#file2240634line2635> > > > > Ditto here from the last review, shouldn't this go into the > > validation.hpp existing stateless validation? Tried to move this into a separate function in `validation.cpp`; not sure if this makes things better or worse. The thing with the validation of roles in constraints is that I want to be very clear that this is orthogonal to the internal validity of `OfferConstraints`. Hence, a stateless validation function like `validate(const FrameworkInfo&, const set& suppressedRoles, const OfferConstraints&)` would look very misleading (or, it will have to do the job of the filter regexp construction just the sake of constraints validation; this will also add the regex construction options to the signature). One more alternative would be to take the whole `createAllocatorFrameworkOptions()` out of `master.cpp`, which will make it unit-testable. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72956/#review222040 ------- On Oct. 14, 2020, 8:18 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72956/ > --- > > (Updated Oct. 14, 2020, 8:18 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10176 > https://issues.apache.org/jira/browse/MESOS-10176 > > > Repository: mesos > > > Description > --- > > This patch makes SUBSCRIBE/UPDATE_FRAMEWORK calls validate that > the framework does not specify offer constraints for roles to which > it is not going to be subscribed. > > > Diffs > - > > src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 > src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 > src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb > src/tests/master/update_framework_tests.cpp > 3f86573e8dfeea63fe99064f2137c61d901f4fc7 > > > Diff: https://reviews.apache.org/r/72956/diff/2/ > > > Testing > --- > > `make check` > `src/mesos-tests > --gtest_filter='*UpdateFrameworkTest.OfferConstraintsForUnsubscribedRole*' > --gtest_break_on_failure --gtest_repeat=1000` > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72955: Consolidated creation and validation of `allocator::Framework` options.
> On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.hpp > > Lines 1061-1062 (original), 1061-1062 (patched) > > <https://reviews.apache.org/r/72955/diff/1/?file=2240632#file2240632line1061> > > > > Maybe just say "based on master flags and the framework not being > > completed"? > > > > Also, this makes it sound like it only validates things against the > > master flags and the framework being non-completed, but it also validates > > somewhat that the FrameworkInfo is valid on its own, like the failover > > timeout. Should the failover timeout validation be moved to the existing > > stateless validation of FrameworkInfo? Moving the failover timeout into stateless validation is a right thing, sent a patch for that. Nevertheless, the very first thing this function does is calling the stateless validation, and I have no intention to change this now. I've reworded this comment change to avoid giving impression that stateless validation is performed separately. > On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2588-2598 (original) > > <https://reviews.apache.org/r/72955/diff/1/?file=2240633#file2240633line2589> > > > > Per above comment, did you mean to also move out the failover timeout > > validation, which looks to be a similarly stateless validation of the > > FrameworkInfo as this bit? > > Andrei Sekretenko wrote: > Thanks! I somehow missed that it is entirely stateless. Will send one > more small patch. Patch for moving failover timeout validation: https://reviews.apache.org/r/72964/ > On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2629-2634 (patched) > > <https://reviews.apache.org/r/72955/diff/1/?file=2240633#file2240633line2641> > > > > Hm.. shouldn't this stateless FrameworkInfo validation go in the > > existing `validation::framework::valdiate(const FrameworkInfo&)` function > > in master/validation.hpp? I would say the this validates not `FrameworkInfo` but rather suppressed roles, which we pass separately (and do not get from resubscribing agents, only via scheduler API). Now this patch moves this validation into a separated function in `validation.hpp`; not sure if it makes things better or worse, though. - Andrei ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72955/#review222039 --- On Oct. 14, 2020, 8:16 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72955/ > --- > > (Updated Oct. 14, 2020, 8:16 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10176 > https://issues.apache.org/jira/browse/MESOS-10176 > > > Repository: mesos > > > Description > --- > > This merges three near-identical pieces of scattered code in SUBSCRIBE > and UPDATE_FRAMEWORK execution paths in the Master that validate > and construct parts of `allocator::FrameworkOptions` (the set of > suppressed roles and the offer constraints filter) into a single > function. > > This is a prerequisite to adding validation of offer constraint roles. > > > Diffs > - > > src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 > src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 > src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 > src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb > > > Diff: https://reviews.apache.org/r/72955/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Andrei Sekretenko > >
Review Request 72964: Moved failover timeout validation to stateless FrameworkInfo validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72964/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10176 https://issues.apache.org/jira/browse/MESOS-10176 Repository: mesos Description --- This turns the validation of the failover timeout in `FrameworkInfo` into a part of `validation::framework::validate()` that performs all the other validations that depend on `FrameworkInfo` only. Diffs - src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb Diff: https://reviews.apache.org/r/72964/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72956: Added validation that offer constraints are set only for existing roles.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72956/ --- (Updated Oct. 14, 2020, 8:18 p.m.) Review request for mesos and Benjamin Mahler. Bugs: MESOS-10176 https://issues.apache.org/jira/browse/MESOS-10176 Repository: mesos Description --- This patch makes SUBSCRIBE/UPDATE_FRAMEWORK calls validate that the framework does not specify offer constraints for roles to which it is not going to be subscribed. Diffs (updated) - src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb src/tests/master/update_framework_tests.cpp 3f86573e8dfeea63fe99064f2137c61d901f4fc7 Diff: https://reviews.apache.org/r/72956/diff/2/ Changes: https://reviews.apache.org/r/72956/diff/1-2/ Testing --- `make check` `src/mesos-tests --gtest_filter='*UpdateFrameworkTest.OfferConstraintsForUnsubscribedRole*' --gtest_break_on_failure --gtest_repeat=1000` Thanks, Andrei Sekretenko
Re: Review Request 72955: Consolidated creation and validation of `allocator::Framework` options.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72955/ --- (Updated Oct. 14, 2020, 8:16 p.m.) Review request for mesos and Benjamin Mahler. Bugs: MESOS-10176 https://issues.apache.org/jira/browse/MESOS-10176 Repository: mesos Description --- This merges three near-identical pieces of scattered code in SUBSCRIBE and UPDATE_FRAMEWORK execution paths in the Master that validate and construct parts of `allocator::FrameworkOptions` (the set of suppressed roles and the offer constraints filter) into a single function. This is a prerequisite to adding validation of offer constraint roles. Diffs (updated) - src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb Diff: https://reviews.apache.org/r/72955/diff/2/ Changes: https://reviews.apache.org/r/72955/diff/1-2/ Testing --- make check Thanks, Andrei Sekretenko
Re: Review Request 72955: Consolidated creation and validation of `allocator::Framework` options.
> On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2588-2598 (original) > > <https://reviews.apache.org/r/72955/diff/1/?file=2240633#file2240633line2589> > > > > Per above comment, did you mean to also move out the failover timeout > > validation, which looks to be a similarly stateless validation of the > > FrameworkInfo as this bit? Thanks! I somehow missed that it is entirely stateless. Will send one more small patch. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72955/#review222039 ------- On Oct. 12, 2020, 10:12 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72955/ > --- > > (Updated Oct. 12, 2020, 10:12 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10176 > https://issues.apache.org/jira/browse/MESOS-10176 > > > Repository: mesos > > > Description > --- > > This merges three near-identical pieces of scattered code in SUBSCRIBE > and UPDATE_FRAMEWORK execution paths in the Master that validate > and construct parts of `allocator::FrameworkOptions` (the set of > suppressed roles and the offer constraints filter) into a single > function. > > This is a prerequisite to adding validation of offer constraint roles. > > > Diffs > - > > src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 > src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 > > > Diff: https://reviews.apache.org/r/72955/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Andrei Sekretenko > >
Review Request 72956: Added validation that offer constraints are set only for existing roles.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72956/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10176 https://issues.apache.org/jira/browse/MESOS-10176 Repository: mesos Description --- This patch makes SUBSCRIBE/UPDATE_FRAMEWORK calls validate that the framework does not specify offer constraints for roles to which it is not going to be subscribed. Diffs - src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 src/tests/master/update_framework_tests.cpp 3f86573e8dfeea63fe99064f2137c61d901f4fc7 Diff: https://reviews.apache.org/r/72956/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72955: Consolidated creation and validation of `allocator::Framework` options.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72955/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10176 https://issues.apache.org/jira/browse/MESOS-10176 Repository: mesos Description --- This merges three near-identical pieces of scattered code in SUBSCRIBE and UPDATE_FRAMEWORK execution paths in the Master that validate and construct parts of `allocator::FrameworkOptions` (the set of suppressed roles and the offer constraints filter) into a single function. This is a prerequisite to adding validation of offer constraint roles. Diffs - src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 Diff: https://reviews.apache.org/r/72955/diff/1/ Testing --- make check Thanks, Andrei Sekretenko
Review Request 72953: Made the offer constraints filter non-optional inside the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72953/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description --- Now that the master always constructs an offer constraints filter for a framework (potentially a no-op one) and always passes this filter into an allocator, storing the filter as an `Option` inside the hierarchical allocator is no longer necessary. Diffs - src/master/allocator/mesos/hierarchical.hpp 4ec15b64167efd75437c8edf107b94a984fbc9f6 src/master/allocator/mesos/hierarchical.cpp 35264b9d866c9b95b76fccde5cf3b438e3bae160 Diff: https://reviews.apache.org/r/72953/diff/1/ Testing --- `make check` Thanks, Andrei Sekretenko
Review Request 72954: Added suppressed roles to `allocator/offer_constraints_debug` endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72954/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description --- This simplifies debugging frameworks that use offer constraints. For example, in case a framework is not receiving offers for a role, getting a suppressed roles snapshot simultaneously with agent filtering results helps to figure out whether the framework is mis-specifying offer constraints, or just wrongly suppresses a role. Diffs - src/master/allocator/mesos/hierarchical.cpp 35264b9d866c9b95b76fccde5cf3b438e3bae160 Diff: https://reviews.apache.org/r/72954/diff/1/ Testing --- `make check` Thanks, Andrei Sekretenko
Re: Review Request 72845: Added doc for the `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72845/#review222019 --- Fix it, then Ship it! Looks almost good to go! Found two locations where more clarity would IMO be beneficial, and also a couple of very minor typo-like issues. docs/isolators/csi-volume.md Lines 51 (patched) <https://reviews.apache.org/r/72845/#comment311092> s/mount onto/mount them onto/? docs/isolators/csi-volume.md Lines 82 (patched) <https://reviews.apache.org/r/72845/#comment311093> Maybe more explicit: s/two flags/the `isolation` and `csi_plugin_config_dir` flags/ ? The values of `--master` and `--work_dir` are irrelevant for CSI configuration despite being needed to launch the agent, right? docs/isolators/csi-volume.md Lines 255-256 (patched) <https://reviews.apache.org/r/72845/#comment311094> Hmmm... this place is hard to grasp for a person not closely familiar with the CSI External Volumes. Shouldn't that be s/of one CSI plugin configuration file **under** the directory/of one **of the** CSI plugin configuration file**s** **in** the directory/? Not sure I'm fully getting the intended meaning; please check that what I suggest is what you actually intended. docs/isolators/csi-volume.md Lines 271-272 (patched) <https://reviews.apache.org/r/72845/#comment311095> s/a absolute/an absolute/ - Andrei Sekretenko On Oct. 8, 2020, 9:57 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72845/ > --- > > (Updated Oct. 8, 2020, 9:57 a.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10157 > https://issues.apache.org/jira/browse/MESOS-10157 > > > Repository: mesos > > > Description > --- > > Added doc for the `volume/csi` isolator. > > > Diffs > - > > docs/isolators/csi-volume.md PRE-CREATION > docs/mesos-containerizer.md 3231cb93481102dfc26a718918566e61b57e3617 > > > Diff: https://reviews.apache.org/r/72845/diff/3/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 72922: Re-added the obsolete `updateFramework` signature into libmesos-java.so.
> On Oct. 1, 2020, 10:19 p.m., Benjamin Mahler wrote: > > src/java/src/org/apache/mesos/MesosSchedulerDriver.java > > Lines 416-418 (patched) > > <https://reviews.apache.org/r/72922/diff/1/?file=2240239#file2240239line416> > > > > Use javadoc style here? maybe make this clear that it's a note for us > > and not the user looking at the interface? Changed style. Starts with `/*` and not with `/**`, so that javadoc does not generate documentation, regardless of whatever flags governing doc generation for private methods. Also seems to trigger different highlighting in some editors. Given that the method is private, it should be more or less clear that this is not for user. Now I've moved the public 3-argument `updateFramework()` higher up, next to the deprecated 2-parameter one, hopefully this will help avoid confusing users. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72922/#review221984 ------- On Oct. 2, 2020, 2:09 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72922/ > --- > > (Updated Oct. 2, 2020, 2:09 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10189 > https://issues.apache.org/jira/browse/MESOS-10189 > > > Repository: mesos > > > Description > --- > > This patch converts the implementation of the obsolete 2-parameter > `SchedulerDriver.updateFramework(frameworkInfo, suppressedRoles)` > from a wrapper around the new signature back into a JNI method > that 930c7e98d17e71192dae1d49b4b2217cc2dbd8b2 attempted to remove. > > This is needed to keep compatibility between older versions of > `mesos.jar` and newer versions of `libmesos-java.so`. > > > Diffs > - > > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 4efde3083bb62253f3ad512b40a291fd7f7f7a78 > src/java/src/org/apache/mesos/MesosSchedulerDriver.java > 4fdae338491709574775c52438e41d03b39917bd > > > Diff: https://reviews.apache.org/r/72922/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72922: Re-added the obsolete `updateFramework` signature into libmesos-java.so.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72922/ --- (Updated Oct. 2, 2020, 2:09 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Changed comments style, added deprecation labels to the 2-parameter signature. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- This patch converts the implementation of the obsolete 2-parameter `SchedulerDriver.updateFramework(frameworkInfo, suppressedRoles)` from a wrapper around the new signature back into a JNI method that 930c7e98d17e71192dae1d49b4b2217cc2dbd8b2 attempted to remove. This is needed to keep compatibility between older versions of `mesos.jar` and newer versions of `libmesos-java.so`. Diffs (updated) - src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 4efde3083bb62253f3ad512b40a291fd7f7f7a78 src/java/src/org/apache/mesos/MesosSchedulerDriver.java 4fdae338491709574775c52438e41d03b39917bd Diff: https://reviews.apache.org/r/72922/diff/2/ Changes: https://reviews.apache.org/r/72922/diff/1-2/ Testing --- `make check` Thanks, Andrei Sekretenko
Review Request 72922: Re-added the obsolete `updateFramework` signature into libmesos-java.so.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72922/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- This patch converts the implementation of the obsolete 2-parameter `SchedulerDriver.updateFramework(frameworkInfo, suppressedRoles)` from a wrapper around the new signature back into a JNI method that 930c7e98d17e71192dae1d49b4b2217cc2dbd8b2 attempted to remove. This is needed to keep compatibility between older versions of `mesos.jar` and newer versions of `libmesos-java.so`. Diffs - src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 4efde3083bb62253f3ad512b40a291fd7f7f7a78 src/java/src/org/apache/mesos/MesosSchedulerDriver.java 4fdae338491709574775c52438e41d03b39917bd Diff: https://reviews.apache.org/r/72922/diff/1/ Testing --- `make check` Thanks, Andrei Sekretenko
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review221963 --- src/common/http.hpp Lines 402-404 (original), 401-403 (patched) <https://reviews.apache.org/r/72448/#comment311028> The comment about MESOS-10085 is not relevant anymore after this patch? src/common/http.hpp Lines 419 (patched) <https://reviews.apache.org/r/72448/#comment311030> Does this correctly handle `tryApproved(...)`? If no, then can you make sure (at the very least, with a `static_assert`) that it is not accidentally used for VIEW_ROLE? src/common/http.hpp Lines 422-426 (patched) <https://reviews.apache.org/r/72448/#comment311029> Just `return approved(...)` ? src/master/master.cpp Line 12277 (original), 12286 (patched) <https://reviews.apache.org/r/72448/#comment311032> Can you follow the same approach for obtaining `VIEW_ROLE` authorization as well, here and below? - Andrei Sekretenko On July 23, 2020, 11:03 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated July 23, 2020, 11:03 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/3/ > > > Testing > --- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72900: Added offer constraints to `updateFramework()` in the Java bindings.
> On Sept. 24, 2020, 8:34 p.m., Benjamin Mahler wrote: > > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > > Line 1071 (original), 1071 (patched) > > <https://reviews.apache.org/r/72900/diff/1/?file=2239840#file2239840line1071> > > > > The suppressed roles collection is gone now? (Not sure if it was > > specified right originally) Thanks for noticing! It was specified right originally; JNI signatures of generics contain no information about the type parameters, a `Collection` indeed maps onto `Ljava/util/Collection` - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72900/#review221954 ------- On Sept. 25, 2020, 5:48 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72900/ > --- > > (Updated Sept. 25, 2020, 5:48 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10189 > https://issues.apache.org/jira/browse/MESOS-10189 > > > Repository: mesos > > > Description > --- > > Added offer constraints to `updateFramework()` in the Java bindings. > > > Diffs > - > > src/examples/java/TestFramework.java > 1dea79c0e5b76385bad74ded755911f4d1858112 > src/java/jni/construct.cpp a48ca07664f7178df97c3a9557f6908507442333 > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 1817bbafc4ce65ee5e58eb787e71c6d06481a32f > src/java/src/org/apache/mesos/MesosSchedulerDriver.java > 3e74be9e59e1419569ad940e5db5390b629ca376 > src/java/src/org/apache/mesos/SchedulerDriver.java > e04916e055349ee287eb0c9327bc5f6d1e6caec3 > > > Diff: https://reviews.apache.org/r/72900/diff/2/ > > > Testing > --- > > Testing Done > > With empty constraints: > make check > (and, specifically, src/mesos-tests > --gtest_filter='ExamplesTest.JavaFramework') > > > With non-empty constraints: extended ExamplesTest.JavaFramework (see WIP > https://reviews.apache.org/r/72901 which will most likely be discarded) and > re-ran it. > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72900: Added offer constraints to `updateFramework()` in the Java bindings.
> On Sept. 24, 2020, 8:34 p.m., Benjamin Mahler wrote: > > src/examples/java/TestFramework.java > > Lines 31-53 (original), 31-58 (patched) > > <https://reviews.apache.org/r/72900/diff/1/?file=2239838#file2239838line31> > > > > Seems we should separate the TestFramework.java changes from the > > binding changes? I'd rather switch to the new signature in the same patch: this patch `@Deprecate`s the old signature, and thus should trigger compilation warnings unless we switch to the new one. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72900/#review221954 ------- On Sept. 25, 2020, 5:48 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72900/ > --- > > (Updated Sept. 25, 2020, 5:48 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10189 > https://issues.apache.org/jira/browse/MESOS-10189 > > > Repository: mesos > > > Description > --- > > Added offer constraints to `updateFramework()` in the Java bindings. > > > Diffs > - > > src/examples/java/TestFramework.java > 1dea79c0e5b76385bad74ded755911f4d1858112 > src/java/jni/construct.cpp a48ca07664f7178df97c3a9557f6908507442333 > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 1817bbafc4ce65ee5e58eb787e71c6d06481a32f > src/java/src/org/apache/mesos/MesosSchedulerDriver.java > 3e74be9e59e1419569ad940e5db5390b629ca376 > src/java/src/org/apache/mesos/SchedulerDriver.java > e04916e055349ee287eb0c9327bc5f6d1e6caec3 > > > Diff: https://reviews.apache.org/r/72900/diff/2/ > > > Testing > --- > > Testing Done > > With empty constraints: > make check > (and, specifically, src/mesos-tests > --gtest_filter='ExamplesTest.JavaFramework') > > > With non-empty constraints: extended ExamplesTest.JavaFramework (see WIP > https://reviews.apache.org/r/72901 which will most likely be discarded) and > re-ran it. > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72900: Added offer constraints to `updateFramework()` in the Java bindings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72900/ --- (Updated Sept. 25, 2020, 5:48 p.m.) Review request for mesos and Benjamin Mahler. Changes --- - Fixed the signature comment in the C++ JNI code - `updateFramework` javadoc comments: synced from the C++ driver, adjusted @params, added @deprecated to the comment for the old signature. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- Added offer constraints to `updateFramework()` in the Java bindings. Diffs (updated) - src/examples/java/TestFramework.java 1dea79c0e5b76385bad74ded755911f4d1858112 src/java/jni/construct.cpp a48ca07664f7178df97c3a9557f6908507442333 src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 1817bbafc4ce65ee5e58eb787e71c6d06481a32f src/java/src/org/apache/mesos/MesosSchedulerDriver.java 3e74be9e59e1419569ad940e5db5390b629ca376 src/java/src/org/apache/mesos/SchedulerDriver.java e04916e055349ee287eb0c9327bc5f6d1e6caec3 Diff: https://reviews.apache.org/r/72900/diff/2/ Changes: https://reviews.apache.org/r/72900/diff/1-2/ Testing --- Testing Done With empty constraints: make check (and, specifically, src/mesos-tests --gtest_filter='ExamplesTest.JavaFramework') With non-empty constraints: extended ExamplesTest.JavaFramework (see WIP https://reviews.apache.org/r/72901 which will most likely be discarded) and re-ran it. Thanks, Andrei Sekretenko
Re: Review Request 72897: Made offer constraints filter and protobuf non-optional inside the code.
> On Sept. 24, 2020, 7:58 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp
> > Line 113 (original), 113-118 (patched)
> > <https://reviews.apache.org/r/72897/diff/2/?file=2239966#file2239966line113>
> >
> > Hm.. is this used? I would think we're always passing in the object to
> > create, even if not set?
There are at least two cases where we would otherwise need to explicitly
perform something like
`CHECK_NOTERROR(OfferConstraintsFilter::create({Bytes(0), 0}, {})`:
- recovering a non-subscribed framework from agent's FrameworkInfo
- various locations in tests, especially the hierarchical allocator tests
Given the sheer number of call sites of `Allocator::addFramework` in the
testing codebase, we'll have to add some kind of wrapper for that.
I don't think this is better than just keeping framewok's allocator options
default-constructable.
- Andrei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72897/#review221952
-------
On Sept. 24, 2020, 5:07 p.m., Andrei Sekretenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72897/
> ---
>
> (Updated Sept. 24, 2020, 5:07 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10189
> https://issues.apache.org/jira/browse/MESOS-10189
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Given that Mesos now provides a guarantee that specifying no offer
> constraint in UPDATE_FRAMEWORK/SUBSCRIBE call is equivalent to
> specifying default-constructed `OfferConstraints`, and that we are
> intending to make the V0 scheduler driver always require offer
> constraints as an argument to the `updateFramework()`, it no longer
> makes sense to keep `OfferConstraints`/`OfferConstraintsFilter`
> optional inside the Mesos code base.
>
> This patch replaces a non-set `Option` with
> default-constructed `OfferConstraints`, and a non-set
> `Option` with a default-constructed filter.
>
>
> Diffs
> -
>
> include/mesos/allocator/allocator.hpp
> b0a5d6aaa08a05cb1ece318999a8760df932ba64
> src/master/allocator/mesos/offer_constraints_filter.cpp
> 441ebc10219bf3bd623fac2bb08945ea9deb7ea3
> src/master/framework.cpp 980828e04cae96f091a6b30e553ff667fa58a917
> src/master/http.cpp 3d8e7095ee58c2505042095b2c593c2317f3232f
> src/master/master.hpp 95aca586f17a206b6ac7f0c48ea99c8525c336ad
> src/master/master.cpp 576ae107a6a3c99b51516fdde4df9bff86c90a99
> src/master/readonly_handler.cpp df499d1a5e962ee2fe0c47bc77fb1ef9d8bc
>
>
> Diff: https://reviews.apache.org/r/72897/diff/2/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72898: Added `OfferConstraints` validity criteria into protobuf comments.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72898/ --- (Updated Sept. 24, 2020, 5:13 p.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Added `OfferConstraints` validity criteria into protobuf comments. Diffs - include/mesos/scheduler/scheduler.proto f70738ce5f4aeddb38c402130f953dcff1d43d6a Diff: https://reviews.apache.org/r/72898/diff/2/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72897: Made offer constraints filter and protobuf non-optional inside the code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72897/ --- (Updated Sept. 24, 2020, 5:07 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Removed filter/constraints optionality instead of the previous approach. Summary (updated) - Made offer constraints filter and protobuf non-optional inside the code. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description (updated) --- Given that Mesos now provides a guarantee that specifying no offer constraint in UPDATE_FRAMEWORK/SUBSCRIBE call is equivalent to specifying default-constructed `OfferConstraints`, and that we are intending to make the V0 scheduler driver always require offer constraints as an argument to the `updateFramework()`, it no longer makes sense to keep `OfferConstraints`/`OfferConstraintsFilter` optional inside the Mesos code base. This patch replaces a non-set `Option` with default-constructed `OfferConstraints`, and a non-set `Option` with a default-constructed filter. Diffs (updated) - include/mesos/allocator/allocator.hpp b0a5d6aaa08a05cb1ece318999a8760df932ba64 src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3 src/master/framework.cpp 980828e04cae96f091a6b30e553ff667fa58a917 src/master/http.cpp 3d8e7095ee58c2505042095b2c593c2317f3232f src/master/master.hpp 95aca586f17a206b6ac7f0c48ea99c8525c336ad src/master/master.cpp 576ae107a6a3c99b51516fdde4df9bff86c90a99 src/master/readonly_handler.cpp df499d1a5e962ee2fe0c47bc77fb1ef9d8bc Diff: https://reviews.apache.org/r/72897/diff/2/ Changes: https://reviews.apache.org/r/72897/diff/1-2/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72899: Added a test for creating a filter from default `OfferConstraints`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72899/ --- (Updated Sept. 24, 2020, 5:06 p.m.) Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- Added a test for creating a filter from default `OfferConstraints`. Diffs (updated) - src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d Diff: https://reviews.apache.org/r/72899/diff/2/ Changes: https://reviews.apache.org/r/72899/diff/1-2/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72897: Allowed `OfferConstraintsFilter::create()` to return an empty `Result`.
> On Sept. 22, 2020, 10:50 p.m., Benjamin Mahler wrote: > > Hm.. can you clarify what this gives us? Now I think I need your input here. If we decide to keep in a long term a guarantee that specifying default-constructed OfferConstraints is equivalent to not specifying any constraints at all, we are going to have a certain weirdness internally: right now, there are two functionally equivalent ways to specify to the allocator that the framework has **no offer constraints filtering**. One way is to set the `offerConstraintsFilter` in the framework's allocator options to `None`; another is to pass a filter constructed from empty constraints. My intention was to eliminate one of these two ways. This patch gets rid of the second one (filters that are known to be a no-op are not created). However, on the second thought, just making a filter non-optional might make things simpler. I've benchmarked the default (no constraints) case and failed to find any difference between allocator's `struct Framework` always having a non-optional empty filter vs an empty Option. No difference - despite of the fact that the first thing in the inner allocation loop is the agent filtering check `offerConstraintsFilter.isSome() && offerConstraintsFilter->isAgentExcluded(..)` (the version that makes filter non-optional drops `isSome()`). What do you think about just making the filter obligatory (and default-constructable, equivalent to constructing form default `OfferConstraints`)? - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72897/#review221926 --- On Sept. 22, 2020, 8:05 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72897/ > --- > > (Updated Sept. 22, 2020, 8:05 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10189 > https://issues.apache.org/jira/browse/MESOS-10189 > > > Repository: mesos > > > Description > --- > > This patch makes it possible for the offer constraint filter creation > to return `None()` in cases when specified offer constraints would > result in creating a no-op `OfferConstraintsFilter` that would never > filter out anything, and actually implements this special handling > for an `OfferConstraints` message with an empty role constraints map. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > 6d67d5d5c32c1f18cdf6f925d747156c2bbc7820 > src/master/allocator/mesos/offer_constraints_filter.cpp > 441ebc10219bf3bd623fac2bb08945ea9deb7ea3 > src/master/master.cpp fefa72d338384b6ccb1bcbbed7192713411035db > src/tests/master/offer_constraints_filter_tests.cpp > f80d56c89d937b6a1b261202adefb37a1519bf0d > > > Diff: https://reviews.apache.org/r/72897/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72900: Added offer constraints to `updateFramework()` in the Java bindings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72900/#review221924 --- src/java/src/org/apache/mesos/SchedulerDriver.java Line 388 (original), 390 (patched) <https://reviews.apache.org/r/72900/#comment310976> Comments need to be synchronized with the final version of C++ ones: https://reviews.apache.org/r/72874/ - Andrei Sekretenko On Sept. 22, 2020, 8:09 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72900/ > --- > > (Updated Sept. 22, 2020, 8:09 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10189 > https://issues.apache.org/jira/browse/MESOS-10189 > > > Repository: mesos > > > Description > --- > > Added offer constraints to `updateFramework()` in the Java bindings. > > > Diffs > - > > src/examples/java/TestFramework.java > 1dea79c0e5b76385bad74ded755911f4d1858112 > src/java/jni/construct.cpp 8d8f76e4602e506faa635970f096d344a9ad8562 > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 1817bbafc4ce65ee5e58eb787e71c6d06481a32f > src/java/src/org/apache/mesos/MesosSchedulerDriver.java > 3e74be9e59e1419569ad940e5db5390b629ca376 > src/java/src/org/apache/mesos/SchedulerDriver.java > e04916e055349ee287eb0c9327bc5f6d1e6caec3 > > > Diff: https://reviews.apache.org/r/72900/diff/1/ > > > Testing > --- > > Testing Done > > With empty constraints: > make check > (and, specifically, src/mesos-tests > --gtest_filter='ExamplesTest.JavaFramework') > > > With non-empty constraints: extended ExamplesTest.JavaFramework (see WIP > https://reviews.apache.org/r/72901 which will most likely be discarded) and > re-ran it. > > > Thanks, > > Andrei Sekretenko > >
Review Request 72901: WIP: Added setting offer constraints to the Java example framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72901/ --- Review request for mesos. Repository: mesos Description --- WIP: Added setting offer constraints to the Java example framework. Diffs - src/examples/java/TestFramework.java 1dea79c0e5b76385bad74ded755911f4d1858112 Diff: https://reviews.apache.org/r/72901/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72900: Added offer constraints to `updateFramework()` in the Java bindings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72900/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- Added offer constraints to `updateFramework()` in the Java bindings. Diffs - src/examples/java/TestFramework.java 1dea79c0e5b76385bad74ded755911f4d1858112 src/java/jni/construct.cpp 8d8f76e4602e506faa635970f096d344a9ad8562 src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 1817bbafc4ce65ee5e58eb787e71c6d06481a32f src/java/src/org/apache/mesos/MesosSchedulerDriver.java 3e74be9e59e1419569ad940e5db5390b629ca376 src/java/src/org/apache/mesos/SchedulerDriver.java e04916e055349ee287eb0c9327bc5f6d1e6caec3 Diff: https://reviews.apache.org/r/72900/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72897: Allowed `OfferConstraintsFilter::create()` to return an empty `Result`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72897/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- This patch makes it possible for the offer constraint filter creation to return `None()` in cases when specified offer constraints would result in creating a no-op `OfferConstraintsFilter` that would never filter out anything, and actually implements this special handling for an `OfferConstraints` message with an empty role constraints map. Diffs - include/mesos/allocator/allocator.hpp 6d67d5d5c32c1f18cdf6f925d747156c2bbc7820 src/master/allocator/mesos/offer_constraints_filter.cpp 441ebc10219bf3bd623fac2bb08945ea9deb7ea3 src/master/master.cpp fefa72d338384b6ccb1bcbbed7192713411035db src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d Diff: https://reviews.apache.org/r/72897/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72898: Added `OfferConstraints` validity criteria into protobuf comments.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72898/#review221923 --- include/mesos/scheduler/scheduler.proto Lines 446 (patched) <https://reviews.apache.org/r/72898/#comment310975> I'll need to sync the final version into the V1 protobufs. - Andrei Sekretenko On Sept. 22, 2020, 8:03 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72898/ > --- > > (Updated Sept. 22, 2020, 8:03 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Added `OfferConstraints` validity criteria into protobuf comments. > > > Diffs > - > > include/mesos/scheduler/scheduler.proto > f70738ce5f4aeddb38c402130f953dcff1d43d6a > > > Diff: https://reviews.apache.org/r/72898/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Review Request 72898: Added `OfferConstraints` validity criteria into protobuf comments.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72898/ --- Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Added `OfferConstraints` validity criteria into protobuf comments. Diffs - include/mesos/scheduler/scheduler.proto f70738ce5f4aeddb38c402130f953dcff1d43d6a Diff: https://reviews.apache.org/r/72898/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72899: Added a test for creating a filter from default `OfferConstraints`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72899/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- Added a test for creating a filter from default `OfferConstraints`. Diffs - src/tests/master/offer_constraints_filter_tests.cpp f80d56c89d937b6a1b261202adefb37a1519bf0d Diff: https://reviews.apache.org/r/72899/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72874: Added offer constraints to `MesosSchedulerDriver::updateFramework()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72874/ --- (Updated Sept. 22, 2020, 7:57 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Made passing the constraints obligatory. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description (updated) --- This patch adds an ability to set V0 framework's offer constraints via the C++ V0 scheduler driver. Diffs (updated) - include/mesos/scheduler.hpp 61cc846a03d2980f18fa66df1de196edc5cb4457 src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 1817bbafc4ce65ee5e58eb787e71c6d06481a32f src/python/scheduler/src/mesos/scheduler/mesos_scheduler_driver_impl.cpp 256632a9589c8573cbe0f0aeb39ee6cb1e4b3256 src/sched/sched.cpp 768ce7d4074a1ea50b656717e25be6c38a423368 src/tests/master/update_framework_tests.cpp d6c45f60cf50739aacbb507e99a5163a6cb3d1a0 src/tests/scheduler_driver_tests.cpp 63d7a3ba9a98d8c343324f5ad85f0b4d0cc033be Diff: https://reviews.apache.org/r/72874/diff/2/ Changes: https://reviews.apache.org/r/72874/diff/1-2/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72887: Tied the 1st hierarchical allocator process to a process ID "allocator".
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72887/ --- (Updated Sept. 22, 2020, 12:23 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Reworded the commit description, removed stray diff. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description (updated) --- The instances of `HierarchicalAllocatorProcess` launched in the same instance of libprocess after that will be assigned IDs like "allocator(2)", "allocator(3)" and so on. Previously, to query an HTTP endpoint exposed by the allocator, the user had to figure out the allocator process id and use it to build the URL (like "https://localhost:5050/hierarchical_allocator(1)/..."); now, the allocator debugging endpoints are accessible via fixed URLs like "https://localhost:5050/allocator/offer_constraints_debug;. Diffs (updated) - src/master/allocator/mesos/hierarchical.hpp 63444de857851a267288a530fe868804b7ead2ab Diff: https://reviews.apache.org/r/72887/diff/3/ Changes: https://reviews.apache.org/r/72887/diff/2-3/ Testing --- `make check` on several platforms + tested the debug endpoint manually Thanks, Andrei Sekretenko
Re: Review Request 72887: Tied the 1st hierarchical allocator process to a process ID "allocator".
> On Sept. 18, 2020, 9:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Line 971 (original), 971 (patched)
> > <https://reviews.apache.org/r/72887/diff/1/?file=2239631#file2239631line971>
> >
> > Let's just go with s/hierarchical_allocator/allocator//
> >
> > Note that this means we can only have 1 allocator process in memory, so
> > we wouldn't be able to have a test with more than 1. I think that's
> > currently the case with the master process too, so it's probably acceptable.
> >
> > As an alternative, we could do:
> >
> > ```
> > : ProcessBase(stripTrailing("(1)",
> > process::ID::generate("allocator"))),
> > ```
> >
> > To make it "allocator", "allocator(2)", "allocator(3)", ...
Stripping `"(1)"` sounds like a better idea. Ajdusted the patch (also went with
just "allocator").
I'm almost sure that our tests do not use two allocators at the same time
intentionally.
However, I'm not 100% sure they never let allocator processes slip past the
test boundary, i.e. that all of them wait for the allocator to terminate in the
destructors/teardown.
- Andrei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72887/#review221895
---
On Sept. 21, 2020, 2:43 p.m., Andrei Sekretenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72887/
> ---
>
> (Updated Sept. 21, 2020, 2:43 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10177
> https://issues.apache.org/jira/browse/MESOS-10177
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The instances of `HierarchicalAllocatorProcess` launched in the same
> instance of libprocess after that will be assigned IDs like
> "allocator(2)", "allocator(3)" and so on.
>
> This allows the user to specify a fixed URL like
> `https://localhost:5050/allocator/offer_constraints_debug` when querying
> HTTP endpoints of an allocator in the mesos-master program instead of
> looking up the allocator UPID and using that to compose an URL (like
> `https://localhost:5050/hierarchical_allocator(1)/...`)
>
>
> Diffs
> -
>
> src/master/allocator/mesos/hierarchical.hpp
> 225de160772fe689e07877c895800eb711f211b5
>
>
> Diff: https://reviews.apache.org/r/72887/diff/2/
>
>
> Testing
> ---
>
> `make check` on several platforms + tested the debug endpoint manually
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72887: Tied the 1st hierarchical allocator process to a process ID "allocator".
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72887/ --- (Updated Sept. 21, 2020, 2:43 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Resorted to tying only the first process to `"allocator"`. Summary (updated) - Tied the 1st hierarchical allocator process to a process ID "allocator". Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description (updated) --- The instances of `HierarchicalAllocatorProcess` launched in the same instance of libprocess after that will be assigned IDs like "allocator(2)", "allocator(3)" and so on. This allows the user to specify a fixed URL like `https://localhost:5050/allocator/offer_constraints_debug` when querying HTTP endpoints of an allocator in the mesos-master program instead of looking up the allocator UPID and using that to compose an URL (like `https://localhost:5050/hierarchical_allocator(1)/...`) Diffs (updated) - src/master/allocator/mesos/hierarchical.hpp 225de160772fe689e07877c895800eb711f211b5 Diff: https://reviews.apache.org/r/72887/diff/2/ Changes: https://reviews.apache.org/r/72887/diff/1-2/ Testing --- `make check` on several platforms + tested the debug endpoint manually Thanks, Andrei Sekretenko
Re: Review Request 72874: Added offer constraints to `MesosSchedulerDriver::updateFramework()`.
> On Sept. 18, 2020, 10:22 p.m., Benjamin Mahler wrote: > > include/mesos/scheduler.hpp > > Lines 375-376 (patched) > > <https://reviews.apache.org/r/72874/diff/1/?file=2239588#file2239588line375> > > > > Functionally, what is the difference between a nullptr and a default > > constructed OfferConstraints proto? Or a OfferConstraints proto with a map > > of roles but each one having default constructed RoleConstraints? > > > > Just wondering if we need the nullptr here (we didn't use one fr > > suppressed roles, so looks inconsistent and makes one wonder the above > > question). > > > > Based on just looking at the offer constraints debug endpoint, we might > > want to see whether the framework is capable of setting it..? Right question, thanks for asking! A map of roles with default `RoleConstriants` has an entirely differentt semantics: a default-constructed `RoleConstraints` means that there are no constraint group for this role to match anything. When discussing the validity criteria in the design doc, we settled on disallowing empty `RoleConstraints`, because instead of setting empty `RoleConstraints` a framework should just explicitly suppress the role. On the other hand, default-constructed `OfferConstraints` is a perfectly legitimate thing: it asks Mesos to put constraints on offers for **no roles**. This indeed is equivalent to not setting `offer_constraints` in the `Subscribe`/`UpdateFramework`, and, IMO, it should be kept this way in the future. Probably I should revisit some things we previously landed: at the very least, the protobuf comments, but probably also some places already using `Option`. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72874/#review221898 ------- On Sept. 16, 2020, 9:05 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72874/ > --- > > (Updated Sept. 16, 2020, 9:05 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10189 > https://issues.apache.org/jira/browse/MESOS-10189 > > > Repository: mesos > > > Description > --- > > This patch adds an ability to set or clear V0 framework's offer > constraints via the C++ V0 scheduler driver. > > > Diffs > - > > include/mesos/scheduler.hpp 61cc846a03d2980f18fa66df1de196edc5cb4457 > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 1817bbafc4ce65ee5e58eb787e71c6d06481a32f > src/python/scheduler/src/mesos/scheduler/mesos_scheduler_driver_impl.cpp > 256632a9589c8573cbe0f0aeb39ee6cb1e4b3256 > src/sched/sched.cpp 768ce7d4074a1ea50b656717e25be6c38a423368 > src/tests/master/update_framework_tests.cpp > d6c45f60cf50739aacbb507e99a5163a6cb3d1a0 > src/tests/scheduler_driver_tests.cpp > 63d7a3ba9a98d8c343324f5ad85f0b4d0cc033be > > > Diff: https://reviews.apache.org/r/72874/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Review Request 72885: Made the hierarchical allocator store `FrameworkInfo` internally.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72885/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description --- To make it possible to add into the allocator debugguing HTTP endpoints that will need to authorize principal's viewing access to frameworks, it becomes necessary to store `FrameworkInfo` in the allocator, so that it can compose an authorization object for the VIEW_FRAMEWORK action. Diffs - src/master/allocator/mesos/hierarchical.hpp 7e1980ef74bb4138b83707de7698f1993cc21e41 src/master/allocator/mesos/hierarchical.cpp 47f8a2bfa267d68d39d2c255c7c97c8f213d5f69 Diff: https://reviews.apache.org/r/72885/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72886: Enforced explicit construction of `protobuf::...::Capabilities`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72886/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description --- This makes Mesos code more compliant with the Google C++ styleguide's rule banning implicit conversions for non-interchangeable types, for which the Mesos styleguide makes no exception. In particular, not enabling a type conversion from an iterable into `Capabilities` disallows puzzling code that looks like copying the same protobuf twice. Now, an example like ``` framework.info = info; framework.capabilities = info.capabilities();` ``` can no longer occur. Diffs - src/common/protobuf_utils.hpp 0558249023b39863b4030ddbcfe5c83dc945570a src/master/allocator/mesos/hierarchical.cpp 47f8a2bfa267d68d39d2c255c7c97c8f213d5f69 src/master/master.cpp 54b24b4eb8824737b10b3b4d6d51f387360570a1 src/slave/slave.cpp a69937b73e25e26c51d4fe857782bb75396606c1 Diff: https://reviews.apache.org/r/72886/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72851: Added an endpoint for debugging offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72851/ --- (Updated Sept. 18, 2020, 6:56 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Switched to serving from allocator. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description --- Added an endpoint for debugging offer constraints. Diffs (updated) - include/mesos/allocator/allocator.hpp c6fca65b5b886dec26807871b750c6a9587faee0 src/master/allocator/mesos/hierarchical.hpp 7e1980ef74bb4138b83707de7698f1993cc21e41 src/master/allocator/mesos/hierarchical.cpp 47f8a2bfa267d68d39d2c255c7c97c8f213d5f69 src/master/master.cpp 54b24b4eb8824737b10b3b4d6d51f387360570a1 Diff: https://reviews.apache.org/r/72851/diff/3/ Changes: https://reviews.apache.org/r/72851/diff/2-3/ Testing --- Thanks, Andrei Sekretenko
Review Request 72887: Tied the hierarchical allocator process to a fixed process ID.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72887/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description --- This allows the user to specify a fixed URL like `https://localhost:5050/hierarchical_allocator/offer_constraints_debug` when querying allocator HTTP endpoints instead of looking up the allocator UPID and using that to compose an URL (like `https://localhost:5050/hierarchical_allocator(0)/...`) Diffs - src/master/allocator/mesos/hierarchical.hpp 7e1980ef74bb4138b83707de7698f1993cc21e41 Diff: https://reviews.apache.org/r/72887/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72877: WIP: Modified Java test framework to set offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72877/ --- Review request for mesos. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- WIP: Modified Java test framework to set offer constraints. Diffs - src/examples/java/TestFramework.java 1dea79c0e5b76385bad74ded755911f4d1858112 Diff: https://reviews.apache.org/r/72877/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72876: Added offer constraints to `updateFramework()` in the Java bindings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72876/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- This patch adds an ability to set or clear V0 framework's offer constraints via the Java scheduler driver bindings. Diffs - src/examples/java/TestFramework.java 1dea79c0e5b76385bad74ded755911f4d1858112 src/java/jni/construct.cpp 8d8f76e4602e506faa635970f096d344a9ad8562 src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 1817bbafc4ce65ee5e58eb787e71c6d06481a32f src/java/src/org/apache/mesos/MesosSchedulerDriver.java 3e74be9e59e1419569ad940e5db5390b629ca376 src/java/src/org/apache/mesos/SchedulerDriver.java e04916e055349ee287eb0c9327bc5f6d1e6caec3 Diff: https://reviews.apache.org/r/72876/diff/1/ Testing --- With empty constraints: `make check` (and, specifically, `src/mesos-tests --gtest_filter='ExamplesTest.JavaFramework'`) With non-empty constraints: extended `ExamplesTest.JavaFramework` (see WIP https://reviews.apache.org/r/72877/ which will most likely be discarded) and re-ran it. Thanks, Andrei Sekretenko
Review Request 72875: Consolidated protobuf construction by the JNI scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72875/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- This patch converts the copy-pasted internals of numerous `construct<>()` specializations for protobufs into a single template. Diffs - src/java/jni/construct.cpp 8d8f76e4602e506faa635970f096d344a9ad8562 Diff: https://reviews.apache.org/r/72875/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72874: Added offer constraints to `MesosSchedulerDriver::updateFramework()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72874/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10189 https://issues.apache.org/jira/browse/MESOS-10189 Repository: mesos Description --- This patch adds an ability to set or clear V0 framework's offer constraints via the C++ V0 scheduler driver. Diffs - include/mesos/scheduler.hpp 61cc846a03d2980f18fa66df1de196edc5cb4457 src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 1817bbafc4ce65ee5e58eb787e71c6d06481a32f src/python/scheduler/src/mesos/scheduler/mesos_scheduler_driver_impl.cpp 256632a9589c8573cbe0f0aeb39ee6cb1e4b3256 src/sched/sched.cpp 768ce7d4074a1ea50b656717e25be6c38a423368 src/tests/master/update_framework_tests.cpp d6c45f60cf50739aacbb507e99a5163a6cb3d1a0 src/tests/scheduler_driver_tests.cpp 63d7a3ba9a98d8c343324f5ad85f0b4d0cc033be Diff: https://reviews.apache.org/r/72874/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72851: Added an endpoint for debugging offer constraints.
> On Sept. 15, 2020, 9:25 p.m., Benjamin Mahler wrote: > > Sorry for the delay! Two things came up as I was thinking through this one: > > > > (1) I was expecting this to be an /allocator endpoint served from the > > allocator process rather than a /master endpoint served from the master > > process. The only advantage of being served by the master process is that > > it can leverage the parallel serving logic (any others?). The major > > downside is that we have to put constraint evaluation code in the master > > related logic, although since it's abstracted well it's not currently a big > > deal (much like we did with the /roles information). > > > > The main worry about it being on /master for me, is that if we need to add > > some more information in the endpoint that needs to come from the allocator > > then it will be better placed in the allocator. The obvious example coming > > to mind is: we're going to add resource quantity constraints that make the > > way agents get excluded much more dynamic than the attribute constraints > > alone. Do we want the master evaluating those? A more difficult example is > > per-constraint metrics around evaluations / pass/fail results / latency, > > etc, those might require the master to hold a shared copy of the overall > > filtering object if we were to serve it from /master? > > > > (2) One thing I realized while thinking about future constraints: we may > > want to split apart the exclusion lists as we add more types of > > constraints. E.g. "excluded", "excluded_by_attribute_constraints", > > "excluded_by_resource_constraints", etc? Otherwise, it might be pretty > > dynamic and hard to understand from the endpoint why the agent is overall > > being excluded? > > > > Thoughts? The extendability concerns are perfectly legitimate. Especially if we will want to report some detailed statistics at some point in the future. The parallel serving logic is not that important, as we do not expect this endpoint to be called frequently. Sure, it would be nice to avoid stalling allocator while collecting the debug information, but it doesn't seem that critical. My main concerns about moving the whole implementation into the allocator are **authentication** and **authorization**. Two options here: 1) Perform initial handling of the request in the master and extend the allocator interface with a method returning the debug information (like `Future getOfferConstraintsDebugInfo(std::vector&&)` or, maybe, returning a protobuf `Future getOfferConstraintsDebugInfo(std::vector&&)`) 2) Extend allocator options with the readonly authentication domain (for which an authenticator is already set up by the master) and the authorizer; place the whole implementation into the Mesos allocator. The slight drawback is that this way users of the authorizer interface proliferate (we are adding one more); IMO, this is not a big deal and probably even quite logical. Probably we should indeed prefer the second option: - this way, we don't add ad-hoc methods that the allocator would be obliged to implement - we keeps the debug endpoint as such separate from the allocator interface - providing an authentication realm and an authorizer to an allocator facilitates adding other authenticated authorizable HTTP endpoints to the allocator, should we ever need this in the future What do you think? - Andrei ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72851/#review221861 --- On Sept. 9, 2020, 9:22 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72851/ > --- > > (Updated Sept. 9, 2020, 9:22 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10177 > https://issues.apache.org/jira/browse/MESOS-10177 > > > Repository: mesos > > > Description > --- > > Added an endpoint for debugging offer constraints. > > > Diffs > - > > src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 > src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2 > src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e > src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31 > > > Diff: https://reviews.apache.org/r/72851/diff/2/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72840: Exposed offer constraints via the `/state` and `/frameworks` endpoints.
> On Sept. 10, 2020, 2:03 a.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 914 (patched)
> > <https://reviews.apache.org/r/72840/diff/1/?file=2239297#file2239297line915>
> >
> > Remove this? and consider putting it on the assertion instead?
> >
> > E.g.
> >
> > ```
> > ASSERT_SOME_EQ(JSON::protobuf(updatedConstraints), reportedConstraints)
> > << "Expected: " << ... "\n vs actual: " << ...;
> > ```
> >
> > Is this not printing out the two correctly upon failure?
Whoops, missed that line.
In fact, on failure `ASSERT_SOME_EQ` for `JSON:Object`s prints what we want
without any custom logging, like:
```
src/tests/master/update_framework_tests.cpp:939: Failure
Value of: (reportedConstraints).get()
Actual:
{"role_constraints":{"*":{"groups":[{"attribute_constraints":[{"predicate":{"not_exists":{}},"selector":{"attribute_name":"foo"}}]}]}}}
Expected: JSON::protobuf(updatedConstraints)
Which is:
{"role_constraints":{"*":{"groups":[{"attribute_constraints":[{"predicate":{"not_exists":{}},"selector":{"attribute_name":"foo"}}]}]},"bar":{}}}
```
> On Sept. 10, 2020, 2:03 a.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 919 (patched)
> > <https://reviews.apache.org/r/72840/diff/1/?file=2239297#file2239297line920>
> >
> > It's a little hard to see what it's supposed to look like based on
> > this, seems preferrable to me to check the expected json in some fashion?
> >
> > E.g.
> >
> > ```
> > JSON::Value expected = ...
> > "offer_constraints: { ... }"
> > ```
> >
> > Like we do in other tests, that way, you can read the test and know
> > what the API response is supposed to look like?
This one compares JSONs (and prints them on failure, see above); as the
invariant this aims to check is that the returned constraints are the same as
what we feed in, it doesn't make much sense to define the expected value right
here.
However, I should probably follow your earlier suggestion and convert the
constraints definitions to JSON.
Sent a follow-up patch for that: https://reviews.apache.org/r/72864
> On Sept. 10, 2020, 2:03 a.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 921 (patched)
> > <https://reviews.apache.org/r/72840/diff/1/?file=2239297#file2239297line922>
> >
> > Any reason not to just implement the check?
Added '/frameworks' too.
- Andrei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72840/#review221837
---
On Sept. 11, 2020, 8:03 p.m., Andrei Sekretenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72840/
> ---
>
> (Updated Sept. 11, 2020, 8:03 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10179
> https://issues.apache.org/jira/browse/MESOS-10179
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Exposed offer constraints via the `/state` and `/frameworks` endpoints.
>
>
> Diffs
> -
>
> src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31
> src/tests/master/update_framework_tests.cpp
> 87bc3c7d23ea76118473d9a4ec3c77e7a9d5a97e
>
>
> Diff: https://reviews.apache.org/r/72840/diff/2/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72840: Exposed offer constraints via the `/state` and `/frameworks` endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72840/ --- (Updated Sept. 11, 2020, 8:03 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Added a subtest for `/frameworks`, removed the stray LOG(INFO). Bugs: MESOS-10179 https://issues.apache.org/jira/browse/MESOS-10179 Repository: mesos Description --- Exposed offer constraints via the `/state` and `/frameworks` endpoints. Diffs (updated) - src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31 src/tests/master/update_framework_tests.cpp 87bc3c7d23ea76118473d9a4ec3c77e7a9d5a97e Diff: https://reviews.apache.org/r/72840/diff/2/ Changes: https://reviews.apache.org/r/72840/diff/1-2/ Testing --- Thanks, Andrei Sekretenko
Review Request 72864: Made `UpdateFrameworkTest.OfferConstraints` build constraints from JSON.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72864/ --- Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Made `UpdateFrameworkTest.OfferConstraints` build constraints from JSON. Diffs - src/tests/master/update_framework_tests.cpp 87bc3c7d23ea76118473d9a4ec3c77e7a9d5a97e Diff: https://reviews.apache.org/r/72864/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72839: Added `OfferConstraints` to `struct Framework` in the master.
> On Sept. 10, 2020, 1:57 a.m., Benjamin Mahler wrote:
> > Hm.. I was wondering how we exposed the suppressed roles in /frameworks &
> > GET_FRAMEWORKS, since I didn't see them in the interfaces here, but looks
> > like we don't expose them?
> >
> > An alternative would be to include this in the allocatorOptions, like
> > suppressed roles, but store allocatorOptions in the master's Framework
> > struct, rather than throwing it away after passing it through to the
> > allocator. Seems a bit cleaner and won't require suppressed roles to also
> > get added to all these interfaces? I guess you perhaps didn't go this route
> > since allocator options is storing the compiled form of this information,
> > and the allocator does not need the raw form?
I went this route for a number of reasons:
- allocator does not need the raw form of the offer constraints
- neither does master normally need the filter (only when handling the debug
endpoint, that is, once in eternity)
- filter is non-copyable; keeping it this way would greatly simplify things
should performance reasons at some later point require a piece of a mutable
state in the filter (intermediate result caching, constraint reordering,
whatever)
> On Sept. 10, 2020, 1:57 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3261-3275 (original), 3297-3313 (patched)
> > <https://reviews.apache.org/r/72839/diff/1/?file=2239295#file2239295line3297>
> >
> > Maybe just touch the protobuf once, then work off the option? Looks a
> > bit clearer?
> >
> > ```
> > Option offerConstraints;
> > if (call.has_offer_constraints()) {
> > offerConstraints = std::move(*call.mutable_offer_constraints());
> > }
> >
> > allocator::FrameworkOptions allocatorOptions;
> > if (offerConstraints.isSome()) {
> > // TODO(asekretenko): Validate roles in offer constraints (see
> > MESOS-10176).
> > Try filter = OfferConstraintsFilter::create(
> > offerConstraintsFilterOptions, *offerConstraints);
> >
> > if (filter.isError()) {
> > return process::http::BadRequest(
> > "'UpdateFramework.offer_constraints' are not valid: " +
> > filter.error());
> > }
> >
> > allocatorOptions.offerConstraintsFilter = std::move(*filter);
> > }
> > ```
agree, looks slightly clearer
- Andrei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72839/#review221836
---
On Sept. 7, 2020, 5:26 p.m., Andrei Sekretenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72839/
> ---
>
> (Updated Sept. 7, 2020, 5:26 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10179
> https://issues.apache.org/jira/browse/MESOS-10179
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This is a prerequisite for exposing framework's offer constraints
> via master API endpoints.
>
>
> Diffs
> -
>
> src/master/framework.cpp e9e2d20bdc89dd299b2376dd2f67ae056538c3e5
> src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2
> src/master/master.cpp 438ef98a96013ce378956ed5a0ad96d0dbb4469c
>
>
> Diff: https://reviews.apache.org/r/72839/diff/2/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 72862: Added cleanup of build dir to website bot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72862/#review221856 --- Ship it! Ship It! - Andrei Sekretenko On Sept. 10, 2020, 11:42 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72862/ > --- > > (Updated Sept. 10, 2020, 11:42 p.m.) > > > Review request for mesos and Andrei Sekretenko. > > > Repository: mesos > > > Description > --- > > Added cleanup of build dir to website bot. > > > Diffs > - > > support/mesos-website/entrypoint.sh > 72fd72375e63f7e0dddb5472187edfdbe7a9d148 > > > Diff: https://reviews.apache.org/r/72862/diff/1/ > > > Testing > --- > > Tested via > https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/16/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 72861: Added `--no-same-owner` option to tar command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72861/#review221855 --- Ship it! Ship It! - Andrei Sekretenko On Sept. 10, 2020, 11:40 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72861/ > --- > > (Updated Sept. 10, 2020, 11:40 p.m.) > > > Review request for mesos and Andrei Sekretenko. > > > Repository: mesos > > > Description > --- > > This ensures extracted tarballs are owned by root when running the > build as root. > > > Diffs > - > > 3rdparty/Makefile.am 23f49ecffc5b44529ce69ccd841d942a30a98c2d > > > Diff: https://reviews.apache.org/r/72861/diff/1/ > > > Testing > --- > > > Thanks, > > Vinod Kone > >
Re: Review Request 72859: Added `--no-same-owner` option to tar command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72859/#review221853 --- Ship it! Ship It! - Andrei Sekretenko On Sept. 10, 2020, 11:39 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72859/ > --- > > (Updated Sept. 10, 2020, 11:39 p.m.) > > > Review request for mesos and Andrei Sekretenko. > > > Repository: mesos > > > Description > --- > > This ensures extracted tarballs are owned by root when running the > build as root. > > > Diffs > - > > 3rdparty/stout/3rdparty/Makefile.am > 5a90a11c67131f21dc4487222ea1dde228dd > > > Diff: https://reviews.apache.org/r/72859/diff/1/ > > > Testing > --- > > > Thanks, > > Vinod Kone > >
Re: Review Request 72860: Added `--no-same-owner` option to tar command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72860/#review221854 --- Ship it! Ship It! - Andrei Sekretenko On Sept. 10, 2020, 11:39 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72860/ > --- > > (Updated Sept. 10, 2020, 11:39 p.m.) > > > Review request for mesos and Andrei Sekretenko. > > > Repository: mesos > > > Description > --- > > This ensures extracted tarballs are owned by root when running the > build as root. > > > Diffs > - > > 3rdparty/libprocess/3rdparty/Makefile.am > e148f2dd2470ba973cc3f6d4067be64039299a48 > > > Diff: https://reviews.apache.org/r/72860/diff/1/ > > > Testing > --- > > > Thanks, > > Vinod Kone > >
Re: Review Request 72858: Reduced build parallelism to avoid aborted builds in CI.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72858/#review221846 --- Ship it! support/mesos-website/entrypoint.sh Line 35 (original), 35 (patched) <https://reviews.apache.org/r/72858/#comment310922> Maybe leave a comment that higher values lead to stuck/killed builds in the ASF CI, most likely due to out of memory conditions? - Andrei Sekretenko On Sept. 10, 2020, 2:14 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72858/ > --- > > (Updated Sept. 10, 2020, 2:14 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Reduced build parallelism to avoid aborted builds in CI. > > > Diffs > - > > support/mesos-website/entrypoint.sh > 72fd72375e63f7e0dddb5472187edfdbe7a9d148 > > > Diff: https://reviews.apache.org/r/72858/diff/1/ > > > Testing > --- > > https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 72857: Merged build.sh and entrypoint.sh.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72857/#review221845 --- Ship it! Ship It! - Andrei Sekretenko On Sept. 10, 2020, 2:13 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72857/ > --- > > (Updated Sept. 10, 2020, 2:13 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Merged build.sh and entrypoint.sh. > > > Diffs > - > > support/mesos-website/build.sh afbec486e892b580348091dc790da651ff4b5952 > support/mesos-website/entrypoint.sh > 72fd72375e63f7e0dddb5472187edfdbe7a9d148 > > > Diff: https://reviews.apache.org/r/72857/diff/1/ > > > Testing > --- > > https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 72856: Fixed Website bot to work with docker user namespaces.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72856/#review221844 --- Ship it! support/mesos-website/entrypoint.sh Lines 21 (patched) <https://reviews.apache.org/r/72856/#comment310921> Now that we are relying on --userns-remap, perhaps we should check that the mounted directories are indeed remapped? Something like ``` if [ $(stat . --format=%u) -ne $(id -u) ]; then echo " The mounted mesos sources are not owned by the current user inside the containter. Please check that dockerd has user namespace remapping configured properly. " exit 1 fi ``` in the entrypoint.sh ? Otherwise, running this on a misconfigured dockerd will, at the very least, leave a root-owned waste (`configure`, `Makefile.in`s and, potentially, the whole `build`) in the sources dir. - Andrei Sekretenko On Sept. 10, 2020, 2:12 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72856/ > --- > > (Updated Sept. 10, 2020, 2:12 p.m.) > > > Review request for mesos, Andrei Sekretenko, Benjamin Mahler, and Gilbert > Song. > > > Repository: mesos > > > Description > --- > > ASF CI has enabled user namespaces for docker daemon on the build > machines that are used to build the website. This commit makes the > requisite changes to work with user namespacing. > > > Diffs > - > > support/mesos-website.sh 04a664611cb882813e18d8b45da4aabb578e7d44 > support/mesos-website/Dockerfile 611c4962d1eb99b2e8ea225bb5dcf840d973ab9c > support/mesos-website/entrypoint.sh > 72fd72375e63f7e0dddb5472187edfdbe7a9d148 > > > Diff: https://reviews.apache.org/r/72856/diff/2/ > > > Testing > --- > > Tested via > https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 72851: Added an endpoint for debugging offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72851/ --- (Updated Sept. 9, 2020, 9:22 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Replaced agent filtration state with excluded agent ID lists. Summary (updated) - Added an endpoint for debugging offer constraints. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description (updated) --- Added an endpoint for debugging offer constraints. Diffs (updated) - src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2 src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31 Diff: https://reviews.apache.org/r/72851/diff/2/ Changes: https://reviews.apache.org/r/72851/diff/1-2/ Testing --- Thanks, Andrei Sekretenko
Review Request 72851: Added an `/offer_constraints_debug` endpoint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72851/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10177 https://issues.apache.org/jira/browse/MESOS-10177 Repository: mesos Description --- Added an `/offer_constraints_debug` endpoint. Diffs - src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 src/master/master.hpp 8c2f56a0707eed8f61147715497b64a119bb02d2 src/master/master.cpp e99cc6b8e3610c8116e9b1feedc3d7ce9f954e1e src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31 Diff: https://reviews.apache.org/r/72851/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72843: Fixed a stale comment about delayed permissions when batching requests.
> On Sept. 8, 2020, 7:09 p.m., Benjamin Mahler wrote: > > src/master/http.cpp > > Lines 2368-2370 (original), 2368-2370 (patched) > > <https://reviews.apache.org/r/72843/diff/1/?file=2239304#file2239304line2368> > > > > A bit hard for me to understand how the comment relates to the code, I > > guess what I should be getting out of this comment is that the object > > approvers are always functionally equivalent now? > > > > Seems like the comment belongs up above in the de-duplication search? > > > > ``` > > // NOTE: This is not a general-purpose request comparison, but > > // specific to the batched requests which are always members of > > // `ReadOnlyHandler`, since we rely on the response only > > depending > > // on query parameters and the current master state. > > // > > // We don't need to check that the ObjectApprovers are the same, > > // since ...? > > return handler == batchedRequest.handler && > >principal == batchedRequest.principal && > >outputContentType == batchedRequest.outputContentType && > >queryParameters == batchedRequest.queryParameters; > > ``` > > > > Down here it seems like there's nothing related to object approvers to > > comment on. Good point! While moving the comment, realized how fragile the apporovers reuse actually is: regardless of synchronous authorization, this code heavily relies on the fact that each caller always provides `approvers` for the same set of actions for the same handler. Makes me think about moving `ObjectApprovers` creation into this method (not 100% trivial thanks to that specialization in `ObjectApprovers`). Added a TODO. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72843/#review221817 --- On Sept. 9, 2020, 5:38 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72843/ > --- > > (Updated Sept. 9, 2020, 5:38 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Now that synchronous authorization requires the authorizer to keep > object approvers up-to-date by the authorizer, batched processing > of readonly requests in the Master no longer intorduces an additional > delay into propagation of permission changes. > > > Diffs > - > > src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 > > > Diff: https://reviews.apache.org/r/72843/diff/2/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72843: Fixed a stale comment about delayed permissions when batching requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72843/ --- (Updated Sept. 9, 2020, 5:38 p.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Now that synchronous authorization requires the authorizer to keep object approvers up-to-date by the authorizer, batched processing of readonly requests in the Master no longer intorduces an additional delay into propagation of permission changes. Diffs (updated) - src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 Diff: https://reviews.apache.org/r/72843/diff/2/ Changes: https://reviews.apache.org/r/72843/diff/1-2/ Testing --- Thanks, Andrei Sekretenko
Review Request 72843: Fixed a stale comment about delayed permissions when batching requests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72843/ --- Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Now that synchronous authorization requires the authorizer to keep object approvers up-to-date by the authorizer, batched processing of readonly requests in the Master no longer intorduces an additional delay into propagation of permission changes. Diffs - src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 Diff: https://reviews.apache.org/r/72843/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72842: Exposed framework offer constraints via the GET_FRAMEWORKS call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72842/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10179 https://issues.apache.org/jira/browse/MESOS-10179 Repository: mesos Description --- Exposed framework offer constraints via the GET_FRAMEWORKS call. Diffs - src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 src/tests/master/update_framework_tests.cpp 87bc3c7d23ea76118473d9a4ec3c77e7a9d5a97e Diff: https://reviews.apache.org/r/72842/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72841: Added offer constraints to the `GetFrameworks` protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72841/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10179 https://issues.apache.org/jira/browse/MESOS-10179 Repository: mesos Description --- Added offer constraints to the `GetFrameworks` protobuf. Diffs - include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d Diff: https://reviews.apache.org/r/72841/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72840: Exposed offer constraints via the `/state` and `/frameworks` endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72840/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10179 https://issues.apache.org/jira/browse/MESOS-10179 Repository: mesos Description --- Exposed offer constraints via the `/state` and `/frameworks` endpoints. Diffs - src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31 src/tests/master/update_framework_tests.cpp 87bc3c7d23ea76118473d9a4ec3c77e7a9d5a97e Diff: https://reviews.apache.org/r/72840/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72839: Added `OfferConstraints` to `struct Framework` in the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72839/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10179 https://issues.apache.org/jira/browse/MESOS-10179 Repository: mesos Description --- This is a prerequisite for exposing framework's offer constraints via master API endpoints. Diffs - src/master/framework.cpp e9e2d20bdc89dd299b2376dd2f67ae056538c3e5 src/master/master.hpp 3d1d4723d1bcef1880404007410bf00656399f10 src/master/master.cpp 5b5d5c359ca48eb0b4f5554fe7c91e928d4da08d Diff: https://reviews.apache.org/r/72839/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72838: Moved framework update out of `connectAndActivateRecoveredFramework`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72838/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10179 https://issues.apache.org/jira/browse/MESOS-10179 Repository: mesos Description --- This patch consolidates updating framework in the Subscribe call by moving `updateFramework()` invocation from `connectAndActivateRecoveredFramework()` into `Master::_subscribe()`. Diffs - src/master/master.hpp 3d1d4723d1bcef1880404007410bf00656399f10 src/master/master.cpp 5b5d5c359ca48eb0b4f5554fe7c91e928d4da08d Diff: https://reviews.apache.org/r/72838/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72824: Eliminated redundant update broadcasts in UPDATE_FRAMEWORK.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72824/ --- (Updated Sept. 3, 2020, 5:56 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Adjusted comments and logging. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch makes the UPDATE_FRAMEWORK call avoid sending the `UpdatedFrameworkMessage` to agents and the `FRAMEWORK_UPDATED` event to V1 API subscribers when the FrameworkInfo is not changed by the call. Diffs (updated) - src/master/master.cpp 02723296e569fac9d553b1494a5ca7daa6ef9aa4 Diff: https://reviews.apache.org/r/72824/diff/5/ Changes: https://reviews.apache.org/r/72824/diff/4-5/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72833: Removed `bool operator==(const FrameworkInfo&, const FrameworkInfo&)`.
> On Sept. 3, 2020, 1:31 a.m., Benjamin Mahler wrote: > > include/mesos/type_utils.hpp > > Lines 119-122 (original) > > <https://reviews.apache.org/r/72833/diff/1/?file=2239007#file2239007line119> > > > > Hm.. Can we actually leave these but as `delete`d functions? Seems like > > C++ does allow non-member functions to be deleted? > > > > https://abseil.io/tips/143 > > > > https://stackoverflow.com/questions/42332777/what-is-the-point-of-using-delete-on-a-non-member-function > > > > ``` > > // This has been deleted in favor of ... > > inline bool operator==(const FrameworkInfo& left, const FrameworkInfo& > > right) = delete; > > ``` > > > > That will make it a lot less likely that someone re-adds this operator > > when they find it's missing. Also helps them find the equivalent function. Thanks! > On Sept. 3, 2020, 1:31 a.m., Benjamin Mahler wrote: > > src/tests/api_tests.cpp > > Lines 3354-3358 (patched) > > <https://reviews.apache.org/r/72833/diff/1/?file=2239009#file2239009line3354> > > > > Should we just have DEFAULT_FRAMEWORK_INFO explicitly set it to false? > > Just a thought.. Yes, I thought about it; decided to leave it like this. Tests that verify that a non-set `checkpoint` is handled correctly might implicitly depend on this; if we set it to false in `DEFAULT_FRAMEWORK_INFO`, they might just stop doing anything meaningful. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72833/#review221787 --- On Sept. 3, 2020, 5:34 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72833/ > --- > > (Updated Sept. 3, 2020, 5:34 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10166 > https://issues.apache.org/jira/browse/MESOS-10166 > > > Repository: mesos > > > Description > --- > > This patch removes the outdated `operator==` for `FrameworkInfo` > (which has been comparing only `name` and `user` fields) > and replaces it with `equivalent()` in the tests that need to compare > `FrameworkInfo`s. > > > Diffs > - > > include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 > include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 > src/tests/api_tests.cpp e0ce0335bf99f1f624efcca8c86f797df472d68f > src/tests/master_allocator_tests.cpp > 416b7ba733c3a9fc75e64fecf088ff13548bab3f > src/tests/resources_tests.cpp bd044310e297174155b88d5694641acd5df4cf59 > > > Diff: https://reviews.apache.org/r/72833/diff/2/ > > > Testing > --- > > `support/mesos-gtest-runner.py src/mesos-tests` > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72833: Removed `bool operator==(const FrameworkInfo&, const FrameworkInfo&)`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72833/ --- (Updated Sept. 3, 2020, 5:34 p.m.) Review request for mesos and Benjamin Mahler. Changes --- - marked `operator==` as `= delete`; - got rid of `addFramework` call matching in the `MasterAllocatorTest.OutOfOrderDispatch` test Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch removes the outdated `operator==` for `FrameworkInfo` (which has been comparing only `name` and `user` fields) and replaces it with `equivalent()` in the tests that need to compare `FrameworkInfo`s. Diffs (updated) - include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 src/tests/api_tests.cpp e0ce0335bf99f1f624efcca8c86f797df472d68f src/tests/master_allocator_tests.cpp 416b7ba733c3a9fc75e64fecf088ff13548bab3f src/tests/resources_tests.cpp bd044310e297174155b88d5694641acd5df4cf59 Diff: https://reviews.apache.org/r/72833/diff/2/ Changes: https://reviews.apache.org/r/72833/diff/1-2/ Testing --- `support/mesos-gtest-runner.py src/mesos-tests` Thanks, Andrei Sekretenko
Re: Review Request 72822: Extracted FrameworkInfo comparison logic from tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72822/ --- (Updated Sept. 3, 2020, 5:29 p.m.) Review request for mesos and Benjamin Mahler. Changes --- - converted `FrameworkInfoDifferencer` into a function - renamed the header Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch extracts the code that performs `FrameworkInfo` comparison via protobuf `MessageDifferencer` from the framework update tests into a set of general-purpose methods. Diffs (updated) - include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63 src/common/type_utils.cpp bb437844183688e7f69eb0874d87efe5a7d6daf2 src/common/type_utils_differencers.hpp PRE-CREATION src/tests/master/update_framework_tests.cpp d2a63b696e976d9e3c729390d80f33180ab4aa1c src/v1/mesos.cpp c2fb528d323899c42b32c0713b38ebf6f909dd29 Diff: https://reviews.apache.org/r/72822/diff/4/ Changes: https://reviews.apache.org/r/72822/diff/3-4/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72790: Deduplicated concurrent image pulls by docker store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72790/ --- (Updated Sept. 3, 2020, 12:29 p.m.) Review request for mesos and Qian Zhang. Changes --- Added a comment for the pull hashmap; fixed indentation. Bugs: MESOS-10169 https://issues.apache.org/jira/browse/MESOS-10169 Repository: mesos Description --- This patch makes the docker store reuse a pending pull if asked for an image that is already being pulled. The pull caching follows the same approach as the earlier attempt in https://reviews.apache.org/r/39331 . However, handing out futures to the store callers and handling their discards are performed differently, using a form of reference counting, so that the pull itself is discarded only if all the futures returned by `Store::get()` have been discarded. Diffs (updated) - src/slave/containerizer/mesos/provisioner/docker/store.cpp bf2be90aa4fcd7541849a157eb6930b491ccf03a Diff: https://reviews.apache.org/r/72790/diff/2/ Changes: https://reviews.apache.org/r/72790/diff/1-2/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72824: Eliminated redundant update broadcasts in UPDATE_FRAMEWORK.
> On Sept. 2, 2020, 6:02 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Lines 2847-2848 (patched) > > <https://reviews.apache.org/r/72824/diff/4/?file=2239176#file2239176line2847> > > > > No pid change possible in this path? (unlike the other TODO below?) If > > so, perhaps state so expicitly? Good catch! The HTTP `subscribe()` can remove the PID in case of a framework upgrade from V0 to HTTP. I have to adjust the comment. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72824/#review221784 ------- On Sept. 2, 2020, 2:08 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72824/ > --- > > (Updated Sept. 2, 2020, 2:08 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10166 > https://issues.apache.org/jira/browse/MESOS-10166 > > > Repository: mesos > > > Description > --- > > This patch makes the UPDATE_FRAMEWORK call avoid sending the > `UpdatedFrameworkMessage` to agents and the `FRAMEWORK_UPDATED` > event to V1 API subscribers when the FrameworkInfo is not changed > by the call. > > > Diffs > - > > src/master/master.cpp 02723296e569fac9d553b1494a5ca7daa6ef9aa4 > > > Diff: https://reviews.apache.org/r/72824/diff/4/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72825: Added a test of UPDATE_FRAMEWORK with the same FrameworkInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72825/ --- (Updated Sept. 2, 2020, 2:11 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Adjusted for changes in previous patches. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This test verifies that calling UPDATE_FRAMEWORK with no changes in `FrameworkInfo` (i.e. to change suppressed roles or offer constraints) results in no API event FRAMEWORK_UPDATED sent to the subscribers and no `UpdatedFrameworkMessage` broadcast to the agents. Diffs (updated) - src/tests/master/update_framework_tests.cpp d2a63b696e976d9e3c729390d80f33180ab4aa1c Diff: https://reviews.apache.org/r/72825/diff/3/ Changes: https://reviews.apache.org/r/72825/diff/2-3/ Testing --- `src/mesos-tests --gtest_filter='UpdateFrameworkTest.NoRedundantUpdates' --gtest_repeat=300` Thanks, Andrei Sekretenko
Re: Review Request 72824: Eliminated redundant update broadcasts in UPDATE_FRAMEWORK.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72824/ --- (Updated Sept. 2, 2020, 2:08 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Expanded comments, added logging. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch makes the UPDATE_FRAMEWORK call avoid sending the `UpdatedFrameworkMessage` to agents and the `FRAMEWORK_UPDATED` event to V1 API subscribers when the FrameworkInfo is not changed by the call. Diffs (updated) - src/master/master.cpp 02723296e569fac9d553b1494a5ca7daa6ef9aa4 Diff: https://reviews.apache.org/r/72824/diff/4/ Changes: https://reviews.apache.org/r/72824/diff/3-4/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72824: Eliminated redundant update broadcasts in UPDATE_FRAMEWORK.
> On Aug. 31, 2020, 9:14 p.m., Benjamin Mahler wrote: > > src/master/master.cpp > > Line 3291 (original), 3301 (patched) > > <https://reviews.apache.org/r/72824/diff/1/?file=2238955#file2238955line3301> > > > > The PID could have changed, we can probably just remove that PID code > > within `sendFrameworkUpdates` at this point. > > > > With the PID code still there, one can see that this if condition is > > not capturing the potential PID change we'd want to tell the agent about. Hmm... I'm afraid I do not understand your point. First: can we really stop sending V0 framwork PID updates to agents? The comment around the framework pid in the messages sent by master to an agent doesn't say anything about the PID of the V0 frameworks, does it? Also, it looks like the agent code is using the framework PID for sending executor messages to the V0 frameworks bypassing the master. Although removing the PID update in `sendFrameworkUpdates()` doesn't break any tests, a naive attempt to remove the framework PID from the agent does. I haven't dived deep into that, but it looks like some tests depend on that bypass; my suspicion is that this V0 PID update is just not covered by tests. Second: why are you asking about the potential PID change in the `UpdateFramework` call? I'm looking at `Master::receive()` https://github.com/apache/mesos/blob/a16f3439dca13982bb4a2b9190c24aaf4eb73b0e/src/master/master.cpp#L2386 and not getting how a V0 framework could change its PID without resubscribing... - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72824/#review221760 ------- On Sept. 2, 2020, 2:08 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72824/ > --- > > (Updated Sept. 2, 2020, 2:08 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10166 > https://issues.apache.org/jira/browse/MESOS-10166 > > > Repository: mesos > > > Description > --- > > This patch makes the UPDATE_FRAMEWORK call avoid sending the > `UpdatedFrameworkMessage` to agents and the `FRAMEWORK_UPDATED` > event to V1 API subscribers when the FrameworkInfo is not changed > by the call. > > > Diffs > - > > src/master/master.cpp 02723296e569fac9d553b1494a5ca7daa6ef9aa4 > > > Diff: https://reviews.apache.org/r/72824/diff/4/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72822: Extracted FrameworkInfo comparison logic from tests.
> On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.hpp
> > Lines 142-159 (patched)
> > <https://reviews.apache.org/r/72822/diff/1/?file=2238951#file2238951line142>
> >
> > In type_utils.hpp, we have a lot of operator == overloads for all of
> > our protobuf types, including (a broken) one for FrameworkInfo. Any
> > thoughts on why we might want to break the consistency here?
> >
> > Perhaps we should fix the existing one? For diffs, maybe we have a
> > bunch of `typeutils::diff(X x1, X x2)` overloads in that file?
>
> Andrei Sekretenko wrote:
> Yes, I'm also wondering what to do with the supposedly broken `operator
> == ` for FrameworkInfo declared in the public header that has been "broken"
> since at least 2012.
>
> By "fixing" that operator (and adjusting all the Mesos tests that rely on
> its behaviour), we might silently break external code that relies on the
> current semantics.
>
> Maybe we should simply remove this overload, so that any depending code
> fails to compile?
>
> In that case, we could replace it with `typeutils::equivalent()` in that
> file, so that the potential replacement is easy to find, and also add
> `typeutils::diff()` there.
>
> --
> In either case, switching our tests to comparison backed by
> MessageDifferencer - regardless of how we name it - sounds like a good idea.
> This means that leaving `operator ==` like this is not an option,
> otherwise it will be accidentally used somewhere else again.
>
> Benjamin Mahler wrote:
> I don't think we need to worry about external code (if by that, you mean
> any users of our public headers). We don't bother with any compatibility
> there.
>
> I think == is a bit easier to use but I guess the downside there is one
> might just assume it's more of an "exactly equals" than "equivalent", so
> having a more explicit name seems good.
Moved `equivalent`/`diff` into `type_utils.hpp` (and, for v1, into
`v1/mesos.hpp`).
After adjusting the existing tests that were relying on the stale `operator==`
(and that actually need to compare `FrameworkInfo`s in slightly different
ways), I'm even more in favour of `equivalent`.
The next patch (https://reviews.apache.org/r/72833) removes the `operator==`
and fixes the tests.
> On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.cpp
> > Lines 155-156 (patched)
> > <https://reviews.apache.org/r/72822/diff/1/?file=2238952#file2238952line155>
> >
> > Is this saying that MessageDifferencer is thread safe or not thread
> > safe?
> >
> > I guess this comment is commenting on why we don't have a function like
> > the following?
> >
> > ```
> > MessageDifferencer frameworkInfoDifferencer()
> > {
> > MessageDifferencer differencer;
> > // set it up
> > return differencer;
> > }
> > ```
Yes, exactly.
Extended the comment (added a conditional TODO in case MessageDifferencer
becomes move-constructible some day).
> On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.cpp
> > Lines 174-176 (patched)
> > <https://reviews.apache.org/r/72822/diff/1/?file=2238952#file2238952line174>
> >
> > Perhaps a comment saying that this treats unset the same as set to
> > default?
> >
> > Do we want that?
> >
> > We often use set-ness checks:
> >
> > if (x.has_foo()) {
> > // do something
> > }
> >
> > so that might make set to default different than not set?
> >
> > Perhaps we should just err on the side of more strict equality here and
> > use EQUALS?
>
> Andrei Sekretenko wrote:
> That's a good question whether we actually want this, especially given
> that API endpoints tend to report the missing (i.e. not set by the framework)
> fields as set to their default value.
>
> I'll need to investigate how our tests handle that.
>
> If removing `EQUIVALENT` boils down to only adjusting a couple of tests,
> I'll just remove it.
>
> Benjamin Mahler wrote:
> I guess in this line of thinking we need to distinguish between field
> presence checks for built in types vs sub-messages. For the latter, we
> definitely do a bunch of presence checks, but I'm not sure if we do it as
> often for `string`, `int32` etc fields.
Removed `EQUIVALENT`, as it helps to avoid only a single trivial adjustment in
two tests (`che
Review Request 72833: Removed `bool operator==(const FrameworkInfo&, const FrameworkInfo&)`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72833/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch removes the outdated `operator==` for `FrameworkInfo` (which has been comparing only `name` and `user` fields) and replaces it with `equivalent()` in the tests that need to compare `FrameworkInfo`s. Diffs - include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 src/tests/api_tests.cpp e0ce0335bf99f1f624efcca8c86f797df472d68f src/tests/master_allocator_tests.cpp 416b7ba733c3a9fc75e64fecf088ff13548bab3f src/tests/resources_tests.cpp bd044310e297174155b88d5694641acd5df4cf59 Diff: https://reviews.apache.org/r/72833/diff/1/ Testing --- `support/mesos-gtest-runner.py src/mesos-tests` Thanks, Andrei Sekretenko
Re: Review Request 72822: Extracted FrameworkInfo comparison logic from tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72822/ --- (Updated Sept. 1, 2020, 7:45 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Moved from `protobuf_utils` into `type_utils`/`v1/mesos.hpp`. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch extracts the code that performs `FrameworkInfo` comparison via protobuf `MessageDifferencer` from the framework update tests into a set of general-purpose methods. Diffs (updated) - include/mesos/type_utils.hpp da9fd9694b08827b968514b4b078097c912640c8 include/mesos/v1/mesos.hpp d8304f10d6ac6bbcf014e515e0bfd77a2b96e138 src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63 src/common/type_utils.cpp bb437844183688e7f69eb0874d87efe5a7d6daf2 src/common/type_utils_impl.hpp PRE-CREATION src/tests/master/update_framework_tests.cpp d2a63b696e976d9e3c729390d80f33180ab4aa1c src/v1/mesos.cpp c2fb528d323899c42b32c0713b38ebf6f909dd29 Diff: https://reviews.apache.org/r/72822/diff/3/ Changes: https://reviews.apache.org/r/72822/diff/2-3/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72822: Extracted FrameworkInfo comparison logic from tests.
> On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.hpp
> > Lines 142-159 (patched)
> > <https://reviews.apache.org/r/72822/diff/1/?file=2238951#file2238951line142>
> >
> > In type_utils.hpp, we have a lot of operator == overloads for all of
> > our protobuf types, including (a broken) one for FrameworkInfo. Any
> > thoughts on why we might want to break the consistency here?
> >
> > Perhaps we should fix the existing one? For diffs, maybe we have a
> > bunch of `typeutils::diff(X x1, X x2)` overloads in that file?
Yes, I'm also wondering what to do with the supposedly broken `operator == `
for FrameworkInfo declared in the public header that has been "broken" since at
least 2012.
By "fixing" that operator (and adjusting all the Mesos tests that rely on its
behaviour), we might silently break external code that relies on the current
semantics.
Maybe we should simply remove this overload, so that any depending code fails
to compile?
In that case, we could replace it with `typeutils::equivalent()` in that file,
so that the potential replacement is easy to find, and also add
`typeutils::diff()` there.
--
In either case, switching our tests to comparison backed by MessageDifferencer
- regardless of how we name it - sounds like a good idea.
This means that leaving `operator ==` like this is not an option, otherwise it
will be accidentally used somewhere else again.
> On Aug. 31, 2020, 9:05 p.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.cpp
> > Lines 174-176 (patched)
> > <https://reviews.apache.org/r/72822/diff/1/?file=2238952#file2238952line174>
> >
> > Perhaps a comment saying that this treats unset the same as set to
> > default?
> >
> > Do we want that?
> >
> > We often use set-ness checks:
> >
> > if (x.has_foo()) {
> > // do something
> > }
> >
> > so that might make set to default different than not set?
> >
> > Perhaps we should just err on the side of more strict equality here and
> > use EQUALS?
That's a good question whether we actually want this, especially given that API
endpoints tend to report the missing (i.e. not set by the framework) fields as
set to their default value.
I'll need to investigate how our tests handle that.
If removing `EQUIVALENT` boils down to only adjusting a couple of tests, I'll
just remove it.
- Andrei
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72822/#review221758
---
On Aug. 31, 2020, 6:03 p.m., Andrei Sekretenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72822/
> ---
>
> (Updated Aug. 31, 2020, 6:03 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-10166
> https://issues.apache.org/jira/browse/MESOS-10166
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch extracts the code that performs `FrameworkInfo` comparison
> via protobuf `MessageDifferencer` from the framework update tests
> into a set of general-purpose methods.
>
>
> Diffs
> -
>
> src/common/protobuf_utils.hpp 0558249023b39863b4030ddbcfe5c83dc945570a
> src/common/protobuf_utils.cpp 723d85a8656e61f77ab99e5e63f844ec95303ff0
> src/tests/master/update_framework_tests.cpp
> 6346a4eaf2b6c70d1a7d5f32706e781436d36521
>
>
> Diff: https://reviews.apache.org/r/72822/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Review Request 72826: Augmented `MasterAPITest.FrameworksEvent` to verify reregistration time.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72826/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch makes the test additionally check that the FRAMEWORK_UPDATED event reports a reregistration time larger than the registration time. Diffs - src/tests/api_tests.cpp f222b43d1077d060c96daa81eb3b48b5bb76a05d Diff: https://reviews.apache.org/r/72826/diff/1/ Testing --- `src/mesos-tests --gtest_filter='*MasterAPITest.FrameworksEvent*' --gtest_repeat=300` Thanks, Andrei Sekretenko
Review Request 72825: Added a test of UPDATE_FRAMEWORK with the same FrameworkInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72825/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This test verifies that calling UPDATE_FRAMEWORK with no changes in `FrameworkInfo` (i.e. to change suppressed roles or offer constraints) results in no API event FRAMEWORK_UPDATED sent to the subscribers and no `UpdatedFrameworkMessage` broadcast to the agents. Diffs - src/tests/master/update_framework_tests.cpp 6346a4eaf2b6c70d1a7d5f32706e781436d36521 Diff: https://reviews.apache.org/r/72825/diff/1/ Testing --- `src/mesos-tests --gtest_filter='UpdateFrameworkTest.NoRedundantUpdates' --gtest_repeat=300` Thanks, Andrei Sekretenko
Review Request 72824: Eliminated redundant update broadcasts in UPDATE_FRAMEWORK.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72824/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch makes the UPDATE_FRAMEWORK call avoid sending the `UpdatedFrameworkMessage` to agents and the `FRAMEWORK_UPDATED` event to V1 API subscribers when the FrameworkInfo is not changed by the call. Diffs - src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e Diff: https://reviews.apache.org/r/72824/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72823: Made UpdateFramework tests not rely on redundant FRAMEWORK_UPDATED.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72823/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch adjusts UpdateFramework tests so that they do not rely on an API subscriber receiving the FRAMEWORK_UPDATED event when the UPDATE_FRAMEWORK call does not change the `FrameworkInfo`. This is a prerequisite for removing redundant updates and broadcasts when UPDATE_FRAMEWORK leaves `FrameworkInfo` the same. Diffs - src/tests/master/update_framework_tests.cpp 6346a4eaf2b6c70d1a7d5f32706e781436d36521 Diff: https://reviews.apache.org/r/72823/diff/1/ Testing --- Thanks, Andrei Sekretenko
Review Request 72822: Extracted FrameworkInfo comparison logic from tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72822/ --- Review request for mesos and Benjamin Mahler. Bugs: MESOS-10166 https://issues.apache.org/jira/browse/MESOS-10166 Repository: mesos Description --- This patch extracts the code that performs `FrameworkInfo` comparison via protobuf `MessageDifferencer` from the framework update tests into a set of general-purpose methods. Diffs - src/common/protobuf_utils.hpp 0558249023b39863b4030ddbcfe5c83dc945570a src/common/protobuf_utils.cpp 723d85a8656e61f77ab99e5e63f844ec95303ff0 src/tests/master/update_framework_tests.cpp 6346a4eaf2b6c70d1a7d5f32706e781436d36521 Diff: https://reviews.apache.org/r/72822/diff/1/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72786: Implemented regex-based offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/ --- (Updated Aug. 28, 2020, 12:41 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Moved the `OfferConstraintsFilter::Options` definition into `allocator.hpp`. Left TODO for considering decoupling the filter creation interface in case it doesn't eventually get decoupled for other reasons. Bugs: MESOS-10173 https://issues.apache.org/jira/browse/MESOS-10173 Repository: mesos Description --- Implemented regex-based offer constraints. Diffs (updated) - include/mesos/allocator/allocator.hpp 05d0e9c46485c3048a65d46747d56e715883a0b3 src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b src/master/allocator/mesos/offer_constraints_filter.cpp d724fd0a9832feea26f6db1a498b6bdee830 src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e src/tests/master/offer_constraints_filter_tests.cpp 84a1e917a52d0b98f979e454c2b982c6402b0176 Diff: https://reviews.apache.org/r/72786/diff/10/ Changes: https://reviews.apache.org/r/72786/diff/9-10/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72786: Implemented regex-based offer constraints.
> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/offer_constraints_filter.hpp > > Lines 27 (patched) > > <https://reviews.apache.org/r/72786/diff/7/?file=2238780#file2238780line27> > > > > Hm.. why is this decoupled into a different header from > > OfferConstraintsFilter? Is it to minimize the stuff pulled into master.hpp? > > If so, not sure if that's worth the extra indirection? > > Andrei Sekretenko wrote: > Right at this moment, this also helps minimize things that are kept in > the public header `` (which, for example, is > included into something like 1/3 of the volume of our tests, directly or > indirectly). > > Benjamin Mahler wrote: > Oh, just to minimize the content of ``? > We're only saving 1 small struct here, doesn't seem worth it? Or have you > seen it actually make a difference in compile times? > > I would suspect we have better low hanging fruit for improving compile > times more generally? This is not a question of a clean compile time but of a recompile time. When any change in the filter creation interface - an internal interface which is absolutely of no concern to the code that mocks `TestAllocator` and mostly of no concern to all the other stuff that includes `allocator.hpp` - results in recompiling 1/4 of the project, this just does not look right. Sure, we have a very similar issue with protobufs. However, protobufs are by design essentially append-only, which is not the case with the interfaces internal to Mesos (or, at least, should not be the case). Overall, in the public headers we have around 140 non-protobuf class/struct definitions, most of which are really needed there (used by modules/schedulers). -- On the other hand, the construction interface will probably end up converted into a factory-like entity in future. Given that this is likely to happen before we add more options, I'm inclined to stop thinking about this particular case and just define `struct Options` in this header, but leave a TODO for pulling the construction interface away from the public header in case we find ourselves adding more options and this `struct Options` is still here. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/#review221727 ------- On Aug. 27, 2020, 11:52 a.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72786/ > --- > > (Updated Aug. 27, 2020, 11:52 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10173 > https://issues.apache.org/jira/browse/MESOS-10173 > > > Repository: mesos > > > Description > --- > > Implemented regex-based offer constraints. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > 05d0e9c46485c3048a65d46747d56e715883a0b3 > src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b > src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION > src/master/allocator/mesos/offer_constraints_filter.cpp > d724fd0a9832feea26f6db1a498b6bdee830 > src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b > src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 > src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a > src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 > src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e > src/tests/master/offer_constraints_filter_tests.cpp > 84a1e917a52d0b98f979e454c2b982c6402b0176 > > > Diff: https://reviews.apache.org/r/72786/diff/9/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72786: Implemented regex-based offer constraints.
> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/offer_constraints_filter.hpp > > Lines 27 (patched) > > <https://reviews.apache.org/r/72786/diff/7/?file=2238780#file2238780line27> > > > > Hm.. why is this decoupled into a different header from > > OfferConstraintsFilter? Is it to minimize the stuff pulled into master.hpp? > > If so, not sure if that's worth the extra indirection? Right at this moment, this also helps minimize things that are kept in the public header `` (which, for example, is included into something like 1/3 of the volume of our tests, directly or indirectly). > On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/offer_constraints_filter.cpp > > Lines 68 (patched) > > <https://reviews.apache.org/r/72786/diff/7/?file=2238781#file2238781line68> > > > > No period at the end > > > > Can we include the program size value and max? > > > > E.g. > > > > Regex 'foo|bar' is too complex: program size of 112 vs maximum of 100 > > allowed via --offer_constraints_re2_max_program_size The value and the limit definitely make sense! Added both. Not sure if we want to add one more location that contains the non-local knowledge about how exactly the limit is configured. I would hope that if a regex breaches the limit, the author of the regex in most cases will be able to reformulate the constraints instead of pushing the cluster administrator to increase the global limit... > On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote: > > src/tests/master/offer_constraints_filter_tests.cpp > > Lines 46 (patched) > > <https://reviews.apache.org/r/72786/diff/7/?file=2238787#file2238787line46> > > > > Should we have the type of this also be Bytes, or clarify the units in > > the name? > > > > ``` > > options.re2Limits.maxMem = Kilobytes(4); > > ``` > > > > ``` > > options.re2Limits.maxMemInBytes = 4 * 1024; > > ``` Thanks, looks like I forgot to adjust this location; the first option is clearly better. Makes me wonder if the `Bytes(unit64_t)` constructor should be made `explicit`... - Andrei ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/#review221727 --- On Aug. 27, 2020, 11:52 a.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72786/ > --- > > (Updated Aug. 27, 2020, 11:52 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10173 > https://issues.apache.org/jira/browse/MESOS-10173 > > > Repository: mesos > > > Description > --- > > Implemented regex-based offer constraints. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > 05d0e9c46485c3048a65d46747d56e715883a0b3 > src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b > src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION > src/master/allocator/mesos/offer_constraints_filter.cpp > d724fd0a9832feea26f6db1a498b6bdee830 > src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b > src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 > src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a > src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 > src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e > src/tests/master/offer_constraints_filter_tests.cpp > 84a1e917a52d0b98f979e454c2b982c6402b0176 > > > Diff: https://reviews.apache.org/r/72786/diff/9/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72786: Implemented regex-based offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/ --- (Updated Aug. 27, 2020, 11:52 a.m.) Review request for mesos and Benjamin Mahler. Bugs: MESOS-10173 https://issues.apache.org/jira/browse/MESOS-10173 Repository: mesos Description --- Implemented regex-based offer constraints. Diffs (updated) - include/mesos/allocator/allocator.hpp 05d0e9c46485c3048a65d46747d56e715883a0b3 src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION src/master/allocator/mesos/offer_constraints_filter.cpp d724fd0a9832feea26f6db1a498b6bdee830 src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e src/tests/master/offer_constraints_filter_tests.cpp 84a1e917a52d0b98f979e454c2b982c6402b0176 Diff: https://reviews.apache.org/r/72786/diff/8/ Changes: https://reviews.apache.org/r/72786/diff/7-8/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72786: Implemented regex-based offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/ --- (Updated Aug. 26, 2020, 12:23 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Replaced filter factory with `create()` options. Bugs: MESOS-10173 https://issues.apache.org/jira/browse/MESOS-10173 Repository: mesos Description --- Implemented regex-based offer constraints. Diffs (updated) - include/mesos/allocator/allocator.hpp 05d0e9c46485c3048a65d46747d56e715883a0b3 src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION src/master/allocator/mesos/offer_constraints_filter.cpp d724fd0a9832feea26f6db1a498b6bdee830 src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e src/tests/master/offer_constraints_filter_tests.cpp 84a1e917a52d0b98f979e454c2b982c6402b0176 Diff: https://reviews.apache.org/r/72786/diff/7/ Changes: https://reviews.apache.org/r/72786/diff/6-7/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72786: Implemented regex-based offer constraints.
> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote: > > Hm.. is it possible to have a simpler patch that avoids the factory and > > instead passes the flags through the creation path? > > Andrei Sekretenko wrote: > This is possible; however, I would argue that putting limits on RE2 is > rather deep implemenation detail which should not pollute the **public > allocator interface**. > > Given this, and also the need for caching, I'm not sure that passing the > flags through creation path and removing them right after that makes a lot of > sense. Realized two things: - that I will end up refactoring the factory when adding caching if I leave the factory like this - that there is no need to have the definiotion of the filter options in the public allocator interface; they can be just forward-declared. Removed the factory for now. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/#review221697 ------- On Aug. 26, 2020, 12:23 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72786/ > --- > > (Updated Aug. 26, 2020, 12:23 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10173 > https://issues.apache.org/jira/browse/MESOS-10173 > > > Repository: mesos > > > Description > --- > > Implemented regex-based offer constraints. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > 05d0e9c46485c3048a65d46747d56e715883a0b3 > src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b > src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION > src/master/allocator/mesos/offer_constraints_filter.cpp > d724fd0a9832feea26f6db1a498b6bdee830 > src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b > src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 > src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a > src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 > src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e > src/tests/master/offer_constraints_filter_tests.cpp > 84a1e917a52d0b98f979e454c2b982c6402b0176 > > > Diff: https://reviews.apache.org/r/72786/diff/7/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72786: Implemented regex-based offer constraints.
> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/offer_constraints_filter.cpp > > Lines 113-119 (patched) > > <https://reviews.apache.org/r/72786/diff/5/?file=2238651#file2238651line113> > > > > How did you figure this out? Just from your own testing? > > > > What about with clang? Same result? > > > > How much faster? Is it worth bothering? > > Andrei Sekretenko wrote: > Around 10% of the allocation loop time in a rather pecualr, although not > exotic, situation: a fast regexp that filters out most of the agents. > Probably not worth bothering until we have a well-parametrized benchmark. > > Maybe instead I should figure out if -O3 (which is the default in ther > Makefile, as opposed to cMake with RelWithDebInfo, which by defautl uses -O2) > can get us rid of this buffer zeroing: > https://github.com/google/re2/blob/ca11026a032ce2a3de4b3c389ee53d2bdc8794d6/re2/re2.cc#L907 > in the case of FullMatch. > > If it does, it might be worth making the CMake build less consistent > internally but more consistent with the "standard" build flags of RE2. Hmm.. now I don't see such a large effect with the draft benchmark I have at hand; either I've lost the version that was exhibiting that 10% difference or that was a measurement error. Tried -O3, that doesn't help to optimize out that `memset` in `RE2::DoMatch()`. Dropped this wrapper. > On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote: > > src/master/constants.hpp > > Lines 182 (patched) > > <https://reviews.apache.org/r/72786/diff/5/?file=2238652#file2238652line182> > > > > regular > > > > Anything you can add about how these numbers were produced? Would be > > helpful. Added an example regex similar to what we saw in some of the real-world Marathon apps and extracted the minimum max_mem under which it constructs successfully. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/#review221697 --- On Aug. 25, 2020, 6:43 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72786/ > --- > > (Updated Aug. 25, 2020, 6:43 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10173 > https://issues.apache.org/jira/browse/MESOS-10173 > > > Repository: mesos > > > Description > --- > > Implemented regex-based offer constraints. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > 05d0e9c46485c3048a65d46747d56e715883a0b3 > src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b > src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION > src/master/allocator/mesos/offer_constraints_filter.cpp > d724fd0a9832feea26f6db1a498b6bdee830 > src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b > src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 > src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a > src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 > src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e > src/tests/master/offer_constraints_filter_tests.cpp > 84a1e917a52d0b98f979e454c2b982c6402b0176 > > > Diff: https://reviews.apache.org/r/72786/diff/6/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72786: Implemented regex-based offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/ --- (Updated Aug. 25, 2020, 6:43 p.m.) Review request for mesos and Benjamin Mahler. Changes --- - switched the `max_mem` flag to `Bytes`, fixed the type of `DEFAULT_OFFER_CONSTRAINTS_RE2_MAX_PROGRAM_SIZE` - dropped the local `fullMatch()` wrapper Bugs: MESOS-10173 https://issues.apache.org/jira/browse/MESOS-10173 Repository: mesos Description --- Implemented regex-based offer constraints. Diffs (updated) - include/mesos/allocator/allocator.hpp 05d0e9c46485c3048a65d46747d56e715883a0b3 src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION src/master/allocator/mesos/offer_constraints_filter.cpp d724fd0a9832feea26f6db1a498b6bdee830 src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e src/tests/master/offer_constraints_filter_tests.cpp 84a1e917a52d0b98f979e454c2b982c6402b0176 Diff: https://reviews.apache.org/r/72786/diff/6/ Changes: https://reviews.apache.org/r/72786/diff/5-6/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72784: Added protobuf messages for regex-based offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72784/ --- (Updated Aug. 25, 2020, 6:38 p.m.) Review request for mesos and Benjamin Mahler. Changes --- Extended the comment on RE2 limits Bugs: MESOS-10173 https://issues.apache.org/jira/browse/MESOS-10173 Repository: mesos Description --- This patch adds protobuf messages for setting offer constraints that check if agent's (pseudo)attribute matches a specified RE2 regular expression. Both added contsraint predicates will evaluate to `true` when the attribute is not TEXT. This way, schedulers will have to apply on their own whatever filtration they do for non-TEXT attributes which happen to be selected by the constraint's `Selector`. Given that in the real world schedulers seem to rarely put constraints on attributes that are normally Scalar/Ranges, this should not prevent them from obtaining performance benefits by setting offer constraints. Diffs (updated) - include/mesos/scheduler/scheduler.proto 06d62f5f4ed9286a6ad50f80d1261e4e732d1bc7 include/mesos/v1/scheduler/scheduler.proto 08fb10a146e0b57896a0cb098178a40e618df392 Diff: https://reviews.apache.org/r/72784/diff/4/ Changes: https://reviews.apache.org/r/72784/diff/3-4/ Testing --- Thanks, Andrei Sekretenko
Re: Review Request 72786: Implemented regex-based offer constraints.
> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote: > > Hm.. is it possible to have a simpler patch that avoids the factory and > > instead passes the flags through the creation path? This is possible; however, I would argue that putting limits on RE2 is rather deep implemenation detail which should not pollute the **public allocator interface**. Given this, and also the need for caching, I'm not sure that passing the flags through creation path and removing them right after that makes a lot of sense. > On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/offer_constraints_filter.cpp > > Lines 113-119 (patched) > > <https://reviews.apache.org/r/72786/diff/5/?file=2238651#file2238651line113> > > > > How did you figure this out? Just from your own testing? > > > > What about with clang? Same result? > > > > How much faster? Is it worth bothering? Around 10% of the allocation loop time in a rather pecualr, although not exotic, situation: a fast regexp that filters out most of the agents. Probably not worth bothering until we have a well-parametrized benchmark. Maybe instead I should figure out if -O3 (which is the default in ther Makefile, as opposed to cMake with RelWithDebInfo, which by defautl uses -O2) can get us rid of this buffer zeroing: https://github.com/google/re2/blob/ca11026a032ce2a3de4b3c389ee53d2bdc8794d6/re2/re2.cc#L907 in the case of FullMatch. If it does, it might be worth making the CMake build less consistent internally but more consistent with the "standard" build flags of RE2. > On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote: > > src/master/flags.hpp > > Lines 125-126 (patched) > > <https://reviews.apache.org/r/72786/diff/5/?file=2238653#file2238653line125> > > > > Just noticed that these don't line up with the above constant types, is > > that intentional? > > > > Can we make max_mem of type `Bytes`? That would let a user specify > > "4kb", "4MB" etc These are types are identical to those used in RE2. I'll try `Bytes`, though; if flags work with that, that would be great. - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/#review221697 --- On Aug. 24, 2020, 7:46 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72786/ > --- > > (Updated Aug. 24, 2020, 7:46 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10173 > https://issues.apache.org/jira/browse/MESOS-10173 > > > Repository: mesos > > > Description > --- > > Implemented regex-based offer constraints. > > > Diffs > - > > include/mesos/allocator/allocator.hpp > 05d0e9c46485c3048a65d46747d56e715883a0b3 > src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b > src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION > src/master/allocator/mesos/offer_constraints_filter.cpp > ef8a948260d92a9d54efb65f0e5c8aa3d0c67b91 > src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b > src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 > src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a > src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 > src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e > src/tests/master/offer_constraints_filter_tests.cpp > f88e2011bc510323344ffe58fd550b7683242950 > > > Diff: https://reviews.apache.org/r/72786/diff/5/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72776: Added basic tests for offer constraints on a string attribute equality.
> On Aug. 24, 2020, 9:14 p.m., Benjamin Mahler wrote: > > Do we also want to be checking pseudoattributes? (Different enums and > > handling optionality (e.g. domain)). We surely need. Added https://issues.apache.org/jira/browse/MESOS-10180 to the epic for this (and for other poorly covered functionality). - Andrei --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72776/#review221695 --- On Aug. 24, 2020, 7:35 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72776/ > --- > > (Updated Aug. 24, 2020, 7:35 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10172 > https://issues.apache.org/jira/browse/MESOS-10172 > > > Repository: mesos > > > Description > --- > > Added basic tests for offer constraints on a string attribute equality. > > > Diffs > - > > src/tests/master/offer_constraints_filter_tests.cpp > f88e2011bc510323344ffe58fd550b7683242950 > > > Diff: https://reviews.apache.org/r/72776/diff/4/ > > > Testing > --- > > `support/mesos-gtest-runner.py src/mesos-tests` > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72786: Implemented regex-based offer constraints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72786/ --- (Updated Aug. 24, 2020, 7:46 p.m.) Review request for mesos and Benjamin Mahler. Changes --- - Separated regex caching from the initial implementation of the regex constraints. - Parametrized the RE2 limits. - Adjusted for constraints rename. Bugs: MESOS-10173 https://issues.apache.org/jira/browse/MESOS-10173 Repository: mesos Description --- Implemented regex-based offer constraints. Diffs (updated) - include/mesos/allocator/allocator.hpp 05d0e9c46485c3048a65d46747d56e715883a0b3 src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION src/master/allocator/mesos/offer_constraints_filter.cpp ef8a948260d92a9d54efb65f0e5c8aa3d0c67b91 src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e src/tests/master/offer_constraints_filter_tests.cpp f88e2011bc510323344ffe58fd550b7683242950 Diff: https://reviews.apache.org/r/72786/diff/5/ Changes: https://reviews.apache.org/r/72786/diff/4-5/ Testing --- Thanks, Andrei Sekretenko
