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 Sat, 25 Mar 2017 00:22:33 GMT

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



Getting there! Mostly minor comments around style/logging + a possible race condition that
needs to be handled.


src/slave/slave.cpp
Lines 118 (patched)
<https://reviews.apache.org/r/57743/#comment242832>

    Nit: You only seem to use it once. Can you directly invoke it with `common::validation::validateSecret`
at the call-site?



src/slave/slave.cpp
Lines 2557 (patched)
<https://reviews.apache.org/r/57743/#comment242834>

    Nit: Use the `->` operator instead.



src/slave/slave.cpp
Lines 2557 (patched)
<https://reviews.apache.org/r/57743/#comment242854>

    Nit: Use the `->` operator instead of `get()`



src/slave/slave.cpp
Lines 2558 (patched)
<https://reviews.apache.org/r/57743/#comment242855>

    Any reasons why you did not use the `!=` operator directly? Also, can you log the type
of secret that was produced by the generator?



src/slave/slave.cpp
Lines 2599-2613 (patched)
<https://reviews.apache.org/r/57743/#comment242837>

    Can we combine these? 
    
    Let's have a temporary `executorState` to capture the `terminating`/`terminated` string.



src/slave/slave.cpp
Lines 2601 (patched)
<https://reviews.apache.org/r/57743/#comment242840>

    s/with/in
    s/ID//
    
    Ditto for the other occurences.
    
    Also, you can directly use the `Executor` output operator.
    
    ```cpp
    LOG(WARNING) << "Ignoring launching executor " << *executor
                 << " in container " << executor->containerId
                 << " because the executor is " << executorState;
    ```



src/slave/slave.cpp
Lines 2604 (patched)
<https://reviews.apache.org/r/57743/#comment242846>

    hmm, this is problematic: You need to call `executorTerminated(...)` for both these cases
too. 
    
    If the framework is explicitly shutdown by the scheduler before the secret could be generated,
we still need clean up the executor.



src/slave/slave.cpp
Lines 2614 (patched)
<https://reviews.apache.org/r/57743/#comment242856>

    Explicitly assert that the executor state is `REGISTERING` here?



src/slave/slave.cpp
Lines 2619-2622 (patched)
<https://reviews.apache.org/r/57743/#comment242843>

    Modify this as per earlier comment to use the output operator.



src/slave/slave.cpp
Lines 2622 (patched)
<https://reviews.apache.org/r/57743/#comment242844>

    Can you also log the `Future` failure reason?



src/slave/slave.cpp
Lines 6606-6608 (original)
<https://reviews.apache.org/r/57743/#comment242850>

    Can we keep this `TODO` for now? Looks like we can now revisit this given that `launchExecutor`
is async.


- Anand Mazumdar


On March 24, 2017, 9:20 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
> 
> (Updated March 24, 2017, 9:20 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/10/
> 
> 
> 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