mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 41075: Added support for implicit roles.
Date Tue, 15 Dec 2015 09:56:46 GMT

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


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


src/master/allocator/mesos/hierarchical.hpp (lines 367 - 368)
<https://reviews.apache.org/r/41075/#comment170368>

    "Roles are removed from this map..." and more importantly is removed from the roleSorter
and has its frameworkSorter deleted.



src/master/allocator/mesos/hierarchical.cpp (lines 997 - 1012)
<https://reviews.apache.org/r/41075/#comment170354>

    If the allocator's roleSorter doesn't know about the role (i.e. no frameworks are registered
with that role), do we still want to make an explicit `allocate()` call on a SetQuota request
for that role? Will setting quota on an unregistered role have any impact on the fair shares
of other frameworks?



src/master/allocator/mesos/hierarchical.cpp 
<https://reviews.apache.org/r/41075/#comment170355>

    If there are no registered frameworks in any role, do we want to early-exit from the allocate()
call?



src/master/http.cpp (line 316)
<https://reviews.apache.org/r/41075/#comment170365>

    I'm in agreement with Yong. This form of `model(role, name, weight)` isn't used anywhere,
and is confusing alongside `modelRole(roleName, weight, Option<role>)`. Remove it and
rename the other one to fit the `model(...)` pattern.



src/master/http.cpp (lines 1601 - 1602)
<https://reviews.apache.org/r/41075/#comment170359>

    Why do you list unregistered roles with quota configured, if you don't model/display their
quota?
    If you think quota (and roles configured for quota) should show up in `/roles`, file a
JIRA and we'll do it all right in one pass.



src/master/http.cpp (lines 1616 - 1619)
<https://reviews.apache.org/r/41075/#comment170363>

    Would it be appropriate to make this an `Option<double> weight` and set the default
display value in the `model()` function rather than in the request handler?



src/master/master.hpp (lines 1336 - 1338)
<https://reviews.apache.org/r/41075/#comment170336>

    I'm hesitant on this `validRoles`/`validRole()` naming, especially as I review another
patch about valid role names based on special characters being disallowed.
    How about `explicitRoles`, or `roleWhitelist`? If there is no explicit role whitelist,
then any role is allowed, but in the presence of a whitelist, any new role has to be on the
list.



src/master/master.cpp (lines 553 - 554)
<https://reviews.apache.org/r/41075/#comment170338>

    Why do you need the '\n' in your LOG message? Seems like an unnecessarily short wrap.



src/master/master.cpp (line 2658)
<https://reviews.apache.org/r/41075/#comment170342>

    `Master::isWhitelistedRole()`?



src/master/quota_handler.cpp (lines 181 - 192)
<https://reviews.apache.org/r/41075/#comment170348>

    Unnecessary change. `rescindOffers()` is only called from `Master::QuotaHandler::set()`,
which will return BadRequest `if (!master->roles.contains(role))`, so this is indeed validated
earlier.
    Besides, if `master->roles` didn't container `role`, wouldn't you want to exit out
of the whole function rather than just skip that loop?


- Adam B


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

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

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

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


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