You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/03/18 01:47:35 UTC
Review Request 57743: Updated the agent to generate executor secrets.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
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/1/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review169660
-----------------------------------------------------------
src/slave/slave.cpp
Lines 2711-2718 (patched)
<https://reviews.apache.org/r/57743/#comment242177>
Fail the tasks in these cases too. Might be worth declaring a common lambda helper within this callback that can be used to fail tasks/send updates.
- Greg Mann
On March 21, 2017, 10:28 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 10:28 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/4/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review170084
-----------------------------------------------------------
src/slave/slave.cpp
Lines 2549 (patched)
<https://reviews.apache.org/r/57743/#comment242872>
Remove Extra newline?
src/slave/slave.cpp
Lines 2552 (patched)
<https://reviews.apache.org/r/57743/#comment242873>
How about being consistent with other validation failure strings in our code:
"Failed to validate generated secret: ..."
src/slave/slave.cpp
Lines 2555-2557 (patched)
<https://reviews.apache.org/r/57743/#comment242874>
How about the following string:
"Expecting generated secret to be of VALUE type instead of " + stringify(secret.type()) + " type; only VALUE type secrets are supported at this time"
src/slave/slave.cpp
Lines 2607 (patched)
<https://reviews.apache.org/r/57743/#comment242875>
Can you add a comment for posterity on why this is needed?
src/slave/slave.cpp
Lines 2618 (patched)
<https://reviews.apache.org/r/57743/#comment242876>
s/CHECK/CHECK_EQ
src/slave/slave.cpp
Lines 6681-6683 (original), 6899-6901 (patched)
<https://reviews.apache.org/r/57743/#comment242877>
Why this change here? If needed can we do it in a separate review.
- Anand Mazumdar
On March 25, 2017, 4:32 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 25, 2017, 4:32 a.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 e06525bf6a6596d35ae038e576f12eb96ab65a35
> src/slave/slave.cpp d68d6b9a1bae79bed27babe020bf8a5d41d116bd
>
>
> Diff: https://reviews.apache.org/r/57743/diff/11/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review170097
-----------------------------------------------------------
Ship it!
Ship It!
- Anand Mazumdar
On March 25, 2017, 6:09 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 25, 2017, 6:09 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 e06525bf6a6596d35ae038e576f12eb96ab65a35
> src/slave/slave.cpp d68d6b9a1bae79bed27babe020bf8a5d41d116bd
>
>
> Diff: https://reviews.apache.org/r/57743/diff/12/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 25, 2017, 6:09 p.m.)
Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
Changes
-------
Addressed comments.
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 (updated)
-----
src/slave/slave.hpp e06525bf6a6596d35ae038e576f12eb96ab65a35
src/slave/slave.cpp d68d6b9a1bae79bed27babe020bf8a5d41d116bd
Diff: https://reviews.apache.org/r/57743/diff/12/
Changes: https://reviews.apache.org/r/57743/diff/11-12/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 25, 2017, 4:32 a.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 (updated)
-----
src/slave/slave.hpp e06525bf6a6596d35ae038e576f12eb96ab65a35
src/slave/slave.cpp d68d6b9a1bae79bed27babe020bf8a5d41d116bd
Diff: https://reviews.apache.org/r/57743/diff/11/
Changes: https://reviews.apache.org/r/57743/diff/10-11/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 25, 2017, 12:22 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp
> > Lines 2558 (patched)
> > <https://reviews.apache.org/r/57743/diff/10/?file=1674559#file1674559line2558>
> >
> > Any reasons why you did not use the `!=` operator directly? Also, can you log the type of secret that was produced by the generator?
I added another patch, previous to this one, with a streaming operator overload to facilitate this logging.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review170063
-----------------------------------------------------------
On March 25, 2017, 4:32 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 25, 2017, 4:32 a.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 e06525bf6a6596d35ae038e576f12eb96ab65a35
> src/slave/slave.cpp d68d6b9a1bae79bed27babe020bf8a5d41d116bd
>
>
> Diff: https://reviews.apache.org/r/57743/diff/11/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Changed 'createExecutor' to 'addExecutor'.
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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/10/
Changes: https://reviews.apache.org/r/57743/diff/9-10/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 23, 2017, 9:30 p.m.)
Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
Changes
-------
Rebase.
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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/9/
Changes: https://reviews.apache.org/r/57743/diff/8-9/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 23, 2017, 5:13 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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/8/
Changes: https://reviews.apache.org/r/57743/diff/7-8/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 23, 2017, 4:14 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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/7/
Changes: https://reviews.apache.org/r/57743/diff/6-7/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 23, 2017, 3:35 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp
> > Lines 2777-2778 (patched)
> > <https://reviews.apache.org/r/57743/diff/6/?file=1671458#file1671458line2779>
> >
> > 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?
Just removed this comment since the code is self-explanatory.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review169731
-----------------------------------------------------------
On March 23, 2017, 4:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 23, 2017, 4:14 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/7/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 23, 2017, 3:35 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp
> > Lines 2655-2705 (patched)
> > <https://reviews.apache.org/r/57743/diff/6/?file=1671458#file1671458line2657>
> >
> > 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?
>
> Greg Mann wrote:
> Implemented this using `executorTerminated`, but now not seeing status updates, need to investigate.
This was a problem with my test, fixed now.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review169731
-----------------------------------------------------------
On March 23, 2017, 9:30 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 23, 2017, 9:30 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/9/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 23, 2017, 3:35 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp
> > Lines 2655-2705 (patched)
> > <https://reviews.apache.org/r/57743/diff/6/?file=1671458#file1671458line2657>
> >
> > 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?
Implemented this using `executorTerminated`, but now not seeing status updates, need to investigate.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review169731
-----------------------------------------------------------
On March 23, 2017, 4:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 23, 2017, 4:14 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/7/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/6/
Changes: https://reviews.apache.org/r/57743/diff/5-6/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 22, 2017, 1:31 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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/5/
Changes: https://reviews.apache.org/r/57743/diff/4-5/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/#review169659
-----------------------------------------------------------
src/slave/slave.hpp
Line 34 (original), 34 (patched)
<https://reviews.apache.org/r/57743/#comment242176>
#include <mesos/authentication/secret_generator.hpp>
- Greg Mann
On March 21, 2017, 10:28 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57743/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 10:28 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/4/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 21, 2017, 10:28 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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/4/
Changes: https://reviews.apache.org/r/57743/diff/3-4/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 21, 2017, 2:49 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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/3/
Changes: https://reviews.apache.org/r/57743/diff/2-3/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 57743: Updated the agent to generate executor
secrets.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57743/
-----------------------------------------------------------
(Updated March 20, 2017, 6:15 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 (updated)
-----
src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb
src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b
Diff: https://reviews.apache.org/r/57743/diff/2/
Changes: https://reviews.apache.org/r/57743/diff/1-2/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann