mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 57743: Updated the agent to generate executor secrets.
Date Thu, 23 Mar 2017 03:35:12 GMT

-----------------------------------------------------------
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
> 
>


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