Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 18, 2015, 6:04 a.m., Yongqiao Wang wrote:
> > src/master/master.cpp, line 597
> > 
> >
> > 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/#review21
---


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.

[Neil Conway]

> On Dec. 19, 2015, 5:23 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 2675
> > 
> >
> > 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.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review111360
---



src/master/master.cpp (line 2658)


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.

[Yongqiao Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review21
---



src/master/master.cpp (line 588)


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.

[Neil Conway]

> On Dec. 18, 2015, 1:24 a.m., Adam B wrote:
> > src/master/master.cpp, lines 2677-2681
> > 
> >
> > 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
> > 
> >
> > 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.

[Adam B]

---
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)


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)


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)


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.

[Alexander Rukletsov]

> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1016-1031
> > 
> >
> > 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
> > 
> >
> > 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 
> 

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

---
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.

[Neil Conway]

> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1016-1031
> > 
> >
> > 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
> > 
> >
> > 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.

[Adam B]

> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1016-1031
> > 
> >
> > 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
> > 
> >
> > 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)`. 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`, 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)` 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& statuses);


> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1601-1602
> > 
> >
> > 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 

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, line 316
> > 
> >
> > 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)`. 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`, 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)` 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& 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.

[Neil Conway]

> On Dec. 17, 2015, 2:54 a.m., Yongqiao Wang wrote:
> > src/master/http.cpp, line 1543
> > 
> >
> > 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.

[Neil Conway]

---
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.

[Neil Conway]

---
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.

[Yongqiao Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110867
---



src/master/http.cpp (line 1521)


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.

[Yongqiao Wang]

---
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.

[Neil Conway]

> On Dec. 15, 2015, 3:10 a.m., Yongqiao Wang wrote:
> > src/master/http.cpp, line 1556
> > 
> >
> > 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.

[Neil Conway]

---
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.

[Neil Conway]

> 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
> > 
> >
> > 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)`. 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`, 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)` is supposed to do.


> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1601-1602
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

---
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.

[Yongqiao Wang]

---
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.

[Neil Conway]

---
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.

[Neil Conway]

> 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.

[Neil Conway]

> On Dec. 15, 2015, 2:10 a.m., Yongqiao Wang wrote:
> > src/master/http.cpp, line 1578
> > 
> >
> > 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.

[Yongqiao Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110381
---



src/master/http.cpp (line 1556)


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.

[Yongqiao Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110374
---



src/master/http.cpp (line 316)


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_)?


- 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.

[Yongqiao Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review110371
---



src/master/http.cpp (line 1578)


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.

[Alexander Rukletsov]

> On Dec. 10, 2015, 10:32 p.m., Neil Conway wrote:
> > src/master/http.cpp, line 1588
> > 
> >
> > 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.

[Greg Mann]

> On Dec. 10, 2015, 10:32 p.m., Neil Conway wrote:
> > src/master/http.cpp, line 1588
> > 
> >
> > 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.

[Neil Conway]

> On Dec. 10, 2015, 10:32 p.m., Neil Conway wrote:
> > src/master/http.cpp, line 1588
> > 
> >
> > 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.

[Neil Conway]

---
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.

[Neil Conway]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109878
---



src/master/http.cpp (line 1588)


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.

[Neil Conway]

> On Dec. 10, 2015, 10:13 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.hpp, lines 367-368
> > 
> >
> > 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
> > 
> >
> > 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.

[Adam B]

---
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)


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)


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)


Would an `Option` make the `if(explicitRoles.isNone())` 
check more readable than `if(validRoles.empty())`?



src/master/master.hpp 


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)


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)


How about a CHECK << "message" explaining what's failing?



src/master/master.cpp (lines 5534 - 5538)


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)


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)


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 

Re: Review Request 41075: Added support for implicit roles.

[Alexander Rukletsov]

> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1543-1545
> > 
> >
> > 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 

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

---
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.

[Neil Conway]

> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > 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:
> 
> * 

Re: Review Request 41075: Added support for implicit roles.

[Adam B]

> 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
> > 
> >
> > 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 
>   

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

---
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.

[Yong Qiao Wang]

---
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)


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.

[Yong Qiao Wang]

> 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 

Re: Review Request 41075: Added support for implicit roles.

[Qian Zhang]

> On Dec. 8, 2015, 3:02 p.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > 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 

Re: Review Request 41075: Added support for implicit roles.

[Qian Zhang]

---
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)


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)


s/iff/if/



src/master/master.hpp (line 1331)


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)


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.

[Alexander Rukletsov]

> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > 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 

Re: Review Request 41075: Added support for implicit roles.

[Alexander Rukletsov]

---
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)


s/Frameworks/Roles?



src/master/allocator/mesos/hierarchical.cpp (lines 213 - 215)


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)


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)


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)


Why don't we check `framework->info.role()` is valid?



src/master/master.cpp (lines 5817 - 5818)


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.

[Neil Conway]

> On Dec. 8, 2015, noon, Yong Qiao Wang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 320
> > 
> >
> > 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.

[Anand Mazumdar]

---
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.

[Neil Conway]

> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 407
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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.

[Neil Conway]

> 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.

[Neil Conway]

> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 367
> > 
> >
> > s/Frameworks/Roles?

Thanks, fixed.


> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/master.hpp, lines 1332-1333
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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` 
> 

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

---
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.

[Neil Conway]

> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1543-1545
> > 
> >
> > 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.

[Neil Conway]

---
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.

[Yong Qiao Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41075/#review109287
---



include/mesos/master/allocator.hpp (line 99)


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.

[Neil Conway]

> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > 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.

[Yong Qiao Wang]

> 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
> > 
> >
> > 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
> 
>

Review Request 41075: Added support for implicit roles.

[Neil Conway]

---
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.

[Neil Conway]

---
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.

[Neil Conway]

---
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