----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57743/#review169731 -----------------------------------------------------------
I was thinking of decoupling `launchExecutor` that currently does both creation/launch into two methods now that the creation is synchronous and the launch is asynchronous. Let's call the new method `Framework::addExecutor` with the signature `Executor* addExecutor(const ExecutorInfo& executorInfo)` The workflow would then look something like: - When executor AuthN is disabled: `addExecutor()` -> `launchExecutor()` - When executor AuthN is enabled: `addExecutor()` -> `generateSecret()` -> `launchExecutor()` I have a rough sketch of the diff of the proposal here (it won't compile/has style issues and also has the new proposed methods in addition to the ones you have already in your review): https://gist.github.com/hatred/27c9e245e6a8a1d503c16732ecc226f2 src/slave/slave.hpp Lines 365 (patched) <https://reviews.apache.org/r/57743/#comment242276> Since the function is executed synchronously, can we take `Framework*` as argument? src/slave/slave.hpp Lines 375 (patched) <https://reviews.apache.org/r/57743/#comment242277> Move this argument after L372? src/slave/slave.cpp Lines 2524-2526 (patched) <https://reviews.apache.org/r/57743/#comment242289> You can kill this if you pass `Framework*` as argument. src/slave/slave.cpp Lines 2641 (patched) <https://reviews.apache.org/r/57743/#comment242346> Would it make sense to make the order as "fid", "eid", "cid" since that is the logical order? src/slave/slave.cpp Lines 2649-2654 (patched) <https://reviews.apache.org/r/57743/#comment242356> Nit: The captures would fit in one line. ```cpp .onAny(defer( self(), [this, frameworkId, executorInfo_, taskInfo, taskGroup, containerId]( const Future<Secret>& future) ``` src/slave/slave.cpp Lines 2655-2705 (patched) <https://reviews.apache.org/r/57743/#comment242357> hmm, there are two problems with this helper. - The severe one is that if the secret generation fails and we go ahead and remove the executor if the framework had no pending tasks. The assertion would fail https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L4943 since the state of the executor is still `REGISTERING`. - We also don't send a `ExitedExecutorMessage` to the master. This would mean that the master won't ever free up the executor resources since it would still think that the task is active. Can we instead delegate the cleanup to the `executorTerminated` function and just invoke it from here? src/slave/slave.cpp Lines 2749-2755 (patched) <https://reviews.apache.org/r/57743/#comment242499> Move this to `_launchExecutor()`. Starting the timeout clock upon an executor launch seems more intuitive. src/slave/slave.cpp Lines 2771-2773 (patched) <https://reviews.apache.org/r/57743/#comment242490> Rename this to say "Ignoring launching executor ..." and log the `frameworkId`, `executorId` and `containerId`. Once you do this, you won't need the `taskGroup` as an argument to this function? src/slave/slave.cpp Lines 2777-2778 (patched) <https://reviews.apache.org/r/57743/#comment242491> hmm, copy/paste error for the comment? Can you modify it to say that we don't launch the executor as the framework is terminating? src/slave/slave.cpp Lines 2780-2782 (patched) <https://reviews.apache.org/r/57743/#comment242492> Modify it as per my earlier comment for the log statement. src/slave/slave.cpp Lines 2784-2787 (patched) <https://reviews.apache.org/r/57743/#comment242493> Kill this. src/slave/slave.cpp Lines 2792 (patched) <https://reviews.apache.org/r/57743/#comment242496> hmm, it is possible that the executor was expicitly shutdown by the scheduler by the time this callback was invoked. We need to have a check here to ignore the launch if the state of the executor is `TERMINATING/TERMINATED`. Also, makes sense to have an explicit invariant check that the state of the executor is `REGISTERING` after this. src/slave/slave.cpp Lines 7592 (patched) <https://reviews.apache.org/r/57743/#comment242500> the `->` operator did not work? Ditto fo the line after this. - Anand Mazumdar On March 22, 2017, 2:42 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57743/ > ----------------------------------------------------------- > > (Updated March 22, 2017, 2:42 p.m.) > > > Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-6999 > https://issues.apache.org/jira/browse/MESOS-6999 > > > Repository: mesos > > > Description > ------- > > This patch updates the agent code to generate executor > authentication tokens when executor authentication is > enabled. For now, the generated `Secret` objects must > be of `VALUE` type, and they're passed directly into the > executor environment. > > > Diffs > ----- > > src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb > src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b > > > Diff: https://reviews.apache.org/r/57743/diff/6/ > > > Testing > ------- > > Testing details can be found at the end of this chain. > > > Thanks, > > Greg Mann > >
