You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2015/12/08 06:31:13 UTC
Review Request 41075: Added support for implicit roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* I noticed a non-deterministic test failure in "MasterAllocatorTest/1.FrameworkExited" on Ubuntu 15.10. It repros reliably but I haven't determined yet whether it is a flakey test or a bug in this patch.
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109281
-----------------------------------------------------------
The test failure that I noticed on Ubuntu 15.10 seems to occur without these changes applied. I'll investigate but for now it seems unrelated -- opened https://issues.apache.org/jira/browse/MESOS-4095 to track it.
- Neil Conway
On Dec. 8, 2015, 5:31 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:31 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * I noticed a non-deterministic test failure in "MasterAllocatorTest/1.FrameworkExited" on Ubuntu 15.10. It repros reliably but I haven't determined yet whether it is a flakey test or a bug in this patch.
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 10, 2015, 10:32 p.m., Neil Conway wrote:
> > src/master/http.cpp, line 1588
> > <https://reviews.apache.org/r/41075/diff/5/?file=1159421#file1159421line1588>
> >
> > AlexR suggested including roles that have any reserved resources here. That makes sense, but AFAIK there isn't an easy way to find this information (unless we want to iterate over all the slaves and examine their resources). Thoughts?
Alternatively, we can bookkeep roles with reservations.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109878
-----------------------------------------------------------
On Dec. 10, 2015, 10:09 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 10:09 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 10, 2015, 10:32 p.m., Neil Conway wrote:
> > src/master/http.cpp, line 1588
> > <https://reviews.apache.org/r/41075/diff/5/?file=1159421#file1159421line1588>
> >
> > AlexR suggested including roles that have any reserved resources here. That makes sense, but AFAIK there isn't an easy way to find this information (unless we want to iterate over all the slaves and examine their resources). Thoughts?
>
> Alexander Rukletsov wrote:
> Alternatively, we can bookkeep roles with reservations.
>
> Greg Mann wrote:
> Yea, I would say if this information isn't already available, then it's probably outside the scope of this patch. Perhaps we should create a JIRA for bookkeeping of reservations & their roles?
Sure, makes sense to me.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109878
-----------------------------------------------------------
On Dec. 10, 2015, 10:09 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 10:09 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 10, 2015, 10:32 p.m., Neil Conway wrote:
> > src/master/http.cpp, line 1588
> > <https://reviews.apache.org/r/41075/diff/5/?file=1159421#file1159421line1588>
> >
> > AlexR suggested including roles that have any reserved resources here. That makes sense, but AFAIK there isn't an easy way to find this information (unless we want to iterate over all the slaves and examine their resources). Thoughts?
>
> Alexander Rukletsov wrote:
> Alternatively, we can bookkeep roles with reservations.
Yea, I would say if this information isn't already available, then it's probably outside the scope of this patch. Perhaps we should create a JIRA for bookkeeping of reservations & their roles?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109878
-----------------------------------------------------------
On Dec. 10, 2015, 10:09 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 10:09 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109878
-----------------------------------------------------------
src/master/http.cpp (line 1588)
<https://reviews.apache.org/r/41075/#comment169539>
AlexR suggested including roles that have any reserved resources here. That makes sense, but AFAIK there isn't an easy way to find this information (unless we want to iterate over all the slaves and examine their resources). Thoughts?
- Neil Conway
On Dec. 10, 2015, 10:09 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 10:09 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110374
-----------------------------------------------------------
src/master/http.cpp (line 316)
<https://reviews.apache.org/r/41075/#comment170181>
This function does not be called in the code diff, do we still need to change it? can we remove it directly and rename the new function modelRole() to mode(const string& name, double weight, Option<Role*> role_)?
- Yongqiao Wang
On Dec. 14, 2015, 11:16 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 11:16 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Adam B <ad...@mesosphere.io>.
> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1016-1031
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163905#file1163905line1016>
> >
> > If the allocator's roleSorter doesn't know about the role (i.e. no frameworks are registered with that role), do we still want to make an explicit `allocate()` call on a SetQuota request for that role? Will setting quota on an unregistered role have any impact on the fair shares of other frameworks?
>
> Neil Conway wrote:
> It is possible for `roleSorter` to not know about the role but for `quotaRoleSorter` to know about it -- i.e., if the role has a configured quota but no active frameworks. In general, I can imagine circumstances in which we would still want to call `allocate()` here (e.g., if we end up revoking/rescinding offers to non-quota frameworks to preserve resources needed by a quota'd role). Perhaps AlexR can comment?
>
> Alexander Rukletsov wrote:
> Once quota is set for a role, the next `allocate()` call will lay away resources. I think Adam means that we do not necessarily need to allocate straight away because there are no consumers (no active frameworks in the quota'ed role) of these resources. Hence though quota on an unregistered role (I assume it means a role without any frameworks) does impact other frameworks, we should not necessarily rush with `allocate()`.
>
> However, does it make sense to introduce a condition here? What is the advantage? One extra `allocate()` call should not be a big deal, since set quota operation won't happen that often. So even Adam has the point, I would leave the code as is for readability.
"Adam means that we do not necessarily need to allocate straight away because there are no consumers"
That is indeed what I meant.
"One extra allocate() call should not be a big deal... I would leave the code as is for readability."
SGTM. Dropping the issue.
> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/http.cpp, line 316
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line316>
> >
> > I'm in agreement with Yong. This form of `model(role, name, weight)` isn't used anywhere, and is confusing alongside `modelRole(roleName, weight, Option<role>)`. Remove it and rename the other one to fit the `model(...)` pattern.
>
> Neil Conway wrote:
> Whoops, this version of `model(role, name, weight)` was included accidentally (it is from an old version of the patch). Will remove.
>
> I'm not sure that changing `modelRole()` to use the `model(...)` naming pattern is the right thing to do: the pattern is that `model(X)` returns a JSON object modeled after `X`. In this case, the parameters will be `string name, double weight, Option<Role*>`, so (a) it doesn't quite fit (b) from the function name + arguments, it is a little unclear that `model(string name, double weight, Option<Role*>)` is supposed to do.
Although precedent exists, I would agree that it is also unclear what the following existing `model(...)` call is supposed to do:
JSON::Object model(
const TaskInfo& task,
const FrameworkID& frameworkId,
const TaskState& state,
const std::vector<TaskStatus>& statuses);
> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line1601>
> >
> > Why do you list unregistered roles with quota configured, if you don't model/display their quota?
> > If you think quota (and roles configured for quota) should show up in `/roles`, file a JIRA and we'll do it all right in one pass.
>
> Neil Conway wrote:
> This was per discussion with AlexR. The idea is that, previously, "/roles" showed all the "potentially interesting" roles: by definition, it will include any role that has a non-default weight or non-default quota. With implicit roles, we want to show the same set of roles that have configured properties.
>
> Whether we show a role with configured quota is separate from whether we show quota information about any of the roles. For the latter, not sure if we want that information in "/roles", quota-related endpoints, or both. In any case, seems fine to defer this part of it.
>
> Alexander Rukletsov wrote:
> That's correct. Let me repeat my argument one more time: once a quota becomes "whitelisted" or, I prefer, "visible", we should show it in "/roles" to make operator's life easier. If an operator sets a quota for a role, they should see this role in "/roles", otherwise it becomes very tedious to track all "whitelisted" roles and potential typos.
Fair enough. Dropping this issue.
Just make sure that ROLES_HELP accurately explains this. I'll open a separate issue for that.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110449
-----------------------------------------------------------
On Dec. 16, 2015, 1:08 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 1:08 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > Good progress. I'm more confident now in the removal of RoleInfo in favor of a weights map.
> > - I'm liking the idea of naming this a role "whitelist" rather than "validRoles".
> > - We need to decide if /roles should care about quota. I vote "not right now"
> > - Are you planning to "update documentation" and "add tests" in a separate patch?
* Whitelist sounds good! I'll make that change.
* Re: quota, see discussion below -- the current behavior is intentional, but we can certainly change it.
* There are tests here (https://reviews.apache.org/r/41225/). I haven't done docs yet, I'll look at doing that shortly.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, line 316
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line316>
> >
> > I'm in agreement with Yong. This form of `model(role, name, weight)` isn't used anywhere, and is confusing alongside `modelRole(roleName, weight, Option<role>)`. Remove it and rename the other one to fit the `model(...)` pattern.
Whoops, this version of `model(role, name, weight)` was included accidentally (it is from an old version of the patch). Will remove.
I'm not sure that changing `modelRole()` to use the `model(...)` naming pattern is the right thing to do: the pattern is that `model(X)` returns a JSON object modeled after `X`. In this case, the parameters will be `string name, double weight, Option<Role*>`, so (a) it doesn't quite fit (b) from the function name + arguments, it is a little unclear that `model(string name, double weight, Option<Role*>)` is supposed to do.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line1601>
> >
> > Why do you list unregistered roles with quota configured, if you don't model/display their quota?
> > If you think quota (and roles configured for quota) should show up in `/roles`, file a JIRA and we'll do it all right in one pass.
This was per discussion with AlexR. The idea is that, previously, "/roles" showed all the "potentially interesting" roles: by definition, it will include any role that has a non-default weight or non-default quota. With implicit roles, we want to show the same set of roles that have configured properties.
Whether we show a role with configured quota is separate from whether we show quota information about any of the roles. For the latter, not sure if we want that information in "/roles", quota-related endpoints, or both. In any case, seems fine to defer this part of it.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/master.cpp, lines 560-561
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163908#file1163908line560>
> >
> > Why do you need the '\n' in your LOG message? Seems like an unnecessarily short wrap.
Seems like we use long lines in log messages, so I'll just remove all the newlines.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/quota_handler.cpp, lines 181-192
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163909#file1163909line181>
> >
> > Unnecessary change. `rescindOffers()` is only called from `Master::QuotaHandler::set()`, which will return BadRequest `if (!master->roles.contains(role))`, so this is indeed validated earlier.
> > Besides, if `master->roles` didn't container `role`, wouldn't you want to exit out of the whole function rather than just skip that loop?
The check is necessary because `Master::QuotaHandler::set()` only checks that the role is valid; it does not check whether the role appears in `master->roles` (and indeed it should not check that, because `roles` contains only the roles with > 0 registered frameworks. I've been thinking I should rename `master->roles` to `master->activeRoles`, so I'll go ahead and do that).
Re: returning early from the function, as written the function will return almost immediately if there are no frameworks in the role. Should be no perf difference, so it is a question of style -- I slightly prefer the code as written, but happy to reconsider if you disagree.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110449
-----------------------------------------------------------
On Dec. 14, 2015, 11:16 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 11:16 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1016-1031
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163905#file1163905line1016>
> >
> > If the allocator's roleSorter doesn't know about the role (i.e. no frameworks are registered with that role), do we still want to make an explicit `allocate()` call on a SetQuota request for that role? Will setting quota on an unregistered role have any impact on the fair shares of other frameworks?
It is possible for `roleSorter` to not know about the role but for `quotaRoleSorter` to know about it -- i.e., if the role has a configured quota but no active frameworks. In general, I can imagine circumstances in which we would still want to call `allocate()` here (e.g., if we end up revoking/rescinding offers to non-quota frameworks to preserve resources needed by a quota'd role). Perhaps AlexR can comment?
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1130
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163905#file1163905line1130>
> >
> > If there are no registered frameworks in any role, do we want to early-exit from the allocate() call?
I think we'll exit quite quickly regardless, so I'd vote against adding an early-exit.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110449
-----------------------------------------------------------
On Dec. 16, 2015, 4:19 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 4:19 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, line 316
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line316>
> >
> > I'm in agreement with Yong. This form of `model(role, name, weight)` isn't used anywhere, and is confusing alongside `modelRole(roleName, weight, Option<role>)`. Remove it and rename the other one to fit the `model(...)` pattern.
>
> Neil Conway wrote:
> Whoops, this version of `model(role, name, weight)` was included accidentally (it is from an old version of the patch). Will remove.
>
> I'm not sure that changing `modelRole()` to use the `model(...)` naming pattern is the right thing to do: the pattern is that `model(X)` returns a JSON object modeled after `X`. In this case, the parameters will be `string name, double weight, Option<Role*>`, so (a) it doesn't quite fit (b) from the function name + arguments, it is a little unclear that `model(string name, double weight, Option<Role*>)` is supposed to do.
>
> Adam B wrote:
> Although precedent exists, I would agree that it is also unclear what the following existing `model(...)` call is supposed to do:
> JSON::Object model(
> const TaskInfo& task,
> const FrameworkID& frameworkId,
> const TaskState& state,
> const std::vector<TaskStatus>& statuses);
Sure, I renamed `modelRole()` -> `model()`.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110449
-----------------------------------------------------------
On Dec. 16, 2015, 10:43 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1016-1031
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163905#file1163905line1016>
> >
> > If the allocator's roleSorter doesn't know about the role (i.e. no frameworks are registered with that role), do we still want to make an explicit `allocate()` call on a SetQuota request for that role? Will setting quota on an unregistered role have any impact on the fair shares of other frameworks?
>
> Neil Conway wrote:
> It is possible for `roleSorter` to not know about the role but for `quotaRoleSorter` to know about it -- i.e., if the role has a configured quota but no active frameworks. In general, I can imagine circumstances in which we would still want to call `allocate()` here (e.g., if we end up revoking/rescinding offers to non-quota frameworks to preserve resources needed by a quota'd role). Perhaps AlexR can comment?
Once quota is set for a role, the next `allocate()` call will lay away resources. I think Adam means that we do not necessarily need to allocate straight away because there are no consumers (no active frameworks in the quota'ed role) of these resources. Hence though quota on an unregistered role (I assume it means a role without any frameworks) does impact other frameworks, we should not necessarily rush with `allocate()`.
However, does it make sense to introduce a condition here? What is the advantage? One extra `allocate()` call should not be a big deal, since set quota operation won't happen that often. So even Adam has the point, I would leave the code as is for readability.
> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line1601>
> >
> > Why do you list unregistered roles with quota configured, if you don't model/display their quota?
> > If you think quota (and roles configured for quota) should show up in `/roles`, file a JIRA and we'll do it all right in one pass.
>
> Neil Conway wrote:
> This was per discussion with AlexR. The idea is that, previously, "/roles" showed all the "potentially interesting" roles: by definition, it will include any role that has a non-default weight or non-default quota. With implicit roles, we want to show the same set of roles that have configured properties.
>
> Whether we show a role with configured quota is separate from whether we show quota information about any of the roles. For the latter, not sure if we want that information in "/roles", quota-related endpoints, or both. In any case, seems fine to defer this part of it.
That's correct. Let me repeat my argument one more time: once a quota becomes "whitelisted" or, I prefer, "visible", we should show it in "/roles" to make operator's life easier. If an operator sets a quota for a role, they should see this role in "/roles", otherwise it becomes very tedious to track all "whitelisted" roles and potential typos.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110449
-----------------------------------------------------------
On Dec. 16, 2015, 4:19 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 4:19 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110449
-----------------------------------------------------------
Good progress. I'm more confident now in the removal of RoleInfo in favor of a weights map.
- I'm liking the idea of naming this a role "whitelist" rather than "validRoles".
- We need to decide if /roles should care about quota. I vote "not right now"
- Are you planning to "update documentation" and "add tests" in a separate patch?
src/master/allocator/mesos/hierarchical.hpp (lines 367 - 368)
<https://reviews.apache.org/r/41075/#comment170368>
"Roles are removed from this map..." and more importantly is removed from the roleSorter and has its frameworkSorter deleted.
src/master/allocator/mesos/hierarchical.cpp (lines 997 - 1012)
<https://reviews.apache.org/r/41075/#comment170354>
If the allocator's roleSorter doesn't know about the role (i.e. no frameworks are registered with that role), do we still want to make an explicit `allocate()` call on a SetQuota request for that role? Will setting quota on an unregistered role have any impact on the fair shares of other frameworks?
src/master/allocator/mesos/hierarchical.cpp
<https://reviews.apache.org/r/41075/#comment170355>
If there are no registered frameworks in any role, do we want to early-exit from the allocate() call?
src/master/http.cpp (line 316)
<https://reviews.apache.org/r/41075/#comment170365>
I'm in agreement with Yong. This form of `model(role, name, weight)` isn't used anywhere, and is confusing alongside `modelRole(roleName, weight, Option<role>)`. Remove it and rename the other one to fit the `model(...)` pattern.
src/master/http.cpp (lines 1601 - 1602)
<https://reviews.apache.org/r/41075/#comment170359>
Why do you list unregistered roles with quota configured, if you don't model/display their quota?
If you think quota (and roles configured for quota) should show up in `/roles`, file a JIRA and we'll do it all right in one pass.
src/master/http.cpp (lines 1616 - 1619)
<https://reviews.apache.org/r/41075/#comment170363>
Would it be appropriate to make this an `Option<double> weight` and set the default display value in the `model()` function rather than in the request handler?
src/master/master.hpp (lines 1336 - 1338)
<https://reviews.apache.org/r/41075/#comment170336>
I'm hesitant on this `validRoles`/`validRole()` naming, especially as I review another patch about valid role names based on special characters being disallowed.
How about `explicitRoles`, or `roleWhitelist`? If there is no explicit role whitelist, then any role is allowed, but in the presence of a whitelist, any new role has to be on the list.
src/master/master.cpp (lines 553 - 554)
<https://reviews.apache.org/r/41075/#comment170338>
Why do you need the '\n' in your LOG message? Seems like an unnecessarily short wrap.
src/master/master.cpp (line 2658)
<https://reviews.apache.org/r/41075/#comment170342>
`Master::isWhitelistedRole()`?
src/master/quota_handler.cpp (lines 181 - 192)
<https://reviews.apache.org/r/41075/#comment170348>
Unnecessary change. `rescindOffers()` is only called from `Master::QuotaHandler::set()`, which will return BadRequest `if (!master->roles.contains(role))`, so this is indeed validated earlier.
Besides, if `master->roles` didn't container `role`, wouldn't you want to exit out of the whole function rather than just skip that loop?
- Adam B
On Dec. 14, 2015, 3:16 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 3:16 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 15, 2015, 3:10 a.m., Yongqiao Wang wrote:
> > src/master/http.cpp, line 1556
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line1556>
> >
> > Roles with a non-default quota are shown in /roles endpoint, but their quota infromation does not be shown, is it reasonalbe? Like weight, non-default weight is shown in /roles, why non-default quota does not be shown, I think we should keep consitence.
I'm going to close this, because AdamB raised a similar point below.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110381
-----------------------------------------------------------
On Dec. 14, 2015, 11:16 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 11:16 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110381
-----------------------------------------------------------
src/master/http.cpp (line 1556)
<https://reviews.apache.org/r/41075/#comment170190>
Roles with a non-default quota are shown in /roles endpoint, but their quota infromation does not be shown, is it reasonalbe? Like weight, non-default weight is shown in /roles, why non-default quota does not be shown, I think we should keep consitence.
- Yongqiao Wang
On Dec. 14, 2015, 11:16 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 11:16 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 18, 2015, 1:24 a.m., Adam B wrote:
> > src/master/master.cpp, lines 2677-2681
> > <https://reviews.apache.org/r/41075/diff/12/?file=1168191#file1168191line2677>
> >
> > Or how about `return roleWhitelist.isNone() || roleWhitelist.get().contains(name);`?
Personally, I'd rather leave this as-is: the `||` version is more concise, but I find it less readable.
> On Dec. 18, 2015, 1:24 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1620-1630
> > <https://reviews.apache.org/r/41075/diff/12/?file=1168189#file1168189line1620>
> >
> > I notice you're getting role then weight, but you call `model(name, weight, role)`. Might want to keep the order consistent.
Good catch!
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review111096
-----------------------------------------------------------
On Dec. 17, 2015, 2:58 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2015, 2:58 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review111096
-----------------------------------------------------------
Ship it!
Looks great. Worst thing I could find was an unnecessary '.' in a log message. I'll let you respond to the other two suggestions, and you can either fix/ignore them yourself or tell me what to change when I commit it.
src/master/http.cpp (lines 1598 - 1608)
<https://reviews.apache.org/r/41075/#comment171078>
I notice you're getting role then weight, but you call `model(name, weight, role)`. Might want to keep the order consistent.
src/master/master.cpp (line 555)
<https://reviews.apache.org/r/41075/#comment171080>
Technically, our style is to end log messages without punctuation. We make exceptions for a few `!`s, but I'd rather see all of them removed. Logs are no place for excitement
src/master/master.cpp (lines 2660 - 2664)
<https://reviews.apache.org/r/41075/#comment171081>
Or how about `return roleWhitelist.isNone() || roleWhitelist.get().contains(name);`?
- Adam B
On Dec. 16, 2015, 6:58 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 6:58 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110879
-----------------------------------------------------------
Ship it!
Ship It!
- Yongqiao Wang
On Dec. 17, 2015, 2:58 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2015, 2:58 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 18, 2015, 6:04 a.m., Yongqiao Wang wrote:
> > src/master/master.cpp, line 597
> > <https://reviews.apache.org/r/41075/diff/13/?file=1170527#file1170527line597>
> >
> > If the value of the specified weight is default value 1.0, do we still need to put it in weights hashmap and pass to allocator?
I don't think it is worth special-casing this.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review111121
-----------------------------------------------------------
On Dec. 18, 2015, 1:54 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 1:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review111121
-----------------------------------------------------------
src/master/master.cpp (line 588)
<https://reviews.apache.org/r/41075/#comment171108>
If the value of the specified weight is default value 1.0, do we still need to put it in weights hashmap and pass to allocator?
- Yongqiao Wang
On Dec. 18, 2015, 1:54 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 1:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 19, 2015, 5:23 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 2675
> > <https://reviews.apache.org/r/41075/diff/13/?file=1170527#file1170527line2675>
> >
> > Can we rename this as _isValidRole_ or some others which is more clear? The current _isWhitelistedRole_ can return true even if the roleWhitelist is empty which does not match the name much.
`validRole` was actually what I called this method initially :)
Adam suggested I change it, for reasons that I thought were reasonable (see discussion above). Note that the whitelist being disabled is _not_ the same as the whitelist being empty. I'll clarify the header comment in master.hpp, which might help a bit.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review111360
-----------------------------------------------------------
On Dec. 18, 2015, 1:54 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 1:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review111360
-----------------------------------------------------------
src/master/master.cpp (line 2658)
<https://reviews.apache.org/r/41075/#comment171521>
Can we rename this as _isValidRole_ or some others which is more clear? The current _isWhitelistedRole_ can return true even if the roleWhitelist is empty which does not match the name much.
- Guangya Liu
On ๅไบๆ 18, 2015, 1:54 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated ๅไบๆ 18, 2015, 1:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838
> src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 1:54 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Address review comments.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838
src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Add tests for allocation behavior for weights + implicit roles
* More tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 17, 2015, 2:58 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Tweak help text.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Add tests for allocation behavior for weights + implicit roles
* More tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 17, 2015, 2:54 a.m., Yongqiao Wang wrote:
> > src/master/http.cpp, line 1543
> > <https://reviews.apache.org/r/41075/diff/11/?file=1168068#file1168068line1543>
> >
> > I am not sure /roles will return the reserved resources of role, as far as I know, it will return the used resources of each registered framework.
Ah, right -- I called this "total allocated resources" (since it includes both offered and used).
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110867
-----------------------------------------------------------
On Dec. 17, 2015, 2:01 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2015, 2:01 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110867
-----------------------------------------------------------
src/master/http.cpp (line 1521)
<https://reviews.apache.org/r/41075/#comment170891>
I am not sure /roles will return the reserved resources of role, as far as I know, it will return the used resources of each registered framework.
- Yongqiao Wang
On Dec. 17, 2015, 2:01 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2015, 2:01 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 17, 2015, 2:01 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Address code review comments.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Add tests for allocation behavior for weights + implicit roles
* More tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110770
-----------------------------------------------------------
Ship it!
Looks great! Only a couple of nits. Update the ROLES_HELP, and I'll commit it!
src/master/allocator/mesos/hierarchical.cpp (line 1030)
<https://reviews.apache.org/r/41075/#comment170803>
Do we need to `CHECK(quotaRoleSorter.contains(role))`?
src/master/http.cpp (lines 1517 - 1518)
<https://reviews.apache.org/r/41075/#comment170777>
Please update this to explain that the endpoint shows the weight, total reserved resources, and registered frameworks for each role that is whitelisted, registered, or has a non-default weight or quota.
The current TLDR/description does not tell me what to expect.
src/master/http.cpp (line 1524)
<https://reviews.apache.org/r/41075/#comment170791>
Please wrap each parameter onto a separate line, since the entire function prototype doesn't fit on one line.
src/master/http.cpp (line 1539)
<https://reviews.apache.org/r/41075/#comment170792>
I often get confused about our official 'underscore' naming policy, but I think this should be `role = _role.get();`
"We prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing"
"Some trailing underscores are used to distinguish between similar variables in the same scope (think prime symbols), but this should be avoided as much as possible, including removing existing instances in the code base."
- Adam B
On Dec. 16, 2015, 2:43 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 2:43 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 16, 2015, 10:43 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Rename modelRole() -> model(), per discussion.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Add tests for allocation behavior for weights + implicit roles
* More tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 16, 2015, 9:08 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Rebase.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Add tests for allocation behavior for weights + implicit roles
* More tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 16, 2015, 4:19 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing (updated)
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Add tests for allocation behavior for weights + implicit roles
* More tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 16, 2015, 4:18 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Address code review comments: use term "role whitelist", rename field to "activeRoles" in the master.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 16, 2015, 2:41 a.m., Yongqiao Wang wrote:
> > Just a reminder, no any changes are found between revision 6 and 7, please check if something is wrong.
Weird -- I sent the update when connected via unreliable wifi, so maybe RB got confused somehow. I'll post a revised version shortly anyway.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110604
-----------------------------------------------------------
On Dec. 15, 2015, 8:56 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 8:56 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110604
-----------------------------------------------------------
Just a reminder, no any changes are found between revision 6 and 7, please check if something is wrong.
- Yongqiao Wang
On Dec. 15, 2015, 8:56 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 8:56 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 8:56 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Address review comments, rebase.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 15, 2015, 2:10 a.m., Yongqiao Wang wrote:
> > src/master/http.cpp, line 1578
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line1578>
> >
> > In Implicit Roles, Quota configuration and Weight configuration will not create a role in Mesos master, and a new role will be implicitly created when framework register,right? If yes, it confuses me, Quota and weight setting will not create role, but them can be shown with /roles endpoint?
Roles are never really "created"; as discussed, a role is shown in /roles if we think the admin would want to know about it (e.g., > 0 registered frameworks, configured weight or quota).
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110371
-----------------------------------------------------------
On Dec. 15, 2015, 8:56 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 8:56 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110371
-----------------------------------------------------------
src/master/http.cpp (line 1578)
<https://reviews.apache.org/r/41075/#comment170178>
In Implicit Roles, Quota configuration and Weight configuration will not create a role in Mesos master, and a new role will be implicitly created when framework register,right? If yes, it confuses me, Quota and weight setting will not create role, but them can be shown with /roles endpoint?
- Yongqiao Wang
On Dec. 14, 2015, 11:16 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 11:16 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
> src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
> src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 14, 2015, 11:16 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Rebase, code cleanup.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 10, 2015, 10:09 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
Changes
-------
Rebase, address code review comments.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 10, 2015, 10:13 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 367-368
> > <https://reviews.apache.org/r/41075/diff/4/?file=1156816#file1156816line367>
> >
> > I'm not sure how badly we need to clean up this map. What happens if we leave a role in the activeRoles list even after a framework unregisters? Will it make the roleSorter noticeably slower?
I haven't tested the performance implications either way, but keeping the map accurate seems like it is more readable/easier for the programmer to reason about. Can you elaborate on your concern with cleaning up the map?
> On Dec. 10, 2015, 10:13 a.m., Adam B wrote:
> > src/master/master.hpp, line 2052
> > <https://reviews.apache.org/r/41075/diff/4/?file=1156819#file1156819line2052>
> >
> > Seems strange that you wouldn't replace this with a `Role(string name, double weight)` constructor. You complain about Role not knowing its name/weight elsewhere.
I did this to avoid redundancy: we're already storing the name of the role (as the map key), and the role's weight is stored in a separate map.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109708
-----------------------------------------------------------
On Dec. 9, 2015, 5:54 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2015, 5:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yongqiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109708
-----------------------------------------------------------
Seems like the right overall approach, but I've realized that we need to be careful about breaking the Allocator module API. Put RoleInfo back and I'll do another pass.
src/master/allocator/mesos/allocator.hpp (line 58)
<https://reviews.apache.org/r/41075/#comment169348>
I don't think we need to break the allocator API for this.
Sorry for misleading you before, but RoleInfo (and everything in include/mesos/master/allocator.hpp) is a part of the allocator module interface, potentially in use by community allocator modules. So, we'd have to issue an API change proposal/notification if you're actually going to change the parameter type. We're running into the opposite issue now with the Isolator module, where we're breaking the API to introduce a protobuf IsolatorRecoveryInfo parameter just to hold work_dir. Once we have the protobuf, we can then add new optional parameters without breaking the API again.
src/master/allocator/mesos/hierarchical.hpp (lines 367 - 368)
<https://reviews.apache.org/r/41075/#comment169351>
I'm not sure how badly we need to clean up this map. What happens if we leave a role in the activeRoles list even after a framework unregisters? Will it make the roleSorter noticeably slower?
src/master/master.hpp (lines 1332 - 1333)
<https://reviews.apache.org/r/41075/#comment169324>
Would an `Option<hashset<string>>` make the `if(explicitRoles.isNone())` check more readable than `if(validRoles.empty())`?
src/master/master.hpp
<https://reviews.apache.org/r/41075/#comment169325>
Seems strange that you wouldn't replace this with a `Role(string name, double weight)` constructor. You complain about Role not knowing its name/weight elsewhere.
src/master/master.cpp (lines 552 - 556)
<https://reviews.apache.org/r/41075/#comment169336>
We don't this kind of `*`-boxing for deprecation warnings.
We also don't usually get excited enough to use exclamation marks.
src/master/master.cpp (line 5534)
<https://reviews.apache.org/r/41075/#comment169339>
How about a CHECK << "message" explaining what's failing?
src/master/master.cpp (lines 5534 - 5538)
<https://reviews.apache.org/r/41075/#comment169340>
For how many times you say `framework->info.role()`, you should probably assign it to a local variable.
src/master/master.cpp (lines 5827 - 5830)
<https://reviews.apache.org/r/41075/#comment169346>
I guess with Role.RoleInfo or Role.Weights, you wouldn't delete the Role if it had a non-default weight.
src/master/quota_handler.cpp (line 303)
<https://reviews.apache.org/r/41075/#comment169347>
s/permitted/on the role whitelist, if it exists/
"permitted" sounds too much like ACL permissions to me.
- Adam B
On Dec. 8, 2015, 9:54 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 9:54 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 9, 2015, 5:54 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
Changes
-------
Bug fixes for quota allocation.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161
src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7
src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 7:18 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
Changes
-------
Address review comments, rebase.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1543-1545
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155878#file1155878line1543>
> >
> > Why don't we consider roles without frameworks but with a non-default weight active? Or roles with quota but without frameworks?
> >
> > Or if you would like to reserve the term "active" for roles with frameworks, how about expanding the terminology to something like:
> > ```
> > Visible = {active} U {weighted} U {quota'ed} U {with-dyn-res}
> > ```
> >
> > Anyway, my point is that we should show all "visible" roles here.
>
> Neil Conway wrote:
> Yeah, this is a good point. Do we think that a user wants to see `{active} U {weighted} U {quota'ed} U {with-dyn-res}`?
>
> That does seem reasonable to me, although with the way the data structures are organized, it will be somewhat ugly to implement :-\
Thinking about this a bit more:
* `/roles` currently doesn't show quotas.
* It also doesn't show dynamically reserved resources.
* If you set ACLs that involve a role, you could argue that it should show up in `/roles` as well.
Given that most of these issues are not related to implicit roles as such, I'm inclined to say we should defer making these changes -- perhaps as part of the work on dynamic weights/ACLs, we can reorganize the data structures to make this easier to implement anyway. I created https://issues.apache.org/jira/browse/MESOS-4097 to track this.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109326
-----------------------------------------------------------
On Dec. 8, 2015, 7:18 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 7:18 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1543-1545
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155878#file1155878line1543>
> >
> > Why don't we consider roles without frameworks but with a non-default weight active? Or roles with quota but without frameworks?
> >
> > Or if you would like to reserve the term "active" for roles with frameworks, how about expanding the terminology to something like:
> > ```
> > Visible = {active} U {weighted} U {quota'ed} U {with-dyn-res}
> > ```
> >
> > Anyway, my point is that we should show all "visible" roles here.
>
> Neil Conway wrote:
> Yeah, this is a good point. Do we think that a user wants to see `{active} U {weighted} U {quota'ed} U {with-dyn-res}`?
>
> That does seem reasonable to me, although with the way the data structures are organized, it will be somewhat ugly to implement :-\
>
> Neil Conway wrote:
> Thinking about this a bit more:
>
> * `/roles` currently doesn't show quotas.
> * It also doesn't show dynamically reserved resources.
> * If you set ACLs that involve a role, you could argue that it should show up in `/roles` as well.
>
> Given that most of these issues are not related to implicit roles as such, I'm inclined to say we should defer making these changes -- perhaps as part of the work on dynamic weights/ACLs, we can reorganize the data structures to make this easier to implement anyway. I created https://issues.apache.org/jira/browse/MESOS-4097 to track this.
I'm fine to do it in a separate ticket, however, I would argue it's a rather important desing decision for implicit roles. Let me elaborate on that.
Currently, there is actually no need to check role with quotas and dynamic reservations, because an operator *knows* there is no way a quota (or a dynamic reservations) can be set for a role which had not been specified in `--roles`.
When we switch to implicit roles, it's much harder to an operator to track roles, that I call "visible". Hence if we do not improve the "/roles" endpoint, I would say we worsen the opertator experience. Does it make sense?
Reagrding to the implementation. I've though about that and see two alternatives. Maintain multiple containers (as you do) and then assemble everything in the ednpoint handler. Or track "visible" roles directly (similar to reference counting in smart pointers). Neither solution is nice and concise. Curious what'd you say.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109326
-----------------------------------------------------------
On Dec. 9, 2015, 5:54 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2015, 5:54 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 5d33138d60e684b23f07e1781de7991209d3e161
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 367
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155876#file1155876line367>
> >
> > s/Frameworks/Roles?
Thanks, fixed.
> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/master.hpp, lines 1332-1333
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155879#file1155879line1332>
> >
> > Do you want to add a `TODO` here to remove this hashtable once the deprecation cycle is over?
I put a TODO (along with a warning) in master.cpp when we parse `--roles`.
> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 5837-5838
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155880#file1155880line5837>
> >
> > Fits one line.
You mean
```
CHECK(roles.contains(role))
<< "Unknown role " << role << " of framework " << *framework;
```
I believe this would violate the "anti-jaggedness" suggestion in the style guide.
> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1543-1545
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155878#file1155878line1543>
> >
> > Why don't we consider roles without frameworks but with a non-default weight active? Or roles with quota but without frameworks?
> >
> > Or if you would like to reserve the term "active" for roles with frameworks, how about expanding the terminology to something like:
> > ```
> > Visible = {active} U {weighted} U {quota'ed} U {with-dyn-res}
> > ```
> >
> > Anyway, my point is that we should show all "visible" roles here.
Yeah, this is a good point. Do we think that a user wants to see `{active} U {weighted} U {quota'ed} U {with-dyn-res}`?
That does seem reasonable to me, although with the way the data structures are organized, it will be somewhat ugly to implement :-\
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109326
-----------------------------------------------------------
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109326
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (line 367)
<https://reviews.apache.org/r/41075/#comment168816>
s/Frameworks/Roles?
src/master/allocator/mesos/hierarchical.cpp (lines 213 - 215)
<https://reviews.apache.org/r/41075/#comment168817>
We would like to keep all `CHECK*` blocks in the beginning of the function for clarity. Now after you removed several checks, let's swap `const string& role = frameworkInfo.role();` and `CHECK(initialized);`
src/master/http.cpp (lines 1543 - 1545)
<https://reviews.apache.org/r/41075/#comment168810>
Why don't we consider roles without frameworks but with a non-default weight active? Or roles with quota but without frameworks?
Or if you would like to reserve the term "active" for roles with frameworks, how about expanding the terminology to something like:
```
Visible = {active} U {weighted} U {quota'ed} U {with-dyn-res}
```
Anyway, my point is that we should show all "visible" roles here.
src/master/master.hpp (lines 1332 - 1333)
<https://reviews.apache.org/r/41075/#comment168812>
Do you want to add a `TODO` here to remove this hashtable once the deprecation cycle is over?
src/master/master.cpp (lines 5529 - 5531)
<https://reviews.apache.org/r/41075/#comment168813>
Why don't we check `framework->info.role()` is valid?
src/master/master.cpp (lines 5817 - 5818)
<https://reviews.apache.org/r/41075/#comment168814>
Fits one line.
- Alexander Rukletsov
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 8, 2015, 5:26 p.m., Anand Mazumdar wrote:
> > Would it be a good idea to split this review into smaller bits for ease of reviewing ? It would make it easier to commit the individual sub system component changes.
> >
> > One possible split can be:
> >
> > 1. Removal of `RoleInfo` protobuf.
> > 2. Changes to Allocator + Tests. (`src/master/allocator/*`)
> > 3. Changes to Master (`src/master/master.cpp`)
> > 4. Changes to quota related code to handle Implicit Roles. (`src/master/quota_handler.cpp`)
> > 5. Changes to HTTP handler code `/roles` on master (`src/master/http.cpp`)
> >
> > What do you think ?
I'm happy to do this if that's what the reviewers would like. I didn't use a fine-grained split like this initially for a few reasons:
* My understanding is that we want each commit/RR to be atomic and self-contained -- i.e., each commit should compile and pass the unit tests. If that is true, it is hard to disentangle most of these changes into separate commits.
* Personally, I find it more difficult to review a chain of commits like suggested above than a single, coherent commit that makes all the changes necessary to implement a feature. But maybe that's just personal preference.
FWIW, if all you're doing is splitting a large commit into smaller commits where each small commit touches a single file, that doesn't seem useful to me -- you can just review the changes to each file one at a time in the large commit.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109347
-----------------------------------------------------------
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109347
-----------------------------------------------------------
Would it be a good idea to split this review into smaller bits for ease of reviewing ? It would make it easier to commit the individual sub system component changes.
One possible split can be:
1. Removal of `RoleInfo` protobuf.
2. Changes to Allocator + Tests. (`src/master/allocator/*`)
3. Changes to Master (`src/master/master.cpp`)
4. Changes to quota related code to handle Implicit Roles. (`src/master/quota_handler.cpp`)
5. Changes to HTTP handler code `/roles` on master (`src/master/http.cpp`)
What do you think ?
- Anand Mazumdar
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 8, 2015, noon, Yong Qiao Wang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 320
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155877#file1155877line320>
> >
> > In the current implementation, quota(quotaSorter) does not been removed after the corresponding role is removed, then some resources will still be reserved by this non-active role, this will result in the waste of resources. If you rely on cluster operator to remove this quota, I think it is does not make sence and have a bad user experience, because role is implicitly removed when the last framework gone without notify the clsuter admin.
Roles are never "removed" -- just because we no longer track the role in `activeRoles` does not mean that the cluster admin needs to be notified.
Re: wasting resources, my understanding of the quota design is that roles with a quota defined should still be reserved resources even if they don't have any frameworks currently registered. So this is the intended behavior.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109308
-----------------------------------------------------------
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yong Qiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109308
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.cpp (line 301)
<https://reviews.apache.org/r/41075/#comment168776>
In the current implementation, quota(quotaSorter) does not been removed after the corresponding role is removed, then some resources will still be reserved by this non-active role, this will result in the waste of resources. If you rely on cluster operator to remove this quota, I think it is does not make sence and have a bad user experience, because role is implicitly removed when the last framework gone without notify the clsuter admin.
- Yong Qiao Wang
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 407
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155876#file1155876line407>
> >
> > s/iff/if/
"iff" is what was intended (https://en.wikipedia.org/wiki/If_and_only_if#Origin_of_iff).
> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 367
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155876#file1155876line367>
> >
> > Why "Frameworks are removed from this map"? I think it should be "Role" removed from this map.
Thanks, fixed.
> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/master.hpp, line 1331
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155879#file1155879line1331>
> >
> > Do we need to put a TODO here to mention that `validRoles` needs to be removed once "explicit roles" feature is deprecated? I believe at that time, we will just rely on ACL.
In my opinion, a separate "TODO" is not needed -- the feature is marked as deprecated. When it is removed, all the code required to implement it can be removed.
> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/master.cpp, lines 5842-5843
> > <https://reviews.apache.org/r/41075/diff/2/?file=1155880#file1155880line5842>
> >
> > Suggest to erase the role first, and then delete it, like how we remove offer in `Master::removeOffer()`
Yeah, I considered doing it this way. Why do you think erasing the role first is better? (The `removeOffer` case is not quite analogous, because `delete`-ing the offer means `offer->id()` is no longer valid).
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109321
-----------------------------------------------------------
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109321
-----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (line 367)
<https://reviews.apache.org/r/41075/#comment168803>
Why "Frameworks are removed from this map"? I think it should be "Role" removed from this map.
src/master/allocator/mesos/hierarchical.hpp (line 407)
<https://reviews.apache.org/r/41075/#comment168804>
s/iff/if/
src/master/master.hpp (line 1331)
<https://reviews.apache.org/r/41075/#comment168790>
Do we need to put a TODO here to mention that `validRoles` needs to be removed once "explicit roles" feature is deprecated? I believe at that time, we will just rely on ACL.
src/master/master.cpp (lines 5822 - 5823)
<https://reviews.apache.org/r/41075/#comment168800>
Suggest to erase the role first, and then delete it, like how we remove offer in `Master::removeOffer()`
- Qian Zhang
On Dec. 8, 2015, 4:33 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 4:33 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 8:33 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
Diff: https://reviews.apache.org/r/41075/diff/
Testing (updated)
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Notes:
* There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 8:29 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
Changes
-------
Rebase, a few minor tweaks/cleanups.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
Diff: https://reviews.apache.org/r/41075/diff/
Testing
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Thanks,
Neil Conway
Re: Review Request 41075: Added support for implicit roles.
Posted by Yong Qiao Wang <yq...@cn.ibm.com>.
> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> >
>
> Yong Qiao Wang wrote:
> I have talked with our shepherd Adam B yesterday, and we all agree to improve /roles endpoint to update/remove/list active roles, and Implicit Roles will focus on removing the static role list(specified by --roles flag) and let framework can register with any role. In addition, RoleInfo is still needed, and will be persisted when the default values are changed by /roles endpoint. Currently I also have posted some patches for Dynamic Roles, and there are some serious comflicts between our patches. Can you have a talk with our shepherd? I think we should reach a consensus before coding.
>
> Adam B wrote:
> I'm still not sure about removeRole, but update/list make sense for dynamic weights. We'll have to discuss the pros/cons of RoleInfo vs. a separate weights hashmap, but let's try to communicate & collaborate on our approaches to avoid too many unnecessary merge conflicts.
In the current implementation, role will be implicitly removed after the last framework gone, but it related configurations, such as quota, weight will not be removed, this will cause data inconsistencies, so I think we should let cluster operator to remove the role and it's related configuration after the corresponding framework no longer recovery.
In addition, if we all agree to use /roles to update the weight in RoleInfo and persist RoleInfo, then I think it is better to still use RoleInfo rather than weights hashmap, because it can pass more information to allocator, then it can avoid the frequent changes of the interface of allocator. For example, if we add Grace Period for a role in Opetimisitic Offer ticket, and maybe allocator also need to know it, then we on longer need to change this interface again later.
- Yong Qiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > <https://reviews.apache.org/r/41075/diff/1/?file=1155806#file1155806line101>
> >
> > When allocator initialize, their should be no active frameworks in mesos, can we consider to remove this parmeter from initialize()? and can initialize role-related information in addFramework().
Hmmm -- I think it is better as written. Right now, weights are static and set at initialization-time, so I think it makes more sense for them to be parameters of `initialize`, not `addFramework`. (When we do dynamic weights we'll need to reconsider this, but even then, I don't think passing weights to `addFramework` will be the right API).
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
On Dec. 8, 2015, 5:41 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:41 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Adam B <ad...@mesosphere.io>.
> On Dec. 7, 2015, 11:02 p.m., Yong Qiao Wang wrote:
> >
>
> Yong Qiao Wang wrote:
> I have talked with our shepherd Adam B yesterday, and we all agree to improve /roles endpoint to update/remove/list active roles, and Implicit Roles will focus on removing the static role list(specified by --roles flag) and let framework can register with any role. In addition, RoleInfo is still needed, and will be persisted when the default values are changed by /roles endpoint. Currently I also have posted some patches for Dynamic Roles, and there are some serious comflicts between our patches. Can you have a talk with our shepherd? I think we should reach a consensus before coding.
I'm still not sure about removeRole, but update/list make sense for dynamic weights. We'll have to discuss the pros/cons of RoleInfo vs. a separate weights hashmap, but let's try to communicate & collaborate on our approaches to avoid too many unnecessary merge conflicts.
> On Dec. 7, 2015, 11:02 p.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > <https://reviews.apache.org/r/41075/diff/1/?file=1155806#file1155806line101>
> >
> > When allocator initialize, their should be no active frameworks in mesos, can we consider to remove this parmeter from initialize()? and can initialize role-related information in addFramework().
>
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static and set at initialization-time, so I think it makes more sense for them to be parameters of `initialize`, not `addFramework`. (When we do dynamic weights we'll need to reconsider this, but even then, I don't think passing weights to `addFramework` will be the right API).
>
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be removed, so weights parameter will only have one default value(*,1). so it is make sence to remove this parameter from initialize (),and when framework register, we can add its role and the corresponding weight into allocator, then they can become active.
Even if we eliminate --weights, we'll be persisting the weights map to the registry, and on failover to a new master or on a new master's startup, it will recover the weights from the registry. It seems reasonable to me that after recovering the weights, the master could start the allocator with the newly recovered weights, rather than have to add them after allocator initialization.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
On Dec. 7, 2015, 9:41 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2015, 9:41 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On Dec. 8, 2015, 3:02 p.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > <https://reviews.apache.org/r/41075/diff/1/?file=1155806#file1155806line101>
> >
> > When allocator initialize, their should be no active frameworks in mesos, can we consider to remove this parmeter from initialize()? and can initialize role-related information in addFramework().
>
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static and set at initialization-time, so I think it makes more sense for them to be parameters of `initialize`, not `addFramework`. (When we do dynamic weights we'll need to reconsider this, but even then, I don't think passing weights to `addFramework` will be the right API).
>
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be removed, so weights parameter will only have one default value(*,1). so it is make sence to remove this parameter from initialize (),and when framework register, we can add its role and the corresponding weight into allocator, then they can become active.
>
> Adam B wrote:
> Even if we eliminate --weights, we'll be persisting the weights map to the registry, and on failover to a new master or on a new master's startup, it will recover the weights from the registry. It seems reasonable to me that after recovering the weights, the master could start the allocator with the newly recovered weights, rather than have to add them after allocator initialization.
>
> Neil Conway wrote:
> Note also that a role's weight is required even _before_ `addFramework`, because the weight of a role with quota set is relevant even if the role has no registered frameworks. So I think specifying the weight in `addFramework` isn't right. We could consider replacing the `initialize` argument with a separate `updateWeight` allocator API call, but I'm inclined to defer that until the dynamic weight work is actually done.
Neil, can you please clarify a little more about why the weight of a role with quota set is relevant even if the role has no registered frameworks?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
On Dec. 8, 2015, 4:33 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 4:33 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yong Qiao Wang <yq...@cn.ibm.com>.
> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> >
I have talked with our shepherd Adam B yesterday, and we all agree to improve /roles endpoint to update/remove/list active roles, and Implicit Roles will focus on removing the static role list(specified by --roles flag) and let framework can register with any role. In addition, RoleInfo is still needed, and will be persisted when the default values are changed by /roles endpoint. Currently I also have posted some patches for Dynamic Roles, and there are some serious comflicts between our patches. Can you have a talk with our shepherd? I think we should reach a consensus before coding.
> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > <https://reviews.apache.org/r/41075/diff/1/?file=1155806#file1155806line101>
> >
> > When allocator initialize, their should be no active frameworks in mesos, can we consider to remove this parmeter from initialize()? and can initialize role-related information in addFramework().
>
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static and set at initialization-time, so I think it makes more sense for them to be parameters of `initialize`, not `addFramework`. (When we do dynamic weights we'll need to reconsider this, but even then, I don't think passing weights to `addFramework` will be the right API).
For long term solution in Implicit Roles, --roles and --weights should be removed, so weights parameter will only have one default value(*,1). so it is make sence to remove this parameter from initialize (),and when framework register, we can add its role and the corresponding weight into allocator, then they can become active.
- Yong Qiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
On Dec. 8, 2015, 5:41 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:41 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > <https://reviews.apache.org/r/41075/diff/1/?file=1155806#file1155806line101>
> >
> > When allocator initialize, their should be no active frameworks in mesos, can we consider to remove this parmeter from initialize()? and can initialize role-related information in addFramework().
>
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static and set at initialization-time, so I think it makes more sense for them to be parameters of `initialize`, not `addFramework`. (When we do dynamic weights we'll need to reconsider this, but even then, I don't think passing weights to `addFramework` will be the right API).
>
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be removed, so weights parameter will only have one default value(*,1). so it is make sence to remove this parameter from initialize (),and when framework register, we can add its role and the corresponding weight into allocator, then they can become active.
>
> Adam B wrote:
> Even if we eliminate --weights, we'll be persisting the weights map to the registry, and on failover to a new master or on a new master's startup, it will recover the weights from the registry. It seems reasonable to me that after recovering the weights, the master could start the allocator with the newly recovered weights, rather than have to add them after allocator initialization.
>
> Neil Conway wrote:
> Note also that a role's weight is required even _before_ `addFramework`, because the weight of a role with quota set is relevant even if the role has no registered frameworks. So I think specifying the weight in `addFramework` isn't right. We could consider replacing the `initialize` argument with a separate `updateWeight` allocator API call, but I'm inclined to defer that until the dynamic weight work is actually done.
>
> Qian Zhang wrote:
> Neil, can you please clarify a little more about why the weight of a role with quota set is relevant even if the role has no registered frameworks?
My 2ยข: weight is the property of a role, not a frameworks, hence I don't think we should put it into `addFramework()`. `updateWeight(const string& role, double weight)` will be the right thing to do *once* we introduce dynamic weights.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > <https://reviews.apache.org/r/41075/diff/1/?file=1155806#file1155806line101>
> >
> > When allocator initialize, their should be no active frameworks in mesos, can we consider to remove this parmeter from initialize()? and can initialize role-related information in addFramework().
>
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static and set at initialization-time, so I think it makes more sense for them to be parameters of `initialize`, not `addFramework`. (When we do dynamic weights we'll need to reconsider this, but even then, I don't think passing weights to `addFramework` will be the right API).
>
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be removed, so weights parameter will only have one default value(*,1). so it is make sence to remove this parameter from initialize (),and when framework register, we can add its role and the corresponding weight into allocator, then they can become active.
>
> Adam B wrote:
> Even if we eliminate --weights, we'll be persisting the weights map to the registry, and on failover to a new master or on a new master's startup, it will recover the weights from the registry. It seems reasonable to me that after recovering the weights, the master could start the allocator with the newly recovered weights, rather than have to add them after allocator initialization.
Note also that a role's weight is required even _before_ `addFramework`, because the weight of a role with quota set is relevant even if the role has no registered frameworks. So I think specifying the weight in `addFramework` isn't right. We could consider replacing the `initialize` argument with a separate `updateWeight` allocator API call, but I'm inclined to defer that until the dynamic weight work is actually done.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 8:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
> Notes:
>
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader issue).
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Yong Qiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
-----------------------------------------------------------
include/mesos/master/allocator.hpp (line 99)
<https://reviews.apache.org/r/41075/#comment168754>
When allocator initialize, their should be no active frameworks in mesos, can we consider to remove this parmeter from initialize()? and can initialize role-related information in addFramework().
- Yong Qiao Wang
On Dec. 8, 2015, 5:41 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:41 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
>
>
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
>
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
>
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
>
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
>
>
> Diffs
> -----
>
> include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
> include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
> src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
> src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
> src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
> src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
> src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
> src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
> src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
> src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
> src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
>
> Diff: https://reviews.apache.org/r/41075/diff/
>
>
> Testing
> -------
>
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
>
> TODOs:
>
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 41075: Added support for implicit roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 5:41 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and Yong Qiao Wang.
Changes
-------
Update "Testing Done" comment.
Bugs: MESOS-4085
https://issues.apache.org/jira/browse/MESOS-4085
Repository: mesos
Description
-------
Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.
Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.
Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).
The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.
Diffs
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83
src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2
src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c
src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp fb214a829a57529d3f5c49730ae9733f53e622ca
Diff: https://reviews.apache.org/r/41075/diff/
Testing (updated)
-------
"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely role-related tests.
TODOs:
* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?
Thanks,
Neil Conway