> On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > Looking great Adam. Lets split this into 3 reviews (1) credential read > > refactor 2) slave shutdown message and 3) slave auth) as mentioned in the > > comments.
Thanks for reviewing! This reply responds to most of your comments. I still need to pull readCredentials and ShutdownMessage into separate reviews and then update this one as the main diff, depending on those two. > On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 3400 > > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line3400> > > > > shouldn't this also be done in Master::disconnect(Slave*) similar to > > how we do it in Master::deactivate(Framework*) ? Oops. Moved that into the disconnect(Slave) review, had it removed, and forgot to add it back here. Fixed. > On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 371 > > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line371> > > > > s/error/message/ > > > > What happens if the slaves are upgraded before the master? If the Slaves are upgraded before the master, then the master would only send ShutdownMessages without the 'message' field. What I hoped would happen: Since the 'message' field is optional, install should pass None() to shutdown as an Option, and no error would be printed. After investigating, I now realize that it will default to an empty string, and I'll need to handle that instead of isNone(). Correct? I still think it would be nice if ProtobufProcess::install() could convert an optional protobuf into an Option of that type. > On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 590 > > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line590> > > > > I know this is copy pasted from sched.cpp but can this be > > CHECK_NOTNULL(authenticatee)? We actually want to make sure that authenticatee IS null before we create a new Authenticatee. Looks like there's no CHECK_ISNULL(), so I'll leave this as is. > On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 640 > > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line640> > > > > s/master.get()/self()/ Slave::shutdown() exits without terminating if (from && master != from), so I either need to pass in master or nothing. I thought master was appropriate, since we actually got back a future==false (authentication failure) response from the master, as opposed to a slave-side error. Would you rather I pass nothing? Or alter Slave::shutdown() to also allow (from == self())? > On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 2678 > > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line2678> > > > > we should really consolidate 'deactivate' framework and 'disconnect' > > slave. They are essentially doing similar things to frameworks and slaves. > > maybe a TODO for now if you don't want to tackle that first. Added a TODO in master.hpp. Would need significant refactoring to share code across the Framework and Slave structs, deferring to a later review. > On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > src/sasl/authenticatee.hpp, lines 404-407 > > <https://reviews.apache.org/r/18381/diff/7/?file=540068#file540068line404> > > > > why the change? > On April 11, 2014, 7:54 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 2666 > > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line2666> > > > > Do we really need an AuthenticateMessage::Type? > > > > Why not just go through 'frameworks' and if pid not found go through > > 'slaves'? Similar to what we do in Master::exited(). Fair enough. I was just worried that every time we register a slave, we would unnecessarily iterate through the list of all frameworks, which could be rather large. But I suppose slaves don't register that frequently (except on master failover), so speed is less important there. At least this is better than the other way around, where registering new frameworks (which needs to be fast for interactive jobs) would unnecessarily iterate through thousands of slaves. - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18381/#review40197 ----------------------------------------------------------- On April 2, 2014, 9:44 p.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18381/ > ----------------------------------------------------------- > > (Updated April 2, 2014, 9:44 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-804 > https://issues.apache.org/jira/browse/MESOS-804 > > > Repository: mesos-git > > > Description > ------- > > Added authentication support for slaves. > Fixes MESOS-804. > > Open Questions: > - Should AuthenticateMessage be replaced with AuthenticateFrameworkMessage, > or specify an Authenticatee type as coded here? > - When multiple entries for the same principal exist in the credentials file, > only the last entry is used. Acceptable behavior, but shouldn't this be > documented? > > > Diffs > ----- > > include/mesos/mesos.proto 37f8a7f > src/master/flags.hpp 024f86d > src/master/master.hpp b6b9983 > src/master/master.cpp 5d0ddb0 > src/messages/messages.proto bba17a9 > src/sasl/authenticatee.hpp 42a4eba > src/sasl/common.hpp PRE-CREATION > src/sched/sched.cpp 3684cfe > src/slave/flags.hpp d5c54c0 > src/slave/slave.hpp 15e23ce > src/slave/slave.cpp 6d901dc > src/tests/authentication_tests.cpp 127c5e6 > src/tests/cluster.hpp 11684d9 > src/tests/mesos.cpp ae3aeee > src/tests/sasl_tests.cpp 945426d > src/tests/slave_recovery_tests.cpp 72b6d42 > > Diff: https://reviews.apache.org/r/18381/diff/ > > > Testing > ------- > > make check; manually tested flatfile slave authentication success/failure. > Added new slave authentication unit tests in authentication_tests.cpp. > > > Thanks, > > Adam B > >
