You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@apache.org> on 2020/10/12 20:12:32 UTC
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
Re: Review Request 72955: Consolidated creation and validation of
`allocator::Framework` options.
Posted by Andrei Sekretenko <as...@apache.org>.
> 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
>
>
Re: Review Request 72955: Consolidated creation and validation of
`allocator::Framework` options.
Posted by Andrei Sekretenko <as...@apache.org>.
> 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
>
>
Re: Review Request 72955: Consolidated creation and validation of
`allocator::Framework` options.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72955/#review222039
-----------------------------------------------------------
src/master/master.hpp
Lines 1061-1062 (original), 1061-1062 (patched)
<https://reviews.apache.org/r/72955/#comment311107>
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?
src/master/master.cpp
Lines 2588-2598 (original)
<https://reviews.apache.org/r/72955/#comment311108>
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?
src/master/master.cpp
Lines 2629-2634 (patched)
<https://reviews.apache.org/r/72955/#comment311109>
Hm.. shouldn't this stateless FrameworkInfo validation go in the existing `validation::framework::valdiate(const FrameworkInfo&)` function in master/validation.hpp?
- Benjamin Mahler
On Oct. 12, 2020, 8: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, 8: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
>
>
Re: Review Request 72955: Consolidated creation and validation of
`allocator::Framework` options.
Posted by Andrei Sekretenko <as...@apache.org>.
-----------------------------------------------------------
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 72955: Consolidated creation and validation of
`allocator::Framework` options.
Posted by Andrei Sekretenko <as...@apache.org>.
> 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<Error> 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.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72955/#review222060
-----------------------------------------------------------
Ship it!
src/master/master.hpp
Lines 1061-1062 (original), 1061-1062 (patched)
<https://reviews.apache.org/r/72955/#comment311125>
Perhaps:
```
Performs stateless and stateful validation of the FrameworkInfo.
The stateful validation only uses the master flags and whether
the framework is completed.
TODO: Consider passing in the state explicitly to move this
function into validation.hpp for unit testability.
```
src/master/master.cpp
Lines 2615-2616 (patched)
<https://reviews.apache.org/r/72955/#comment311126>
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<Error> 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.
src/master/validation.hpp
Lines 133-135 (patched)
<https://reviews.apache.org/r/72955/#comment311127>
Per the above comment, the nice part of our stateless validators is we can unit test them (if you want to add a quick one to this patch), and we only need to do the heavy integration test for the wiring.
src/master/validation.cpp
Lines 692-693 (patched)
<https://reviews.apache.org/r/72955/#comment311124>
don't know if we need the quotes here, since we're not showing a string field:
`Suppressed roles {"foo", "bar"} are not ...`
- Benjamin Mahler
On Oct. 14, 2020, 6: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, 6: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
>
>
Re: Review Request 72955: Consolidated creation and validation of
`allocator::Framework` options.
Posted by Andrei Sekretenko <as...@apache.org>.
-----------------------------------------------------------
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