mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 57535: Applied RegisterAgent ACL to the master.
Date Tue, 28 Mar 2017 08:42:56 GMT


> On March 24, 2017, 2:58 p.m., Benjamin Mahler wrote:
> > Nice tests! Overall approach looks good to me. A few comments below.
> > 
> > Unrelated to your changes I noticed a few issues:
> > 
> > There are some inconsistencies between the framework and agent paths. For example,
we don't log when we receive an agent's (re-)registration message but we log the framework's
subscription, not sure why we did that. Also, since we don't track a framework's pending subscription,
if the authorization futures are re-ordered we could process subscre 2 before subscre 1, but
this is unrelated to your change here.
> > 
> > The "queueing up" logic (example [here](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L5344-L5345))
also allows re-ordering.

Thanks!

- The logging is easy to fix. We can add such logging for the agent registration as well.
- Pending framework registration re-ordering: It looks to me we are OK for now because we
only have one hop in the async flow (subscribe -> authorizer -> _subscribe) and do we
[this](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L2902-L2911)
in the 2nd step. If we later add frameworks to the registrar we would need more hops and need
to do something similar to what this patch does.
- Can authorization futures be re-ordered? At least with the local authorizer they are executed
by the same authorizer actor that satifies the futures. I was thinking more about other events
like ExitedEvent being enqueued between duplicate subscribes and registrations.
- The "queueing up" logic: I don't see how `registerSlave` retries can themsevles get re-ordered.
Perhaps two `Master::registerSlave` dispatches enqueued by the authenticator can have an UnregisterSlaveMessage
go between then due to the async step but `registering` and `reregistering` seem to be able
to handle it.


> On March 24, 2017, 2:58 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Line 1640 (original), 1665-1668 (patched)
> > <https://reviews.apache.org/r/57535/diff/5/?file=1671256#file1671256line1665>
> >
> >     It looks like the comment about not answering questions about these transitioning
agents was removed, can we restore it?

Given the recent changes in /r/52083/, Perhaps it's best to put the comment above `recovered`?

```
    // Slaves that have been recovered from the registrar after master
    // failover. Slaves are removed from this collection when they
    // either re-register with the master or are marked unreachable
    // because they do not re-register before `recoveredTimer` fires.
    // We must not answer questions related to these slaves (e.g.,
    // during task reconciliation) until we determine their fate.
    hashmap<SlaveID, SlaveInfo> recovered;
```


- Jiang Yan


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


On March 28, 2017, 1:40 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> -----------------------------------------------------------
> 
> (Updated March 28, 2017, 1:40 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin Mahler, Greg
Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
>     https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/tests/master_authorization_tests.cpp 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/6/
> 
> 
> Testing
> -------
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will add them
when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


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