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 Tue, 01 Dec 2015 18:14:16 GMT


> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771>
> >
> >     Why is it that we need to perform validation within authorization? We perform
`authorizeTask` with a `TaskInfo` that has not been validated yet, and take care of it later
on. Does that pattern not work here?
> 
> Greg Mann wrote:
>     That pattern would work here, and the original patches I posted worked that way.
We ended up switching to this solution because it makes for a cleaner implementation and eliminates
some redundant code. However, it does come at the cost of making error messaging more difficult
(as you noted in another patch) and decreasing the separation of functionality that can make
debugging easier. I don't have a strong preference for either path at the moment. Perhaps
Jie could chime in with his thoughts?
> 
> Jie Yu wrote:
>     I looked at the code again. I guess the reason we validate the operation before authorization
is because we want to get all the principals from resources in UNRESERVE, and we want to make
sure resources in UNRESERVE operation is valid. As MPark pointed out, coupling authorization
with validation does cause some confusion when generating error messages. How about the following:
>     
>     1) we don't perform validation in authorization. In authorizeUnreserveResources,
when we iterate all resources, we can do the following as you previosly did (sorry about the
back and forth, my bad).
>     ```
>     foreach (const Resource& resource, unreserve.resources()) {
>       if (Resources::isDynamicallyReserved(resource)) {
>         request.mutable_reserver_principals()->add_values(
>             resource.reservation().principal());
>       }
>     }
>     ```
>     
>     2) in Http::reserve and Http::unreserve, we still perform validation first before
calling master->authroizeXXX.
>     
>     3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.
>     
>     Let me know if that's better or not?

No worries, Jie! I learned about `Future<>::repair()` in the process so I consider it
worthwhile :-)

I have one question about your plan: for `LAUNCH` operations, tasks are authorized before
they get validated, and I'm wondering if it's worth making this part of the interface consistent
across all types of operations, i.e. perhaps we should change the code for `LAUNCH` to validate
before authorization as well. If we decide to do this, I could do it here or in another review.


- Greg


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2015, 8:35 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