mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Neil Conway" <neil.con...@gmail.com>
Subject Re: Review Request 41075: Added support for implicit roles.
Date Tue, 15 Dec 2015 20:50:10 GMT


> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > Good progress. I'm more confident now in the removal of RoleInfo in favor of a weights
map.
> > - I'm liking the idea of naming this a role "whitelist" rather than "validRoles".
> > - We need to decide if /roles should care about quota. I vote "not right now"
> > - Are you planning to "update documentation" and "add tests" in a separate patch?

* Whitelist sounds good! I'll make that change.
* Re: quota, see discussion below -- the current behavior is intentional, but we can certainly
change it.
* There are tests here (https://reviews.apache.org/r/41225/). I haven't done docs yet, I'll
look at doing that shortly.


> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, line 316
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line316>
> >
> >     I'm in agreement with Yong. This form of `model(role, name, weight)` isn't used
anywhere, and is confusing alongside `modelRole(roleName, weight, Option<role>)`. Remove
it and rename the other one to fit the `model(...)` pattern.

Whoops, this version of `model(role, name, weight)` was included accidentally (it is from
an old version of the patch). Will remove.

I'm not sure that changing `modelRole()` to use the `model(...)` naming pattern is the right
thing to do: the pattern is that `model(X)` returns a JSON object modeled after `X`. In this
case, the parameters will be `string name, double weight, Option<Role*>`, so (a) it
doesn't quite fit (b) from the function name + arguments, it is a little unclear that `model(string
name, double weight, Option<Role*>)` is supposed to do.


> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line1601>
> >
> >     Why do you list unregistered roles with quota configured, if you don't model/display
their quota?
> >     If you think quota (and roles configured for quota) should show up in `/roles`,
file a JIRA and we'll do it all right in one pass.

This was per discussion with AlexR. The idea is that, previously, "/roles" showed all the
"potentially interesting" roles: by definition, it will include any role that has a non-default
weight or non-default quota. With implicit roles, we want to show the same set of roles that
have configured properties.

Whether we show a role with configured quota is separate from whether we show quota information
about any of the roles. For the latter, not sure if we want that information in "/roles",
quota-related endpoints, or both. In any case, seems fine to defer this part of it.


> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/master.cpp, lines 560-561
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163908#file1163908line560>
> >
> >     Why do you need the '\n' in your LOG message? Seems like an unnecessarily short
wrap.

Seems like we use long lines in log messages, so I'll just remove all the newlines.


> On Dec. 15, 2015, 9:56 a.m., Adam B wrote:
> > src/master/quota_handler.cpp, lines 181-192
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163909#file1163909line181>
> >
> >     Unnecessary change. `rescindOffers()` is only called from `Master::QuotaHandler::set()`,
which will return BadRequest `if (!master->roles.contains(role))`, so this is indeed validated
earlier.
> >     Besides, if `master->roles` didn't container `role`, wouldn't you want to
exit out of the whole function rather than just skip that loop?

The check is necessary because `Master::QuotaHandler::set()` only checks that the role is
valid; it does not check whether the role appears in `master->roles` (and indeed it should
not check that, because `roles` contains only the roles with > 0 registered frameworks.
I've been thinking I should rename `master->roles` to `master->activeRoles`, so I'll
go ahead and do that).

Re: returning early from the function, as written the function will return almost immediately
if there are no frameworks in the role. Should be no perf difference, so it is a question
of style -- I slightly prefer the code as written, but happy to reconsider if you disagree.


- Neil


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


On Dec. 14, 2015, 11:16 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
> 
> (Updated Dec. 14, 2015, 11:16 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg Mann, and
Yongqiao Wang.
> 
> 
> Bugs: MESOS-4085
>     https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a future
> version of Mesos.
> 
> 
> Diffs
> -----
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd

>   src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416

>   src/master/http.cpp 37fbcb93074fe189133161afaa28046dc2ad0731 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b

>   src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> -------
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more likely
role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` would probably
be nicer. I'm inclined to leave this as-is for now though (making use of unique_ptr is a broader
issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message