You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jay Guo <gu...@gmail.com> on 2017/02/22 17:43:51 UTC
Review Request 56939: Validate that Resource.allocation_info.roles
are not mixed.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
Bugs: MESOS-6636
https://issues.apache.org/jira/browse/MESOS-6636
Repository: mesos
Description
-------
With support for multi-role frameworks, we need to make sure that
individual tasks and executors cannot mix roles. Likewise, we do
not want to allow a scheduler to make a reservation or create a
volume based on resources with different allocated roles.
Diffs
-----
src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
src/tests/master_validation_tests.cpp 53771f6b5492009fe75cbbfc03a2b542856c1070
Diff: https://reviews.apache.org/r/56939/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 56939: Validate that
Resource.allocation_info.roles are not mixed.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/#review166462
-----------------------------------------------------------
High level thoughts:
(1) Can you split out the validation for task / executor / volume / reservation? That will make it easier to review and commit them piece by piece.
(2) Looks like there are no tests for volumes and reservations?
src/master/validation.cpp (line 588)
<https://reviews.apache.org/r/56939/#comment238415>
How about 'validateSingleAllocationRole'?
src/master/validation.cpp (lines 602 - 603)
<https://reviews.apache.org/r/56939/#comment238417>
These error messages compose a bit oddly:
```
Executor mixes allocation roles: Resources mixes allocations roles: role1 and role2
```
How about:
```
Invalid executor resources: The resources have multiple allocation roles ('role1' and 'role2') but only one allocation role is allowed
```
Here `"Invalid executor resources:"` comes from the caller and `"The resources have multiple allocation roles ('role1' and 'role2') but only one allocation role is allowed"` comes from the helper function.
src/master/validation.cpp (line 1658)
<https://reviews.apache.org/r/56939/#comment238414>
Why couldn't you use `resource::validateAllocationRole(reserve.resources())` here?
- Benjamin Mahler
On Feb. 22, 2017, 5:43 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2017, 5:43 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With support for multi-role frameworks, we need to make sure that
> individual tasks and executors cannot mix roles. Likewise, we do
> not want to allow a scheduler to make a reservation or create a
> volume based on resources with different allocated roles.
>
>
> Diffs
> -----
>
> src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
> src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
> src/tests/master_validation_tests.cpp 53771f6b5492009fe75cbbfc03a2b542856c1070
>
> Diff: https://reviews.apache.org/r/56939/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/#review166749
-----------------------------------------------------------
Fix it, then Ship it!
src/master/validation.cpp (lines 587 - 610)
<https://reviews.apache.org/r/56939/#comment238787>
How about we validate that it's allocated, and it's allocated to 1 role, so something like `validateAllocatedToSingleRole`.
src/master/validation.cpp (lines 602 - 604)
<https://reviews.apache.org/r/56939/#comment238781>
We try to open and close quotes on the same line, since when on different lines we sometimes forget to close them. E.g.
```
"The resources have multiple allocation roles ('" + _role + "'"
" and '" + role.get() + "') but only one allocation role" +
" is allowed");
```
or:
```
"The resources have multiple allocation roles"
" ('" + _role + "' and '" + role.get() + "')"
" but only one allocation role is allowed");
```
src/tests/master_validation_tests.cpp (line 2230)
<https://reviews.apache.org/r/56939/#comment238786>
We should also test the case where the executor has unallocated resources, which is not allowed since the master ensures the allocation info is injected. At this point, maybe a different test name, like `ExecutorInfoAllocatedResources`.
src/tests/master_validation_tests.cpp (lines 2235 - 2236)
<https://reviews.apache.org/r/56939/#comment238838>
Hm.. we should probably just pull this test out into its own scope?
- Benjamin Mahler
On Feb. 24, 2017, 10:48 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 10:48 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With support for multi-role frameworks, we need to make sure that
> individual executors cannot mix allocation roles.
>
>
> Diffs
> -----
>
> src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
> src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
> src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
>
> Diff: https://reviews.apache.org/r/56939/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
Posted by Jay Guo <gu...@gmail.com>.
> On Feb. 24, 2017, 6:51 p.m., Jay Guo wrote:
> > src/master/validation.cpp, lines 590-609
> > <https://reviews.apache.org/r/56939/diff/2/?file=1646991#file1646991line590>
> >
> > Could we leverage `Resources::allocations()` to help with mix detection? e.g.
> > ```
> > If (resources.allocations().size() > 1) {
> > return Error(...);
> > }
> > ```
> >
> > In this case, `RESERVE` operation could not use this method since resources are not always allocated in case we are dealing with Operator API.
Also, should we deal with the case where some of the resources are _not_ allocated, and others are allocated to same role.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/#review166680
-----------------------------------------------------------
On Feb. 24, 2017, 6:48 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 6:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With support for multi-role frameworks, we need to make sure that
> individual executors cannot mix allocation roles.
>
>
> Diffs
> -----
>
> src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
> src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
> src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
>
> Diff: https://reviews.apache.org/r/56939/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/#review166680
-----------------------------------------------------------
src/master/validation.cpp (lines 590 - 609)
<https://reviews.apache.org/r/56939/#comment238704>
Could we leverage `Resources::allocations()` to help with mix detection? e.g.
```
If (resources.allocations().size() > 1) {
return Error(...);
}
```
In this case, `RESERVE` operation could not use this method since resources are not always allocated in case we are dealing with Operator API.
- Jay Guo
On Feb. 24, 2017, 6:48 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> -----------------------------------------------------------
>
> (Updated Feb. 24, 2017, 6:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
>
>
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With support for multi-role frameworks, we need to make sure that
> individual executors cannot mix allocation roles.
>
>
> Diffs
> -----
>
> src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
> src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
> src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
>
> Diff: https://reviews.apache.org/r/56939/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 56939: Added allocation role validation for
ExecutorInfo.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56939/
-----------------------------------------------------------
(Updated Feb. 24, 2017, 6:48 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and Michael Park.
Summary (updated)
-----------------
Added allocation role validation for ExecutorInfo.
Bugs: MESOS-6636
https://issues.apache.org/jira/browse/MESOS-6636
Repository: mesos
Description (updated)
-------
With support for multi-role frameworks, we need to make sure that
individual executors cannot mix allocation roles.
Diffs (updated)
-----
src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19
src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f
src/tests/master_validation_tests.cpp 45254739e2a62fed7008ceada7a8f64bb9ee4e31
Diff: https://reviews.apache.org/r/56939/diff/
Testing
-------
make check
Thanks,
Jay Guo