You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2017/02/06 21:28:41 UTC
Re: Review Request 55461: Made resource reservation validation
multi-role aware.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review164410
-----------------------------------------------------------
src/master/master.cpp (lines 3942 - 3947)
<https://reviews.apache.org/r/55461/#comment236112>
This isn't quite correct since we need to check the capability to determine which field to examine.
Per my suggestion below, can we pass the FrameworkInfo in rather than just the roles?
Also note that there is a `getRoles` helper in protobuf utils to simplify this. We should add a `set<string> roles` field to the `Framework` struct in the master though.
src/master/validation.hpp (lines 223 - 227)
<https://reviews.apache.org/r/55461/#comment236110>
Hm.. any reason this shouldn't follow the pattern used in the create validation right below where we take framework info? That seems to make it a bit clearer that the optional framework means it's none when done by operator.
E.g.
```
// Validates the RESERVE operation.
Option<Error> validate(
const Offer::Operation::Reserve& reserve,
const Option<std::string>& principal,
const Option<FrameworkInfo>& frameworkInfo = None());
```
src/master/validation.cpp (lines 1500 - 1503)
<https://reviews.apache.org/r/55461/#comment236113>
Hm.. do you know which call sites need to be updated? It seems like we need to do the sweep now to ensure the call-sites are correct, no?
src/master/validation.cpp (lines 1509 - 1513)
<https://reviews.apache.org/r/55461/#comment236115>
This condition isn't quite the `*` role, this would be a framework with no roles. The `*` role case is now the 1 element `*` set, but it seems to me this check should be looking at the resource.role() rather than the framework's roles, no?
- Benjamin Mahler
On Jan. 16, 2017, 12:27 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2017, 12:27 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
>
>
> Diffs
> -----
>
> src/master/master.cpp b863ff6e93931c3d1ee056248084c7f44caf2fd9
> src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
> src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb
>
> Diff: https://reviews.apache.org/r/55461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 55461: Made resource reservation validation
multi-role aware.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Feb. 6, 2017, 10:28 p.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1500-1503
> > <https://reviews.apache.org/r/55461/diff/3/?file=1605766#file1605766line1500>
> >
> > Hm.. do you know which call sites need to be updated? It seems like we need to do the sweep now to ensure the call-sites are correct, no?
This was an in process-`TODO`, the call sites should be updated now.
> On Feb. 6, 2017, 10:28 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 3942-3947
> > <https://reviews.apache.org/r/55461/diff/3/?file=1605764#file1605764line3942>
> >
> > This isn't quite correct since we need to check the capability to determine which field to examine.
> >
> > Per my suggestion below, can we pass the FrameworkInfo in rather than just the roles?
> >
> > Also note that there is a `getRoles` helper in protobuf utils to simplify this. We should add a `set<string> roles` field to the `Framework` struct in the master though.
Updated to pass an `Option<FrameworkInfo>`.
> On Feb. 6, 2017, 10:28 p.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 1509-1513
> > <https://reviews.apache.org/r/55461/diff/3/?file=1605766#file1605766line1509>
> >
> > This condition isn't quite the `*` role, this would be a framework with no roles. The `*` role case is now the 1 element `*` set, but it seems to me this check should be looking at the resource.role() rather than the framework's roles, no?
This definitely incorrectly handles single role frameworks in `{*}`. I updated the patch so we check for any both an empty roles set and `{*}` to handle both frameworks with no roles and frameworks in `{*}`. The cases where `*` is included in `roles` is supported, so framework roles `{*}` will remain a valid case even after `FrameworkInfo.role` is removed, right?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review164410
-----------------------------------------------------------
On Feb. 8, 2017, 12:41 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 12:41 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
>
>
> Diffs
> -----
>
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/master/validation.hpp fc1eaeebd0643b897f4e14aabc76e7d94f0de263
> src/master/validation.cpp 37d171512ee54f260aabb6e1071739bcc3769fb0
> src/tests/master_validation_tests.cpp 51185031cb67e64cd69ec6ce1c8f722a0c349970
>
> Diff: https://reviews.apache.org/r/55461/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Bannier
>
>