mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Greg Mann" <g...@mesosphere.io>
Subject Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Date Wed, 02 Dec 2015 18:25:34 GMT


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

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


- Greg


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


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