mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 14292: Added authentication support to scheduler and master.
Date Tue, 01 Oct 2013 17:42:53 GMT


> On Sept. 30, 2013, 6:24 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 141
> > <https://reviews.apache.org/r/14292/diff/6/?file=359613#file359613line141>
> >
> >     const &?

This should take a copy not a reference!


> On Sept. 30, 2013, 6:24 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1567
> > <https://reviews.apache.org/r/14292/diff/6/?file=359614#file359614line1567>
> >
> >     The dispatch is authenticate would need to pass the pid here, since 'from' could
have changed!

I'm going to drop the retry in master altogether per your comments below.


> On Sept. 30, 2013, 6:24 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1607-1609
> > <https://reviews.apache.org/r/14292/diff/6/?file=359614#file359614line1607>
> >
> >     Should we be retrying in the Master? Seems like the retry logic should rest
solely in the client (if the client is down, it looks like this will retry forever).
> >     
> >     Be sure to update the "retry" comments if you think we should remove this bit.

Removed retry, thanks.


> On Sept. 30, 2013, 6:24 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, lines 251-254
> > <https://reviews.apache.org/r/14292/diff/6/?file=359617#file359617line251>
> >
> >     Can you add a comment describing what we discussed for when the future was already
ready at this point (say, if two newMasterDetected calls occurred in quick succession)?
> >     
> >     // Authentication is in progress, try to cancel it.
> >     authenticating.get().discard();
> >     
> >     // If authenticating was already ready, this means there is a pending dispatch
to _authenticate. This call will consider authentication successful even though we may not
be authenticated (say, if two masters were elected in quick succession). In this case, the
driver will proceed with registration but will receive a framework error and will exit as
a result. This is a sufficiently rare race condition so it's not worth complicating the code
here to handle it.
> >     
> >     return;

thanks. fixed.


> On Sept. 30, 2013, 6:24 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, lines 937-949
> > <https://reviews.apache.org/r/14292/diff/6/?file=359617#file359617line937>
> >
> >     Do we want to add a shared initialize(...) function? It looks like the only
difference between the two constructors is the credential option passed to the SchedulerProcess?

Tried it but its not worth it because initialize() cannot take 'Option' as an argument. 'Option'
is not visible from scheduler.hpp


> On Sept. 30, 2013, 6:24 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1574
> > <https://reviews.apache.org/r/14292/diff/6/?file=359614#file359614line1574>
> >
> >     s/Cancel it/Try to cancel it/
> >     
> >     Can you add a comment about what happens when the authenticating future is ready?
(E.g. If the Future was Ready at this point, the pending _authenticate could result in a successful
authentication for a previous Authenticatee and a new Authenticatee will be created on the
client side to retry?).

done


- Vinod


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


On Sept. 30, 2013, 2:46 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14292/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2013, 2:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-704
>     https://issues.apache.org/jira/browse/MESOS-704
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Added authentication support for scheduler driver and master.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 957576bbc1c73513a9591194d017f76fe562a616 
>   include/mesos/scheduler.hpp cf3ecdaaf40fd878a80fe0b6f7e61a0997329cbd 
>   src/Makefile.am ee336130ad93d8b524c841f75be36f00d4a2b147 
>   src/common/type_utils.hpp 674a8820c339c6446dfa7d444457477ab4512e79 
>   src/java/jni/construct.cpp b01bd7ae2eda2dc5e0dcd68848c65bd9f9ea81f0 
>   src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 6d2a03b6a88e71ac4e2e2d1ee8e15925e393ef3d

>   src/java/src/org/apache/mesos/MesosSchedulerDriver.java 7ef1fe7755286bf92b94d7ece4f72d54e5b57a84

>   src/master/flags.hpp d59e67d5b2799d6d7a37e9cfe7246ae7372091ac 
>   src/master/master.hpp bd5cb1ff05967518d7dc7f3a9dc0d32c22eb9643 
>   src/master/master.cpp a49b17ef43fca5b385a89731ca8776a26b61399a 
>   src/python/native/mesos_scheduler_driver_impl.cpp f25d41d38caf2701813dbec0d342a3b327e9dedf

>   src/sasl/authenticator.hpp 2f78cf0fdd97f0ddc3a6ebd162e6559497d708e4 
>   src/sched/sched.cpp c399f2481259683a8e178abb3478307042292f23 
>   src/tests/allocator_tests.cpp c57da6eb3c431b47468b6a6941c3de06af9209e5 
>   src/tests/allocator_zookeeper_tests.cpp 6e3214c15c14dc8ba82082738c172c8833cd0887 
>   src/tests/authentication_tests.cpp PRE-CREATION 
>   src/tests/exception_tests.cpp 3fc1ac32d553644080a88f04f22077691ae1820b 
>   src/tests/fault_tolerance_tests.cpp 10e52c401476eb8416361de49b8e4061bb7ac4f3 
>   src/tests/gc_tests.cpp e404de3bfacfcac5515995f1b45c3d39181e138f 
>   src/tests/isolator_tests.cpp cd3b3000060b379ef10e38a2a98a2eebe69d90fc 
>   src/tests/master_detector_tests.cpp 2d140ba1a364a7af4d643951d6016ac17dd10526 
>   src/tests/master_tests.cpp 52f09d4f1ddeabcc1a797a13fae9641b72425dd5 
>   src/tests/mesos.hpp 8fbd56c8dd438a08673b54630bfe25d60ad5ee0e 
>   src/tests/mesos.cpp 776cb0f13d10b4ae437fe9a3c97dc8b3481290af 
>   src/tests/resource_offers_tests.cpp 3888e461de5c8fa807cff2fd2bd7ca12c704823a 
>   src/tests/slave_recovery_tests.cpp 48b2e6380a9ae688291992f3bf25c3cc473bc808 
>   src/tests/status_update_manager_tests.cpp cf420e4764356402f05b27c3b8e8802c21a58f8e

> 
> Diff: https://reviews.apache.org/r/14292/diff/
> 
> 
> Testing
> -------
> 
> OSX:
> make check
> ./bin/mesos-tests.sh --gtest_filter="*FaultTolerance*" --gtest_repeat=1000 --gtest_break_on_failure
--gtest_shuffle
> 
> Linux:
> make check GTEST_FILTER=''
> 
> Couldn't run tests on linux because the boxes I've access to do not support SASL :/ It
all compiles fwiw.
> 
> All our tests do auth now. yay!
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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