> On Dec. 2, 2015, 4:02 p.m., Alexander Rukletsov wrote: > > src/master/master.cpp, lines 2769-2770 > > <https://reviews.apache.org/r/39987/diff/10/?file=1150202#file1150202line2769> > > > > Is my understanding is correct, we do not allow reserving resources > > neither for operators nor for frameworks without a principal. Why do we > > have a path for `ACL::Entity::Any` here then? If you plan to reserve it for > > future use cases or just be more general — it's fine, but let's leave a fat > > comment that this is not possible currently (or how it's possible if my > > understanding is not correct). > > > > A follow-up question: Is it something we have agreed and maybe even > > documented somewhere, that certain actions require principal? I'm thinking > > about authz for quota and whether we have to make `principal` required > > there. > > Greg Mann wrote: > Yea this is an artifact of doing validation after authorization. We're > doing it in that order here to maintain consistency with the order for LAUNCH > operations, which must validate after authorization because their validation > requires knowledge of the tasks that may have already been launched within a > particular operation. > > You're right that if `!principal.isSome()` here, we know that this > operation will fail. The validation routine will catch that and return the > proper error, but how best to handle it here? In order to ensure that > `validate()` is able to report the correct error, perhaps we should just > `return true;` when `!principal.isSome()`. This is less than ideal, but > perhaps would suffice along with a fat comment, as you suggested :-)
Alex, thank you for the comments! I think those are great questions. > whether we have to make principal required there. I like the idea of making it optional. That gives us more flexibility (see some discussion in MESOS-3064): allowing dynamic reserveration without authz. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39987/#review108655 ----------------------------------------------------------- On Dec. 2, 2015, 6 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39987/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2015, 6 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff. > > > Bugs: MESOS-3062 > https://issues.apache.org/jira/browse/MESOS-3062 > > > Repository: mesos > > > Description > ------- > > Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve. > Note: this review is continued from https://reviews.apache.org/r/37125/ > > > Diffs > ----- > > src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe > src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 > > Diff: https://reviews.apache.org/r/39987/diff/ > > > Testing > ------- > > This is the third in a chain of 5 patches. `make check` was used to test > after all patches were applied. > > > Thanks, > > Greg Mann > >
