mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 69588: Removed outdated authorization logic for offer operations.
Date Sat, 02 Mar 2019 00:54:43 GMT


> On Jan. 15, 2019, 7:27 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3604-3605 (original), 3604-3605 (patched)
> > <https://reviews.apache.org/r/69588/diff/2/?file=2115120#file2115120line3604>
> >
> >     Isn't the empty case valid? Also, use `Resources::reservationRole`?
> 
> Benjamin Bannier wrote:
>     With that helper we can probably even get rid of or reduce above comment block and
assertions, but should `CHECK(Resources::isReserved(resource))` since `Resources::reservationRole`
is only defined for reserved resources. Still need to think about unreserved resources/empty
case.
>     
>     Here and below.

The reason I explicitly set the role to `*` is for backward compatibility.

Since now the resource is always in the post-reservation-refinement format, the empty case
indicates that the resource is unreserved, and the old behavior is to look into `resource.role()`
which would return the default value `*`.
But, let me use the helpers to make it more readable:
```
const string role = Resources::isReserved(resources) ? Resources::reservationRole(resource)
: "*";
```


> On Jan. 15, 2019, 7:27 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Line 3662 (original), 3664 (patched)
> > <https://reviews.apache.org/r/69588/diff/2/?file=2115120#file2115120line3670>
> >
> >     The `reservations.empty()` case is actually an invalid call right? There's nothing
to unreserve.
> >     
> >     Should we be CHECKing that here or is that validated after this point today?

Unfortunately, we validate the operation in `Master::_accept`, *after* getting authorizations:
https://github.com/apache/mesos/blob/2c658770e7e273672d64fe6c7deaed05828e9a15/src/master/master.cpp#L4925,
so we have to deal with this case. I'm just setting it to `*` for consistency and backward
compatibility.

Also see https://github.com/apache/mesos/blob/2c658770e7e273672d64fe6c7deaed05828e9a15/src/master/master.cpp#L3669.
Let me add the comments related to validation back.


> On Jan. 15, 2019, 7:27 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 3723-3724 (original), 3723-3724 (patched)
> > <https://reviews.apache.org/r/69588/diff/2/?file=2115120#file2115120line3734>
> >
> >     Is the empty case valid?
> >     
> >     Also, use `Resources::reservationRole`?

See above.


- Chun-Hung


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


On Dec. 19, 2018, 11:20 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69588/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2018, 11:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Michael Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since the master now upgrades resources to the post-reservation-refinement
> format before authorization (see MESOS-7735), the authorization logic in the
> master becomes outdated. This patch cleans it up.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp b4faf2b077a0288ba36195b7a21402932489d316 
> 
> 
> Diff: https://reviews.apache.org/r/69588/diff/2/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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