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