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 Bannier <be...@mesosphere.io> on 2017/01/12 14:54:14 UTC
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/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
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 1746a88953dbdc148d98881bcf7027b62ad6b040
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 Jan. 13, 2017, 9:17 a.m., Jay Guo wrote:
> > When a resource with `*` is offered to a multi-role framework, how does the framework decide which role to reserve the resource for?
Frameworks with default role cannot reserve resources; this is the first check in the validation function being updated in this patch.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161489
-----------------------------------------------------------
On Jan. 13, 2017, 4:50 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 4:50 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 1746a88953dbdc148d98881bcf7027b62ad6b040
> 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 Jay Guo <gu...@gmail.com>.
> On Jan. 13, 2017, 4:17 p.m., Jay Guo wrote:
> > When a resource with `*` is offered to a multi-role framework, how does the framework decide which role to reserve the resource for?
>
> Benjamin Bannier wrote:
> Frameworks with default role cannot reserve resources; this is the first check in the validation function being updated in this patch.
For example. My framework has role `foo`, `bar` and `*`. I get an offer with resource of role `*`. I want to reserve it for `foo`. Previously I simply issue `RESERVE` operation and the resource will be reserved for the ONLY role I have. But in multi-role scenario, how should I tell which role I want to reserve the resource for?
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161489
-----------------------------------------------------------
On Jan. 13, 2017, 11:50 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 11:50 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 1746a88953dbdc148d98881bcf7027b62ad6b040
> 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 Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161489
-----------------------------------------------------------
When a resource with `*` is offered to a multi-role framework, how does the framework decide which role to reserve the resource for?
- Jay Guo
On Jan. 12, 2017, 10:54 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> 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 1746a88953dbdc148d98881bcf7027b62ad6b040
> 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 Jan. 13, 2017, 9:15 a.m., Jay Guo wrote:
> > src/master/master.cpp, lines 3944-3950
> > <https://reviews.apache.org/r/55461/diff/1/?file=1603853#file1603853line3944>
> >
> > I think we explicitly disallow empyt role field? https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
> > Also, should the case `framework->info.has_role() == false && framework->info.role() != "*"` be invalid and caught upon subscription?
Multi-role frameworks would use the `roles` field, in which case the `role` field would not be set. We need to distinguish here whether a framework uses `role` or `roles`. The check can be simplified to `framework->info.role() != "*"` though, independently of what we assume about this field upstream. I made that change.
Does that address your comment?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161488
-----------------------------------------------------------
On Jan. 13, 2017, 4:50 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 4:50 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 1746a88953dbdc148d98881bcf7027b62ad6b040
> 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 Jan. 13, 2017, 9:15 a.m., Jay Guo wrote:
> > src/master/master.cpp, lines 3944-3950
> > <https://reviews.apache.org/r/55461/diff/1/?file=1603853#file1603853line3944>
> >
> > I think we explicitly disallow empyt role field? https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
> > Also, should the case `framework->info.has_role() == false && framework->info.role() != "*"` be invalid and caught upon subscription?
>
> Benjamin Bannier wrote:
> Multi-role frameworks would use the `roles` field, in which case the `role` field would not be set. We need to distinguish here whether a framework uses `role` or `roles`. The check can be simplified to `framework->info.role() != "*"` though, independently of what we assume about this field upstream. I made that change.
>
> Does that address your comment?
>
> Jay Guo wrote:
> then why not using framework capability here? note that https://reviews.apache.org/r/55021/ is landed which you may want to use.
While this makes the code somewhat more explicit, I am not sure this would make the code more readable. If the framework was not multi-role, we'd still need to check `role != '*'` explicitly since we map `* -> {}` and `A -> {A}` when invoking `validate`. This code could e.g., look like
const hashset<string> frameworkRoles =
framework->capabilities.multiRole
? hashset<string>(
{framework->info.roles().begin(),
framework->info.roles().end()})
: framework->info.role() ! =
"*" ? hashset<string>({framework->info.role()})
: hashset<string>();
Let me know if you feel that that is more readable and I'll update the code.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161488
-----------------------------------------------------------
On Jan. 13, 2017, 4:50 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 4:50 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 1746a88953dbdc148d98881bcf7027b62ad6b040
> 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 Jay Guo <gu...@gmail.com>.
> On Jan. 13, 2017, 4:15 p.m., Jay Guo wrote:
> > src/master/master.cpp, lines 3944-3950
> > <https://reviews.apache.org/r/55461/diff/1/?file=1603853#file1603853line3944>
> >
> > I think we explicitly disallow empyt role field? https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
> > Also, should the case `framework->info.has_role() == false && framework->info.role() != "*"` be invalid and caught upon subscription?
>
> Benjamin Bannier wrote:
> Multi-role frameworks would use the `roles` field, in which case the `role` field would not be set. We need to distinguish here whether a framework uses `role` or `roles`. The check can be simplified to `framework->info.role() != "*"` though, independently of what we assume about this field upstream. I made that change.
>
> Does that address your comment?
then why not using framework capability here? note that https://reviews.apache.org/r/55021/ is landed which you may want to use.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161488
-----------------------------------------------------------
On Jan. 13, 2017, 11:50 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 11:50 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 1746a88953dbdc148d98881bcf7027b62ad6b040
> 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 Jay Guo <gu...@gmail.com>.
> On Jan. 13, 2017, 4:15 p.m., Jay Guo wrote:
> > src/master/master.cpp, lines 3944-3950
> > <https://reviews.apache.org/r/55461/diff/1/?file=1603853#file1603853line3944>
> >
> > I think we explicitly disallow empyt role field? https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
> > Also, should the case `framework->info.has_role() == false && framework->info.role() != "*"` be invalid and caught upon subscription?
>
> Benjamin Bannier wrote:
> Multi-role frameworks would use the `roles` field, in which case the `role` field would not be set. We need to distinguish here whether a framework uses `role` or `roles`. The check can be simplified to `framework->info.role() != "*"` though, independently of what we assume about this field upstream. I made that change.
>
> Does that address your comment?
>
> Jay Guo wrote:
> then why not using framework capability here? note that https://reviews.apache.org/r/55021/ is landed which you may want to use.
>
> Benjamin Bannier wrote:
> While this makes the code somewhat more explicit, I am not sure this would make the code more readable. If the framework was not multi-role, we'd still need to check `role != '*'` explicitly since we map `* -> {}` and `A -> {A}` when invoking `validate`. This code could e.g., look like
>
> const hashset<string> frameworkRoles =
> framework->capabilities.multiRole
> ? hashset<string>(
> {framework->info.roles().begin(),
> framework->info.roles().end()})
> : framework->info.role() ! =
> "*" ? hashset<string>({framework->info.role()})
> : hashset<string>();
>
> Let me know if you feel that that is more readable and I'll update the code.
Yeah I agree that yours looks more concise. Let's hear from other folks @gyliu @bmahler.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161488
-----------------------------------------------------------
On Jan. 16, 2017, 8: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, 8: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 Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review161488
-----------------------------------------------------------
src/master/master.cpp (lines 3944 - 3950)
<https://reviews.apache.org/r/55461/#comment232788>
I think we explicitly disallow empyt role field? https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L67
Also, should the case `framework->info.has_role() == false && framework->info.role() != "*"` be invalid and caught upon subscription?
- Jay Guo
On Jan. 12, 2017, 10:54 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
>
>
> 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 1746a88953dbdc148d98881bcf7027b62ad6b040
> 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
>
>
Re: Review Request 55461: Made resource reservation validation
multi-role aware.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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 Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/#review164823
-----------------------------------------------------------
Fix it, then Ship it!
Will make some adjustments based on the comment below.
src/master/validation.cpp (lines 1581 - 1593)
<https://reviews.apache.org/r/55461/#comment236610>
I think we can remove these per my previous comments.
For checking against the framework having no roles, this is a special case of the reservation role being absent from the framework's roles, which is caught below. Given that a framework with no roles would not receive offers, there doesn't seem to be a lot of value in special casing the error message for this with an additional check of the special case.
For checking against the framework having `{*}` role, it seems odd to check for the framework role's being `{*}` when `{*,foo}` has the same problem: the framework cannot make a reservation for `*`. So, this a special case of the general dynamic reservation check within `resources::validate()` and we can remove this check as well.
- Benjamin Mahler
On Feb. 8, 2017, 11:41 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 11:41 a.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
>
>
Re: Review Request 55461: Made resource reservation validation
multi-role aware.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 55461: Made resource reservation validation
multi-role aware.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 5: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 (updated)
-----
src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
src/master/validation.hpp fc1eaeebd0643b897f4e14aabc76e7d94f0de263
src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd
src/tests/master_validation_tests.cpp 1f833aa8d6cacbd7bf502fb66c556e4e3d4f79e2
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/
-----------------------------------------------------------
(Updated Jan. 16, 2017, 1:27 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Rebased.
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 (updated)
-----
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55461/
-----------------------------------------------------------
(Updated Jan. 13, 2017, 4:50 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
Changes
-------
Addressed review comment from guoger.
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 (updated)
-----
src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040
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