Re: Review Request 57925: Added further tests for executor secret generation.

[Greg Mann]

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

(Updated March 25, 2017, 4:41 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds two further tests for executor secret
generation, `SlaveTest.RunTaskGroupReferenceTypeSecret`
and `SlaveTest.RunTaskGroupFailedSecretAfterShutdown`.


Diffs
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57925/diff/4/


Testing (updated)
---

`make check`

`bin/mesos-tests.sh --gtest_filter="SlaveTest.*Secret*" --gtest_repeat=-1 
--gtest_break_on_failure` was used to check the new tests.


Thanks,

Greg Mann

Re: Review Request 57925: Added further tests for executor secret generation.

[Greg Mann]

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

(Updated March 25, 2017, 4:38 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds two further tests for executor secret
generation, `SlaveTest.RunTaskGroupReferenceTypeSecret`
and `SlaveTest.RunTaskGroupFailedSecretAfterShutdown`.


Diffs (updated)
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57925/diff/4/

Changes: https://reviews.apache.org/r/57925/diff/3-4/


Testing
---

`make check`
`bin/mesos-tests.sh --gtest_filter="*SlaveTest.*Secret*"` was used to check the 
new tests.

NOTE: `SlaveTest.RunTaskGroupFailedSecretAfterShutdown` currently does not pass 
and needs to be debugged.


Thanks,

Greg Mann

Re: Review Request 57924: Added 'shutdownExecutor' methods to the mock agent.

[Greg Mann]

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

(Updated March 25, 2017, 4:36 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Made the agent's `shutdownExecutor` method `virtual`.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds both mocked and unmocked `shutdownExecutor` methods
to the mock agent to facilitate testing of failure scenarios related
to executor secret generation.


Diffs (updated)
-

  src/slave/slave.hpp e06525bf6a6596d35ae038e576f12eb96ab65a35 
  src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
  src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 


Diff: https://reviews.apache.org/r/57924/diff/2/

Changes: https://reviews.apache.org/r/57924/diff/1-2/


Testing
---

Testing details can be found at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 57883: Added new tests for executor secret generation.

[Greg Mann]

> On March 25, 2017, 1 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp
> > Lines 5138 (patched)
> > 
> >
> > hmm, thinking about this more. Can we set the expectations on this 
> > object first and then hand the `Owned` pointer to the `MockSlave` object? 
> > 
> > Currently, it looks possible that the test as well as the agent thread 
> > might be using the `secretGenerator` leading to a race?

Per our discussion, I'll leave this as-is for now, since it seems to be 
consistent with other test code; for example, the master authorization tests, 
which inject the authorizer as a raw pointer.


- Greg


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


On March 25, 2017, 4:34 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57883/
> ---
> 
> (Updated March 25, 2017, 4:34 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new tests,
> `SlaveTest.RunTaskGroupFailedSecretGeneration` and
> `SlaveTest.RunTaskGroupInvalidExecutorSecret`, to
> verify the agent's behavior when generation of the
> executor secret fails.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 
> 
> 
> Diff: https://reviews.apache.org/r/57883/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh 
> --gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*"
>  --gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new 
> tests are not flaky.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57883: Added new tests for executor secret generation.

[Greg Mann]

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

(Updated March 25, 2017, 4:34 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds new tests,
`SlaveTest.RunTaskGroupFailedSecretGeneration` and
`SlaveTest.RunTaskGroupInvalidExecutorSecret`, to
verify the agent's behavior when generation of the
executor secret fails.


Diffs (updated)
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57883/diff/5/

Changes: https://reviews.apache.org/r/57883/diff/4-5/


Testing
---

`make check`
`bin/mesos-tests.sh 
--gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*"
 --gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new 
tests are not flaky.


Thanks,

Greg Mann

Re: Review Request 57743: Updated the agent to generate executor secrets.

[Greg Mann]

> On March 25, 2017, 12:22 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp
> > Lines 2558 (patched)
> > 
> >
> > 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.

[Greg Mann]

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

Review Request 57929: Added a streaming operator overload for Secret::Type.

[Greg Mann]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds an `operator<<` overload for the `Secret::Type`
protobuf enum to facilitate logging.


Diffs
-

  include/mesos/type_utils.hpp 19f34a7d8bfa373ca6c51a2061942f709dffb846 
  include/mesos/v1/mesos.hpp 0fecc4a971cc28de28c5cec5226cce6231c7682f 
  src/common/type_utils.cpp 3ab2dc1a27d7e0f97e0f37df2608ccec06f5936b 
  src/v1/mesos.cpp b21e1e77a816386b03bd378a36992d70c4971fcc 


Diff: https://reviews.apache.org/r/57929/diff/1/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 57926: Windows: Fix return of bad types in stat.hpp.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 24, 2017, 4:48 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57926/
> ---
> 
> (Updated March 24, 2017, 4:48 p.m.)
> 
> 
> Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7307
> https://issues.apache.org/jira/browse/MESOS-7307
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit 5f159cdcb introduced `return Error(...)` logic to functions
> which return `bool`, not `Try`, which broke the Windows build.
> 
> Furthermore, in the instances of `isdir` and `isfile`, erroring when
> asked to not follow a symlink is not correct. The semantics of symlinks
> provide clear answers to `isdir` and `isfile` when the target is a link,
> and is not being followed (it is neither a regular file nor a directory).
> 
> We explicitly match the POSIX semantics for `isfile` where `S_IFREG`
> returns `false` for symbolic links.
> 
> For the functions `mode` and `dev`, which return types wrapped by `Try`,
> we should only error if asked not to follow symlinks, and the target is
> actually a symlink. If it is not a symlink to begin with, we should not
> prematurely error. If it is a symlink, we should error because there is
> no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
> symlink itself.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 8587341282ca2d596a2b6f23f84b84a00053c3d5 
> 
> 
> Diff: https://reviews.apache.org/r/57926/diff/2/
> 
> 
> Testing
> ---
> 
> Build on Windows and run stout-tests.exe
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57883: Added new tests for executor secret generation.

[Anand Mazumdar]

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




src/tests/slave_tests.cpp
Lines 5138 (patched)


hmm, thinking about this more. Can we set the expectations on this object 
first and then hand the `Owned` pointer to the `MockSlave` object? 

Currently, it looks possible that the test as well as the agent thread 
might be using the `secretGenerator` leading to a race?



src/tests/slave_tests.cpp
Lines 5222 (patched)


How about:

```cpp
  // The tasks will fail to launch because the executor secret generation 
fails.
```



src/tests/slave_tests.cpp
Lines 5223-5225 (patched)


You might have to move this earlier as per my earlier comments.



src/tests/slave_tests.cpp
Lines 5280 (patched)


New line after since this comment applies to both `ACKNOWLEDGE` code blocks.



src/tests/slave_tests.cpp
Lines 5336 (patched)


Ditto as the initial comment in the last test around using `Owned` and 
setting the expectation before passing ownership?



src/tests/slave_tests.cpp
Lines 5402-5418 (patched)


Can you move these after L5428 since these would happen only after those 
expectations are fullfilled?



src/tests/slave_tests.cpp
Lines 5421 (patched)


How about:

```cpp
  // The tasks will fail to launch because the executor secret is invalid.
```



src/tests/slave_tests.cpp
Lines 5486 (patched)


Newline after this since this comment applies to both `ACKNOWLEGE` code 
blocks.


- Anand Mazumdar


On March 24, 2017, 9:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57883/
> ---
> 
> (Updated March 24, 2017, 9:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new tests,
> `SlaveTest.RunTaskGroupFailedSecretGeneration` and
> `SlaveTest.RunTaskGroupInvalidExecutorSecret`, to
> verify the agent's behavior when generation of the
> executor secret fails.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 
> 
> 
> Diff: https://reviews.apache.org/r/57883/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh 
> --gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*"
>  --gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new 
> tests are not flaky.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57743: Updated the agent to generate executor secrets.

[Anand Mazumdar]

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


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)


Nit: Use the `->` operator instead.



src/slave/slave.cpp
Lines 2557 (patched)


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



src/slave/slave.cpp
Lines 2558 (patched)


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)


Can we combine these? 

Let's have a temporary `executorState` to capture the 
`terminating`/`terminated` string.



src/slave/slave.cpp
Lines 2601 (patched)


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)


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)


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



src/slave/slave.cpp
Lines 2619-2622 (patched)


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



src/slave/slave.cpp
Lines 2622 (patched)


Can you also log the `Future` failure reason?



src/slave/slave.cpp
Lines 6606-6608 (original)


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 57926: Windows: Fix return of bad types in stat.hpp.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57926]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 24, 2017, 11:48 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57926/
> ---
> 
> (Updated March 24, 2017, 11:48 p.m.)
> 
> 
> Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7307
> https://issues.apache.org/jira/browse/MESOS-7307
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit 5f159cdcb introduced `return Error(...)` logic to functions
> which return `bool`, not `Try`, which broke the Windows build.
> 
> Furthermore, in the instances of `isdir` and `isfile`, erroring when
> asked to not follow a symlink is not correct. The semantics of symlinks
> provide clear answers to `isdir` and `isfile` when the target is a link,
> and is not being followed (it is neither a regular file nor a directory).
> 
> We explicitly match the POSIX semantics for `isfile` where `S_IFREG`
> returns `false` for symbolic links.
> 
> For the functions `mode` and `dev`, which return types wrapped by `Try`,
> we should only error if asked not to follow symlinks, and the target is
> actually a symlink. If it is not a symlink to begin with, we should not
> prematurely error. If it is a symlink, we should error because there is
> no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
> symlink itself.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 8587341282ca2d596a2b6f23f84b84a00053c3d5 
> 
> 
> Diff: https://reviews.apache.org/r/57926/diff/2/
> 
> 
> Testing
> ---
> 
> Build on Windows and run stout-tests.exe
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57926: Windows: Fix return of bad types in stat.hpp.

[Li Li]

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


Ship it!




Ship It!

- Li Li


On March 24, 2017, 11:48 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57926/
> ---
> 
> (Updated March 24, 2017, 11:48 p.m.)
> 
> 
> Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7307
> https://issues.apache.org/jira/browse/MESOS-7307
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit 5f159cdcb introduced `return Error(...)` logic to functions
> which return `bool`, not `Try`, which broke the Windows build.
> 
> Furthermore, in the instances of `isdir` and `isfile`, erroring when
> asked to not follow a symlink is not correct. The semantics of symlinks
> provide clear answers to `isdir` and `isfile` when the target is a link,
> and is not being followed (it is neither a regular file nor a directory).
> 
> We explicitly match the POSIX semantics for `isfile` where `S_IFREG`
> returns `false` for symbolic links.
> 
> For the functions `mode` and `dev`, which return types wrapped by `Try`,
> we should only error if asked not to follow symlinks, and the target is
> actually a symlink. If it is not a symlink to begin with, we should not
> prematurely error. If it is a symlink, we should error because there is
> no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
> symlink itself.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 8587341282ca2d596a2b6f23f84b84a00053c3d5 
> 
> 
> Diff: https://reviews.apache.org/r/57926/diff/2/
> 
> 
> Testing
> ---
> 
> Build on Windows and run stout-tests.exe
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 52083: Changed reconciliation for unregistering, reregistering agents.

[Benjamin Mahler]

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




src/master/master.hpp
Lines 1726-1737 (original), 1727-1734 (patched)


When reviewing an unrelated patch I was confused by this function since it 
no longer represents "transitioning" agents.

We should either:

(1) Remove this function and directly use the particular transitions we 
care about within reconciliation

(2) Preserve the semantics of this function: to return when an agent is 
transitioning from one state to another. And avoid using it in the 
reconciliation code, in favor of directly using the transitions we care about.

In both cases, the comment at the top of this review should really be 
within the code, as I don't think people can easily figure this out.


- Benjamin Mahler


On Oct. 13, 2016, 2:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52083/
> ---
> 
> (Updated Oct. 13, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6206
> https://issues.apache.org/jira/browse/MESOS-6206
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, explicit reconciliation for an agent that was in the process
> of reregistering or unregistering returned no results. This degree of
> cleverness seems unwarranted: if the agent hasn't completed the
> reregistration or unregistration process, it seems simpler for the
> master to return the previous state of the agent. This is what the
> framework would observe if their reconcile request lost the race with
> the reregister/unregister operation, anyway.
> 
> Note that since reregistering agents are no longer considered to be "in
> transition", we need to slightly adjust the rules for how we update the
> `slaves.recovered` collection in the master: an agent remains in the
> "recovered" collection until it has been marked reachable in the
> registry (rather than removing it from "recovered" as soon as the
> reregistration process beings). This is more consistent with how we
> manage the other collections in the master anyway: an agent appears in
> the `recovered` list until the registry operation that reregisters it
> has been successfully applied.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp 7ef898781ce5c2349ffeaa3ce43e68dede19c852 
>   src/tests/master_tests.cpp 88cf1e612ad8186ef2cea161b3a52b0df9517305 
>   src/tests/reconciliation_tests.cpp 1412090299df388456f04ed58a1d384ce3ff550a 
> 
> 
> Diff: https://reviews.apache.org/r/52083/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on OSX, Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57926: Windows: Fix return of bad types in stat.hpp.

[Andrew Schwartzmeyer]

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

(Updated March 24, 2017, 11:48 p.m.)


Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and 
Michael Park.


Changes
---

Return false for `isfile` since the check is explicitly the "is regular file" 
semantics.


Bugs: MESOS-7307
https://issues.apache.org/jira/browse/MESOS-7307


Repository: mesos


Description (updated)
---

Commit 5f159cdcb introduced `return Error(...)` logic to functions
which return `bool`, not `Try`, which broke the Windows build.

Furthermore, in the instances of `isdir` and `isfile`, erroring when
asked to not follow a symlink is not correct. The semantics of symlinks
provide clear answers to `isdir` and `isfile` when the target is a link,
and is not being followed (it is neither a regular file nor a directory).

We explicitly match the POSIX semantics for `isfile` where `S_IFREG`
returns `false` for symbolic links.

For the functions `mode` and `dev`, which return types wrapped by `Try`,
we should only error if asked not to follow symlinks, and the target is
actually a symlink. If it is not a symlink to begin with, we should not
prematurely error. If it is a symlink, we should error because there is
no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
symlink itself.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/windows/stat.hpp 
8587341282ca2d596a2b6f23f84b84a00053c3d5 


Diff: https://reviews.apache.org/r/57926/diff/2/

Changes: https://reviews.apache.org/r/57926/diff/1-2/


Testing
---

Build on Windows and run stout-tests.exe


Thanks,

Andrew Schwartzmeyer

Re: Review Request 57926: Windows: Fix return of bad types in stat.hpp.

[Li Li]

> On March 24, 2017, 10:18 p.m., Li Li wrote:
> > 3rdparty/stout/include/stout/os/windows/stat.hpp
> > Lines 66 (patched)
> > 
> >
> > At this case, should true or false be returned?
> 
> Andrew Schwartzmeyer wrote:
> In POSIX systems, a symlink is traditionally a file semantically. On 
> Windows, it is implemented as a reparse point instead of a file, but this 
> MSDN documentation 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365680(v=vs.85).aspx
>  states:
> 
> > Symbolic links are designed to aid in migration and application 
> compatibility with UNIX operating systems. Microsoft has implemented its 
> symbolic links to function just like UNIX links.
> 
> So I think it makes sense to maintain the POSIX semantics and state that 
> a symlink is a file (i.e. return true).
> 
> That said, I double checked our POSIX implementation, and `isdir` nor 
> `isfile` respectively compare `S_IFDIR` and `S_IFREG`, so they would both 
> return false (just tested `S_IFREG` as I wasn't sure). I don't _think_ this 
> is correct semantically, unless we intend `isfile` to mean "is regular file" 
> (i.e. explicitly not a link).
> 
> I might have just talked myself into returning false here, but would 
> appreciate further input.

Let's be consistent with the current isdir and isfile POSIX implementations. To 
me, isfile is better to tell us if a link is a real file or not. Feel free to 
file an issue to track the ambiguity and make it clear. If we want to change 
the current meaning of isfile, which means "is regular file", both 
places(windows and linux) should be changed together later.


- Li


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


On March 24, 2017, 10:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57926/
> ---
> 
> (Updated March 24, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7307
> https://issues.apache.org/jira/browse/MESOS-7307
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit 5f159cdcb introduced `return Error(...)` logic to functions
> which return `bool`, not `Try`, which broke the Windows build.
> 
> Furthermore, in the instances of `isdir` and `isfile`, erroring when
> asked to not follow a symlink is not correct. The semantics of symlinks
> provide clear answers to `isdir` and `isfile` when the target is a link,
> and is not being followed (it is a file, but not a directory).
> 
> For the functions `mode` and `dev`, which return types wrapped by `Try`,
> we should only error if asked not to follow symlinks, and the target is
> actually a symlink. If it is not a symlink to begin with, we should not
> prematurely error. If it is a symlink, we should error because there is
> no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
> symlink itself.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 8587341282ca2d596a2b6f23f84b84a00053c3d5 
> 
> 
> Diff: https://reviews.apache.org/r/57926/diff/1/
> 
> 
> Testing
> ---
> 
> Build on Windows and run stout-tests.exe
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57926: Windows: Fix return of bad types in stat.hpp.

[Andrew Schwartzmeyer]

> On March 24, 2017, 10:18 p.m., Li Li wrote:
> > 3rdparty/stout/include/stout/os/windows/stat.hpp
> > Lines 66 (patched)
> > 
> >
> > At this case, should true or false be returned?

In POSIX systems, a symlink is traditionally a file semantically. On Windows, 
it is implemented as a reparse point instead of a file, but this MSDN 
documentation 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365680(v=vs.85).aspx 
states:

> Symbolic links are designed to aid in migration and application compatibility 
> with UNIX operating systems. Microsoft has implemented its symbolic links to 
> function just like UNIX links.

So I think it makes sense to maintain the POSIX semantics and state that a 
symlink is a file (i.e. return true).

That said, I double checked our POSIX implementation, and `isdir` nor `isfile` 
respectively compare `S_IFDIR` and `S_IFREG`, so they would both return false 
(just tested `S_IFREG` as I wasn't sure). I don't _think_ this is correct 
semantically, unless we intend `isfile` to mean "is regular file" (i.e. 
explicitly not a link).

I might have just talked myself into returning false here, but would appreciate 
further input.


- Andrew


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


On March 24, 2017, 10:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57926/
> ---
> 
> (Updated March 24, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7307
> https://issues.apache.org/jira/browse/MESOS-7307
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit 5f159cdcb introduced `return Error(...)` logic to functions
> which return `bool`, not `Try`, which broke the Windows build.
> 
> Furthermore, in the instances of `isdir` and `isfile`, erroring when
> asked to not follow a symlink is not correct. The semantics of symlinks
> provide clear answers to `isdir` and `isfile` when the target is a link,
> and is not being followed (it is a file, but not a directory).
> 
> For the functions `mode` and `dev`, which return types wrapped by `Try`,
> we should only error if asked not to follow symlinks, and the target is
> actually a symlink. If it is not a symlink to begin with, we should not
> prematurely error. If it is a symlink, we should error because there is
> no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
> symlink itself.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 8587341282ca2d596a2b6f23f84b84a00053c3d5 
> 
> 
> Diff: https://reviews.apache.org/r/57926/diff/1/
> 
> 
> Testing
> ---
> 
> Build on Windows and run stout-tests.exe
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57926: Windows: Fix return of bad types in stat.hpp.

[Li Li]

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




3rdparty/stout/include/stout/os/windows/stat.hpp
Lines 66 (patched)


At this case, should true or false be returned?


- Li Li


On March 24, 2017, 10:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57926/
> ---
> 
> (Updated March 24, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos, John Kordich, James Peach, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7307
> https://issues.apache.org/jira/browse/MESOS-7307
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit 5f159cdcb introduced `return Error(...)` logic to functions
> which return `bool`, not `Try`, which broke the Windows build.
> 
> Furthermore, in the instances of `isdir` and `isfile`, erroring when
> asked to not follow a symlink is not correct. The semantics of symlinks
> provide clear answers to `isdir` and `isfile` when the target is a link,
> and is not being followed (it is a file, but not a directory).
> 
> For the functions `mode` and `dev`, which return types wrapped by `Try`,
> we should only error if asked not to follow symlinks, and the target is
> actually a symlink. If it is not a symlink to begin with, we should not
> prematurely error. If it is a symlink, we should error because there is
> no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
> symlink itself.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 8587341282ca2d596a2b6f23f84b84a00053c3d5 
> 
> 
> Diff: https://reviews.apache.org/r/57926/diff/1/
> 
> 
> Testing
> ---
> 
> Build on Windows and run stout-tests.exe
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

[Benjamin Mahler]

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



Nice tests! Overall approach looks good to me. A few comments below.

Unrelated to your changes I noticed a few issues:

There are some inconsistencies between the framework and agent paths. For 
example, we don't log when we receive an agent's (re-)registration message but 
we log the framework's subscription, not sure why we did that. Also, since we 
don't track a framework's pending subscription, if the authorization futures 
are re-ordered we could process subscre 2 before subscre 1, but this is 
unrelated to your change here.

The "queueing up" logic (example 
[here](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L5344-L5345))
 also allows re-ordering.


src/master/master.hpp
Line 1640 (original), 1665-1668 (patched)


It looks like the comment about not answering questions about these 
transitioning agents was removed, can we restore it?



src/master/master.cpp
Lines 3642-3656 (patched)


Any reason there's no logging here? It would be nice to log consistently 
with the framework authorization path:


https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L2201-L2203



src/master/master.cpp
Lines 3654 (patched)


Maybe a little comment about why we don't have a request object here?


- Benjamin Mahler


On March 22, 2017, 12:48 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> ---
> 
> (Updated March 22, 2017, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
> Mahler, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f 
>   src/tests/master_authorization_tests.cpp 
> 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/5/
> 
> 
> Testing
> ---
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will 
> add them when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57925: Added further tests for executor secret generation.

[Greg Mann]

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

(Updated March 24, 2017, 9:54 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds two further tests for executor secret
generation, `SlaveTest.RunTaskGroupReferenceTypeSecret`
and `SlaveTest.RunTaskGroupFailedSecretAfterShutdown`.


Diffs (updated)
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57925/diff/3/

Changes: https://reviews.apache.org/r/57925/diff/2-3/


Testing
---

`make check`
`bin/mesos-tests.sh --gtest_filter="*SlaveTest.*Secret*"` was used to check the 
new tests.

NOTE: `SlaveTest.RunTaskGroupFailedSecretAfterShutdown` currently does not pass 
and needs to be debugged.


Thanks,

Greg Mann

Re: Review Request 57667: Added executor authentication to the docs.

[Greg Mann]

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

(Updated March 24, 2017, 9:53 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6998
https://issues.apache.org/jira/browse/MESOS-6998


Repository: mesos


Description
---

Added executor authentication to the docs.


Diffs (updated)
-

  docs/authentication.md 1574db981d5f8ddd7d1f6bef1c2b032823d17297 
  docs/executor-http-api.md d666d3c459540df9490d7f67b0e43323957d5025 


Diff: https://reviews.apache.org/r/57667/diff/7/

Changes: https://reviews.apache.org/r/57667/diff/6-7/


Testing
---


Thanks,

Greg Mann

Re: Review Request 57925: Added further tests for executor secret generation.

[Greg Mann]

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

(Updated March 24, 2017, 9:52 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds two further tests for executor secret
generation, `SlaveTest.RunTaskGroupReferenceTypeSecret`
and `SlaveTest.RunTaskGroupFailedSecretAfterShutdown`.


Diffs (updated)
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57925/diff/2/

Changes: https://reviews.apache.org/r/57925/diff/1-2/


Testing
---

`make check`
`bin/mesos-tests.sh --gtest_filter="*SlaveTest.*Secret*"` was used to check the 
new tests.

NOTE: `SlaveTest.RunTaskGroupFailedSecretAfterShutdown` currently does not pass 
and needs to be debugged.


Thanks,

Greg Mann

Re: Review Request 57883: Added new tests for executor secret generation.

[Greg Mann]

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

(Updated March 24, 2017, 9:51 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds new tests,
`SlaveTest.RunTaskGroupFailedSecretGeneration` and
`SlaveTest.RunTaskGroupInvalidExecutorSecret`, to
verify the agent's behavior when generation of the
executor secret fails.


Diffs (updated)
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57883/diff/4/

Changes: https://reviews.apache.org/r/57883/diff/3-4/


Testing
---

`make check`
`bin/mesos-tests.sh 
--gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*"
 --gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new 
tests are not flaky.


Thanks,

Greg Mann

Re: Review Request 57923: Changed the agent's SecretGenerator from Owned to pointer.

[Greg Mann]

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

(Updated March 24, 2017, 9:50 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Added nullptr initialization.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch updates the agent to hold its `SecretGenerator` as a
raw pointer instead of an `Owned` object. This is more consistent
with the other dependencies injected into the agent, and makes it
easier to test the agent with a mock secret generator.


Diffs (updated)
-

  src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb 
  src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b 


Diff: https://reviews.apache.org/r/57923/diff/2/

Changes: https://reviews.apache.org/r/57923/diff/1-2/


Testing
---

Testing details can be found at the end of this patch chain.


Thanks,

Greg Mann

Review Request 57926: Windows: Fix return of bad types in stat.hpp.

[Andrew Schwartzmeyer]

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

Review request for mesos, John Kordich, James Peach, Joseph Wu, and Michael 
Park.


Bugs: MESOS-7307
https://issues.apache.org/jira/browse/MESOS-7307


Repository: mesos


Description
---

Commit 5f159cdcb introduced `return Error(...)` logic to functions
which return `bool`, not `Try`, which broke the Windows build.

Furthermore, in the instances of `isdir` and `isfile`, erroring when
asked to not follow a symlink is not correct. The semantics of symlinks
provide clear answers to `isdir` and `isfile` when the target is a link,
and is not being followed (it is a file, but not a directory).

For the functions `mode` and `dev`, which return types wrapped by `Try`,
we should only error if asked not to follow symlinks, and the target is
actually a symlink. If it is not a symlink to begin with, we should not
prematurely error. If it is a symlink, we should error because there is
no equivalent of `lstat` on Windows to obtain `st_mode` or `st_dev` of a
symlink itself.


Diffs
-

  3rdparty/stout/include/stout/os/windows/stat.hpp 
8587341282ca2d596a2b6f23f84b84a00053c3d5 


Diff: https://reviews.apache.org/r/57926/diff/1/


Testing
---

Build on Windows and run stout-tests.exe


Thanks,

Andrew Schwartzmeyer

Re: Review Request 57883: Added new tests for executor secret generation.

[Greg Mann]

> On March 24, 2017, 6:24 p.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp
> > Lines 5277-5301 (patched)
> > 
> >
> > Kill this? You are not expecting more status updates for the same task.

These acks, and the similar ones below, are necessary to allow the agent to 
call `removeFramework`. I'll add a comment to make that clear.


- Greg


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


On March 24, 2017, 9:32 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57883/
> ---
> 
> (Updated March 24, 2017, 9:32 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds new tests,
> `SlaveTest.RunTaskGroupFailedSecretGeneration` and
> `SlaveTest.RunTaskGroupInvalidExecutorSecret`, to
> verify the agent's behavior when generation of the
> executor secret fails.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 
> 
> 
> Diff: https://reviews.apache.org/r/57883/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh 
> --gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*"
>  --gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new 
> tests are not flaky.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57883: Added new tests for executor secret generation.

[Greg Mann]

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

(Updated March 24, 2017, 9:32 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds new tests,
`SlaveTest.RunTaskGroupFailedSecretGeneration` and
`SlaveTest.RunTaskGroupInvalidExecutorSecret`, to
verify the agent's behavior when generation of the
executor secret fails.


Diffs (updated)
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57883/diff/3/

Changes: https://reviews.apache.org/r/57883/diff/2-3/


Testing
---

`make check`
`bin/mesos-tests.sh 
--gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*"
 --gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new 
tests are not flaky.


Thanks,

Greg Mann

Re: Review Request 57891: Added a new unmocked method to the mock agent.

[Greg Mann]

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

(Updated March 24, 2017, 9:30 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds a new method to the mock agent,
`MockSlave::unmocked_executorTerminated`. This will
facilitate the testing of executor secret generation,
as it allows us to forward the mock function call to
the base class method.


Diffs (updated)
-

  src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
  src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 


Diff: https://reviews.apache.org/r/57891/diff/2/

Changes: https://reviews.apache.org/r/57891/diff/1-2/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Review Request 57925: Added further tests for executor secret generation.

[Greg Mann]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds two further tests for executor secret
generation, `SlaveTest.RunTaskGroupReferenceTypeSecret`
and `SlaveTest.RunTaskGroupFailedSecretAfterShutdown`.


Diffs
-

  src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a 


Diff: https://reviews.apache.org/r/57925/diff/1/


Testing
---

`make check`
`bin/mesos-tests.sh --gtest_filter="*SlaveTest.*Secret*"` was used to check the 
new tests.

NOTE: `SlaveTest.RunTaskGroupFailedSecretAfterShutdown` currently does not pass 
and needs to be debugged.


Thanks,

Greg Mann

Review Request 57924: Added 'shutdownExecutor' methods to the mock agent.

[Greg Mann]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds both mocked and unmocked `shutdownExecutor` methods
to the mock agent to facilitate testing of failure scenarios related
to executor secret generation.


Diffs
-

  src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
  src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 


Diff: https://reviews.apache.org/r/57924/diff/1/


Testing
---

Testing details can be found at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 57882: Added SecretGenerator injection to the MockSlave.

[Greg Mann]

> On March 24, 2017, 4:23 p.m., Anand Mazumdar wrote:
> > src/tests/mock_slave.hpp
> > Lines 206 (patched)
> > 
> >
> > How about keeping this consistent now that we are using `Owned` on the 
> > agent? Use `Option>` instead.
> > 
> > Modify the above comment accordingly.

Dropping these, since we switch to a raw pointer instead.


- Greg


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


On March 24, 2017, 9:24 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57882/
> ---
> 
> (Updated March 24, 2017, 9:24 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an overload of `Slave::initialize` to the
> mock slave for the purpose of injecting a secret generator.
> 
> 
> Diffs
> -
> 
>   src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
>   src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 
> 
> 
> Diff: https://reviews.apache.org/r/57882/diff/3/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57882: Added SecretGenerator injection to the MockSlave.

[Greg Mann]

> On March 24, 2017, 4:23 p.m., Anand Mazumdar wrote:
> > src/tests/mock_slave.hpp
> > Lines 118 (patched)
> > 
> >
> > `
> > virtual void initialize() override;
> > ` ?

The compiler complains about inconsistent use of override if we add this, so 
I'm leaving it out for now. We could do a batch update of all the mock agent 
methods later if we want. There is already a ticket for doing a sweep of the 
entire codebase to add `override`: MESOS-4871


- Greg


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


On March 24, 2017, 9:24 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57882/
> ---
> 
> (Updated March 24, 2017, 9:24 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an overload of `Slave::initialize` to the
> mock slave for the purpose of injecting a secret generator.
> 
> 
> Diffs
> -
> 
>   src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
>   src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 
> 
> 
> Diff: https://reviews.apache.org/r/57882/diff/3/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57882: Added SecretGenerator injection to the MockSlave.

[Greg Mann]

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

(Updated March 24, 2017, 9:24 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Changed the `mockSecretGenerator` from `Owned` to raw pointer.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds an overload of `Slave::initialize` to the
mock slave for the purpose of injecting a secret generator.


Diffs (updated)
-

  src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
  src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 


Diff: https://reviews.apache.org/r/57882/diff/3/

Changes: https://reviews.apache.org/r/57882/diff/2-3/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 57880: Added a 'MockSecretGenerator'.

[Greg Mann]

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

(Updated March 24, 2017, 9:23 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds a mock secret generator, which will
be used for testing failure scenarios involving
executor secret generation.


Diffs (updated)
-

  src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 


Diff: https://reviews.apache.org/r/57880/diff/4/

Changes: https://reviews.apache.org/r/57880/diff/3-4/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Review Request 57923: Changed the agent's SecretGenerator from Owned to pointer.

[Greg Mann]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch updates the agent to hold its `SecretGenerator` as a
raw pointer instead of an `Owned` object. This is more consistent
with the other dependencies injected into the agent, and makes it
easier to test the agent with a mock secret generator.


Diffs
-

  src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb 
  src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b 


Diff: https://reviews.apache.org/r/57923/diff/1/


Testing
---

Testing details can be found at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 57743: Updated the agent to generate executor secrets.

[Greg Mann]

---
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 57912: Enabled pause/resume for checks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57644, 57648, 56288, 55901, 57645, 57646, 57647, 57854, 57912]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 24, 2017, 3:44 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57912/
> ---
> 
> (Updated March 24, 2017, 3:44 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7277
> https://issues.apache.org/jira/browse/MESOS-7277
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled pause/resume for checks.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp e8af3160c9fd52ec20acf41b86bade50f4539fb1 
>   src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
>   src/launcher/default_executor.cpp ee24531b7172dbda07999e5a1101698c395a8272 
>   src/launcher/executor.cpp a4bac9d932b6574eb2e1b4a27aa44eaaca6aaa62 
> 
> 
> Diff: https://reviews.apache.org/r/57912/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on Fedora 24
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57839: WIP: Added an example framework demonstrating multirole capabilities.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [57839, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On March 24, 2017, 3:39 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57839/
> ---
> 
> (Updated March 24, 2017, 3:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit introduces an examples framework which can register in
> multiple roles.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
>   src/examples/multirole_framework.cpp PRE-CREATION 
>   src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
>   src/tests/multirole_framework_test.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57839/diff/1/
> 
> 
> Testing
> ---
> 
> make check (OS X, Linux)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 57883: Added new tests for executor secret generation.

[Anand Mazumdar]

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



There might be some comments I would have missed that apply to the second test 
too. When modifying the second test, please ensure that you fix the second test 
too.


src/tests/slave_tests.cpp
Lines 5116 (patched)


How about:

```
This test verifies that TASK_FAILED updates are sent correctly for all the 
tasks in a task group when secret generation fails.
```



src/tests/slave_tests.cpp
Lines 5125 (patched)


Move it to after L5138 closer to where it's used. Also, might need to 
change to `Owned` as per my last review comment.



src/tests/slave_tests.cpp
Lines 5127-5128 (patched)


Can you use the v1 classes/helpers that we recently added? I am already 
working on doing a sweep for moving the others tests in this file to use them 
similar to what we did for `default_executor_tests.cpp`. Hopefully, we can land 
them together. This would reduce the `evolve` calls needed in this test and 
make it more readable.

e.g., `v1::Resources`, `v1::ExecutorInfo` `v1::createTask` etc.



src/tests/slave_tests.cpp
Lines 5167 (patched)


// Ignore subsequent offers.



src/tests/slave_tests.cpp
Lines 5189 (patched)


s/EXPECT_NE/ASSERT_NE

It would crash on the next line otherwise when you access offers[0].



src/tests/slave_tests.cpp
Lines 5191-5228 (patched)


Move this after L5241.



src/tests/slave_tests.cpp
Lines 5208 (patched)


Add a comment here for posterity that the tasks fail due to the secret 
generation failing?



src/tests/slave_tests.cpp
Lines 5215 (patched)


Kill this newline?



src/tests/slave_tests.cpp
Lines 5219 (patched)


Why do you need this?



src/tests/slave_tests.cpp
Lines 5220 (patched)


New line before.

Also, can we wait on the `Future` for failure? It's not immediately clear 
to me if by the time `removeFramework` is invoked `failure()` would be invoked 
once. The test might be flaky otherwise.



src/tests/slave_tests.cpp
Lines 5267 (patched)


4 space indent.



src/tests/slave_tests.cpp
Lines 5269 (patched)


s/EXPECT_EQ/ASSERT_EQ

Do you want the expectation on L5272 to run otherwise?



src/tests/slave_tests.cpp
Lines 5277-5301 (patched)


Kill this? You are not expecting more status updates for the same task.



src/tests/slave_tests.cpp
Lines 5310-5311 (patched)


Reword as per the comments above?



src/tests/slave_tests.cpp
Lines 5319 (patched)


Move this.



src/tests/slave_tests.cpp
Lines 5321-5322 (patched)


Use v1 helpers as per earlier comment.



src/tests/slave_tests.cpp
Lines 5383 (patched)


ASSERT_NE



src/tests/slave_tests.cpp
Lines 5385-5401 (patched)


Move this before sending the `ACCEPT` call as per my earlier comment.



src/tests/slave_tests.cpp
Lines 5403 (patched)


Also add a comment on why the task failed updates are generated?



src/tests/slave_tests.cpp
Lines 5406-5409 (patched)


Newline before.

```cpp
authenticationToken.mutable_reference()->set_name(...);
authenticationToken.mutable_reference()->set_key(...);
```



src/tests/slave_tests.cpp
Lines 5416 (patched)


Kill this newline



src/tests/slave_tests.cpp
Lines 5421 (patched)


newline

Also, might want to wait on this `Future` similar to my comment in the 
earlier test.



src/tests/slave_tests.cpp
Lines 5470 (patched)


s/EXPECT_EQ/ASSERT_EQ.



src/tests/slave_tests.cpp
Lines 5475 (patched)


Newline after a multi line statement.



src/tests/slave_tests.cpp
Lines 5480-5504 (patched)

Re: Review Request 57911: WIP: Added UNKNOWN DiskInfo.Source type.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57911]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 24, 2017, 3:39 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57911/
> ---
> 
> (Updated March 24, 2017, 3:39 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We introduce an explicit UNKNOWN enum kind to allow explicit handling
> of unknown enum values (e.g., when the sending and receiving end use
> different versions of a message using the enum).
> 
> This commit also migrates pattern matching of values of this enum from
> if statements to switch statements so that compiler diagnostics can be
> used to identify unhandled cases when other types are added in the
> future.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 9a66967ab459f75f21faf21be644f39b3fad670b 
>   include/mesos/v1/mesos.proto 115f1b4d9e129f83a3aed62a95eb11faa12e04d1 
>   src/common/resources.cpp ca1add1dbbe04fa775004010ebc847254c198bd7 
>   src/slave/paths.cpp 38ad1993aa36a627ec97a7865488677495ee4c5a 
>   src/slave/slave.cpp c6ee4fa9ad216fe82e97095e1808bb1fb1585398 
>   src/v1/resources.cpp 6008a9d2572d04612f0da2d54834be548d366f29 
> 
> 
> Diff: https://reviews.apache.org/r/57911/diff/1/
> 
> 
> Testing
> ---
> 
> make check (OS X, Linux)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 57891: Added a new unmocked method to the mock agent.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On March 23, 2017, 8:32 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57891/
> ---
> 
> (Updated March 23, 2017, 8:32 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new method to the mock agent,
> `MockSlave::unmocked_executorTerminated`. This will
> facilitate the testing of executor secret generation,
> as it allows us to forward the mock function call to
> the base class method.
> 
> 
> Diffs
> -
> 
>   src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
>   src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 
> 
> 
> Diff: https://reviews.apache.org/r/57891/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57882: Added SecretGenerator injection to the MockSlave.

[Anand Mazumdar]

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




src/tests/mock_slave.hpp
Lines 118 (patched)


`
virtual void initialize() override;
` ?



src/tests/mock_slave.hpp
Lines 206 (patched)


How about keeping this consistent now that we are using `Owned` on the 
agent? Use `Option>` instead.

Modify the above comment accordingly.



src/tests/mock_slave.cpp
Lines 156 (patched)


Use `swap()` instead once it's changed to `Owned`. Also, invoke `reset` on 
the current pointer after its swapped.


- Anand Mazumdar


On March 23, 2017, 8:44 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57882/
> ---
> 
> (Updated March 23, 2017, 8:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an overload of `Slave::initialize` to the
> mock slave for the purpose of injecting a secret generator.
> 
> 
> Diffs
> -
> 
>   src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 
>   src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf 
> 
> 
> Diff: https://reviews.apache.org/r/57882/diff/2/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57358: Implemented discard behavior in process::Queue.

[Greg Mann]

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




3rdparty/libprocess/include/process/queue.hpp
Line 58 (original), 58 (patched)


I was just testing this patch locally, and noticed that after applying it, 
the NetSocketTests will deadlock. If I run 
`3rdparty/libprocess/libprocess-tests --gtest_filter="*NetSocketTest.*"` the 
deadlock appears. Haven't had a chance to dig in yet.


- Greg Mann


On March 21, 2017, 9:40 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57358/
> ---
> 
> (Updated March 21, 2017, 9:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Greg Mann.
> 
> 
> Bugs: MESOS-6919
> https://issues.apache.org/jira/browse/MESOS-6919
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a consumer calls `Queue::get` on an empty Queue, the Queue creates
> a Promise to back the Future return value.  This Promise currently
> does not have a discard handler, which may cause problems if the
> Future is chained to multiple continuations.  For example, see
> the scenario in MESOS-6919.
> 
> This commit implements an (potentially expensive) discard handler
> on the Queue's Promise.  If the Future return value is discarded,
> the Queue will remove the corresponding Promise from its internal
> queue of promises.  The operation is expensive because it needs
> to reconstruct the entire internal queue of promises.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/queue.hpp 
> ab08e30df742412f22a06202526f8b55350ed435 
>   3rdparty/libprocess/src/tests/queue_tests.cpp 
> 95b738133fa50641f8f9b83014837d2808e0e4ff 
> 
> 
> Diff: https://reviews.apache.org/r/57358/diff/4/
> 
> 
> Testing
> ---
> 
> cmake .. -DENABLE_LIBEVENT=1 -DENABLE_SSL=1
> 
> make libprocess-tests
> 
> 3rdparty/libprocess/src/tests/libprocess-tests 
> --gtest_filter="Scheme/HTTPTest.Endpoints/0" --gtest_repeat="`ulimit -n`"
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 57912: Enabled pause/resume for checks.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and Vinod Kone.


Bugs: MESOS-7277
https://issues.apache.org/jira/browse/MESOS-7277


Repository: mesos


Description
---

Enabled pause/resume for checks.


Diffs
-

  src/checks/checker.hpp e8af3160c9fd52ec20acf41b86bade50f4539fb1 
  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/launcher/default_executor.cpp ee24531b7172dbda07999e5a1101698c395a8272 
  src/launcher/executor.cpp a4bac9d932b6574eb2e1b4a27aa44eaaca6aaa62 


Diff: https://reviews.apache.org/r/57912/diff/1/


Testing
---

`make check` on Fedora 24


Thanks,

Gastón Kleiman

Re: Review Request 57839: WIP: Added an example framework demonstrating multirole capabilities.

[Benjamin Bannier]

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

(Updated March 24, 2017, 4:39 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

This commit introduces an examples framework which can register in
multiple roles.


Diffs
-

  src/Makefile.am 071656ad7354a802e8292140a7181cb70b68fe9e 
  src/examples/multirole_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/multirole_framework_test.sh PRE-CREATION 


Diff: https://reviews.apache.org/r/57839/diff/1/


Testing (updated)
---

make check (OS X, Linux)


Thanks,

Benjamin Bannier

Review Request 57911: WIP: Added UNKNOWN DiskInfo.Source type.

[Benjamin Bannier]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

We introduce an explicit UNKNOWN enum kind to allow explicit handling
of unknown enum values (e.g., when the sending and receiving end use
different versions of a message using the enum).

This commit also migrates pattern matching of values of this enum from
if statements to switch statements so that compiler diagnostics can be
used to identify unhandled cases when other types are added in the
future.


Diffs
-

  include/mesos/mesos.proto 9a66967ab459f75f21faf21be644f39b3fad670b 
  include/mesos/v1/mesos.proto 115f1b4d9e129f83a3aed62a95eb11faa12e04d1 
  src/common/resources.cpp ca1add1dbbe04fa775004010ebc847254c198bd7 
  src/slave/paths.cpp 38ad1993aa36a627ec97a7865488677495ee4c5a 
  src/slave/slave.cpp c6ee4fa9ad216fe82e97095e1808bb1fb1585398 
  src/v1/resources.cpp 6008a9d2572d04612f0da2d54834be548d366f29 


Diff: https://reviews.apache.org/r/57911/diff/1/


Testing
---

make check (OS X, Linux)


Thanks,

Benjamin Bannier

Re: Review Request 57880: Added a 'MockSecretGenerator'.

[Greg Mann]

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

(Updated March 24, 2017, 2:07 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch adds a mock secret generator, which will
be used for testing failure scenarios involving
executor secret generation.


Diffs (updated)
-

  src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 


Diff: https://reviews.apache.org/r/57880/diff/3/

Changes: https://reviews.apache.org/r/57880/diff/2-3/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 57879: Turned on executor authentication in the DefaultExecutorTests.

[Greg Mann]

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

(Updated March 24, 2017, 2:05 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

Now that executor authentication has been added, this patch
enables authentication for the agent's operator endpoint in
the DefaultExecutorTests. This is only done when build with
SSL, since executor authentication currently has SSL as a
dependency.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp b7440d3c0b2b1b09f3237d2971ba99f904098c4b 


Diff: https://reviews.apache.org/r/57879/diff/2/

Changes: https://reviews.apache.org/r/57879/diff/1-2/


Testing
---

Testing details can be found at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 57807: Updated 'SchedulerTest.TaskGroupRunning'.

[Greg Mann]

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

(Updated March 24, 2017, 2:05 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Benjamin Mahler, and 
Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch updates the test `SchedulerTest.TaskGroupRunning`
to confirm that the agent-side code responsible for launching
task groups works correctly. Previously, this test only
verified that the `RunTaskGroupMessage` was sent to the agent.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp 65259b47ec1a2094b23ff448370d30122fd218ca 


Diff: https://reviews.apache.org/r/57807/diff/7/

Changes: https://reviews.apache.org/r/57807/diff/6-7/


Testing
---

`make check` was done to test that all existing tests succeed when executor 
authentication is enabled by default in the test cluster code. The 
ExecutorHttpApiTests are the only tests in which it has been disabled.


Thanks,

Greg Mann

Re: Review Request 57854: Improved log/failure messages in the (health)checker libraries.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57644, 57648, 56288, 55901, 57645, 57646, 57647, 57854]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 24, 2017, 7:49 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57854/
> ---
> 
> (Updated March 24, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Made log/failure messages consistent across both libraries.
> - Task and container IDs are user generated and can contain spaces, so
>   we have to wrap them in single quotes.
> - Removed the redundant task IDs from 'Failure' messages.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
>   src/checks/health_checker.cpp 2211228f7aa0228af64d8fce6c5f2dd1847328f9 
>   src/tests/check_tests.cpp 16f1c07e109e24d475ad593ef1992dfb9f482ba6 
> 
> 
> Diff: https://reviews.apache.org/r/57854/diff/7/
> 
> 
> Testing
> ---
> 
> Did some manual testing and looked at the new log messages, they're so much 
> nicer now <3.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57750: Turned off executor authentication in the ExecutorHttpApiTests.

[Greg Mann]

> On March 24, 2017, 5:22 a.m., Anand Mazumdar wrote:
> >

I also changed the name of the flags variable from `agentFlags` to `flags`.


- Greg


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


On March 24, 2017, 1:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57750/
> ---
> 
> (Updated March 24, 2017, 1:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch turns off executor authentication in the
> ExecutorHttpApiTests, since authentication is not related
> to the functionality they are meant to test. Since executor
> tokens depend on the FrameworkID/ExecutorID/ContainerID of
> the executor, it would be difficult to enable it in these
> tests.
> 
> 
> Diffs
> -
> 
>   src/tests/executor_http_api_tests.cpp 
> 13d2e1db2ffdad34dadbf2911e91938706fd4fd0 
> 
> 
> Diff: https://reviews.apache.org/r/57750/diff/6/
> 
> 
> Testing
> ---
> 
> `make check` was done to test that all existing tests succeed when executor 
> authentication is enabled by default in the test cluster code. The 
> ExecutorHttpApiTests are the only tests in which it has been disabled.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57750: Turned off executor authentication in the ExecutorHttpApiTests.

[Greg Mann]

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

(Updated March 24, 2017, 1:07 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch turns off executor authentication in the
ExecutorHttpApiTests, since authentication is not related
to the functionality they are meant to test. Since executor
tokens depend on the FrameworkID/ExecutorID/ContainerID of
the executor, it would be difficult to enable it in these
tests.


Diffs (updated)
-

  src/tests/executor_http_api_tests.cpp 
13d2e1db2ffdad34dadbf2911e91938706fd4fd0 


Diff: https://reviews.apache.org/r/57750/diff/6/

Changes: https://reviews.apache.org/r/57750/diff/5-6/


Testing
---

`make check` was done to test that all existing tests succeed when executor 
authentication is enabled by default in the test cluster code. The 
ExecutorHttpApiTests are the only tests in which it has been disabled.


Thanks,

Greg Mann

Re: Review Request 57748: Enabled executor authentication in the tests.

[Greg Mann]

> On March 24, 2017, 4:52 a.m., Anand Mazumdar wrote:
> > src/tests/cluster.hpp
> > Lines 226 (patched)
> > 
> >
> > Why this change?

Sorry, this was an artifact from a previous revision. Removed.


- Greg


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


On March 24, 2017, 12:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57748/
> ---
> 
> (Updated March 24, 2017, 12:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch sets the `--authenticate_http_executors` and
> `--executor_secret_key` flags by default in the tests,
> and it updates the test cluster code to load a secret
> generator when those flags are set.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 250b12fcffd035834817ff6060eb80c2cf3e0246 
>   src/tests/cluster.cpp 15eaf4241873dc86281627db1002f9aadd26e6dc 
>   src/tests/mesos.cpp c507b86075f1c24afed3fedf3e8371464d82 
> 
> 
> Diff: https://reviews.apache.org/r/57748/diff/4/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57748: Enabled executor authentication in the tests.

[Greg Mann]

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

(Updated March 24, 2017, 12:54 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, and Vinod Kone.


Bugs: MESOS-6999
https://issues.apache.org/jira/browse/MESOS-6999


Repository: mesos


Description
---

This patch sets the `--authenticate_http_executors` and
`--executor_secret_key` flags by default in the tests,
and it updates the test cluster code to load a secret
generator when those flags are set.


Diffs (updated)
-

  src/tests/cluster.hpp 250b12fcffd035834817ff6060eb80c2cf3e0246 
  src/tests/cluster.cpp 15eaf4241873dc86281627db1002f9aadd26e6dc 
  src/tests/mesos.cpp c507b86075f1c24afed3fedf3e8371464d82 


Diff: https://reviews.apache.org/r/57748/diff/4/

Changes: https://reviews.apache.org/r/57748/diff/3-4/


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 57854: Improved log/failure messages in the (health)checker libraries.

[Gastón Kleiman]

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

(Updated March 24, 2017, 11:49 a.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Rebased.


Repository: mesos


Description
---

- Made log/failure messages consistent across both libraries.
- Task and container IDs are user generated and can contain spaces, so
  we have to wrap them in single quotes.
- Removed the redundant task IDs from 'Failure' messages.


Diffs (updated)
-

  src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
  src/checks/health_checker.cpp 2211228f7aa0228af64d8fce6c5f2dd1847328f9 
  src/tests/check_tests.cpp 16f1c07e109e24d475ad593ef1992dfb9f482ba6 


Diff: https://reviews.apache.org/r/57854/diff/7/

Changes: https://reviews.apache.org/r/57854/diff/6-7/


Testing
---

Did some manual testing and looked at the new log messages, they're so much 
nicer now <3.


Thanks,

Gastón Kleiman

Re: Review Request 57854: Improved log/failure messages in the (health)checker libraries.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [57854, 57647, 57646, 57645, 55901, 56288, 57648, 57644]

Failed command: python support/apply-reviews.py -n -r 57854

Error:
2017-03-24 09:10:48 URL:https://reviews.apache.org/r/57854/diff/raw/ 
[24738/24738] -> "57854.patch" [1]
error: patch failed: src/checks/health_checker.cpp:532
error: src/checks/health_checker.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/17546/console

- Mesos Reviewbot


On March 24, 2017, 4:42 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57854/
> ---
> 
> (Updated March 24, 2017, 4:42 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Made log/failure messages consistent across both libraries.
> - Task and container IDs are user generated and can contain spaces, so
>   we have to wrap them in single quotes.
> - Removed the redundant task IDs from 'Failure' messages.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp 314354cc374b453ec12e25e3d4730a92697468cf 
>   src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23 
>   src/tests/check_tests.cpp f035c16920deaf559420ae0d7d881330ff65ae44 
> 
> 
> Diff: https://reviews.apache.org/r/57854/diff/6/
> 
> 
> Testing
> ---
> 
> Did some manual testing and looked at the new log messages, they're so much 
> nicer now <3.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57783: Updated website for 1.1.1 release.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57783]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 20, 2017, 5:18 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57783/
> ---
> 
> (Updated March 20, 2017, 5:18 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   site/data/releases.yml 4bffe4df60bd0c7da68b94c7d680569494931a04 
>   site/source/blog/2017-03-14-mesos-1-1-1-released.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57783/diff/1/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57880: Added a 'MockSecretGenerator'.

[Anand Mazumdar]

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




src/tests/mesos.hpp
Lines 2147 (patched)


= default;



src/tests/mesos.hpp
Lines 2148 (patched)


= default;


- Anand Mazumdar


On March 23, 2017, 8:42 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57880/
> ---
> 
> (Updated March 23, 2017, 8:42 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a mock secret generator, which will
> be used for testing failure scenarios involving
> executor secret generation.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp f39e243c2c11bc1c9c757049fda2122727d1fef9 
> 
> 
> Diff: https://reviews.apache.org/r/57880/diff/2/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57762: Fixed environment duplication in command executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57762]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 24, 2017, 2:24 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57762/
> ---
> 
> (Updated March 24, 2017, 2:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, Greg Mann, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-7263
> https://issues.apache.org/jira/browse/MESOS-7263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp db703f054decbca62f7fbe98f5d28f6e4c6c9b0f 
> 
> 
> Diff: https://reviews.apache.org/r/57762/diff/7/
> 
> 
> Testing
> ---
> 
> make check + functional testing..
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 57879: Turned on executor authentication in the DefaultExecutorTests.

[Anand Mazumdar]

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




src/tests/default_executor_tests.cpp
Lines 86 (patched)


Similar to my comment in the previous review create a virtual function 
`CreateSlaveFlags()`.



src/tests/default_executor_tests.cpp
Lines 91 (patched)


Can you also add the reasoning as to why this is needed for posterity? You 
can copy/paste your comment in the previous review 

```cpp
  // Executor authentication currently has SSL as a dependency, so we cannot
  // require executors to authenticate with the agent's operator API if 
Mesos
  // was not built with SSL support.
```



src/tests/default_executor_tests.cpp
Lines 1395 (patched)


Ditto as above.



src/tests/default_executor_tests.cpp
Lines 1400 (patched)


Ditto as above.


- Anand Mazumdar


On March 23, 2017, 4:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57879/
> ---
> 
> (Updated March 23, 2017, 4:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that executor authentication has been added, this patch
> enables authentication for the agent's operator endpoint in
> the DefaultExecutorTests. This is only done when build with
> SSL, since executor authentication currently has SSL as a
> dependency.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> b7440d3c0b2b1b09f3237d2971ba99f904098c4b 
> 
> 
> Diff: https://reviews.apache.org/r/57879/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this patch chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57807: Updated 'SchedulerTest.TaskGroupRunning'.

[Anand Mazumdar]

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




src/tests/scheduler_tests.cpp
Lines 543 (patched)


s/agent's/agent ?



src/tests/scheduler_tests.cpp
Lines 575 (patched)


// Ignore subsequent offers.



src/tests/scheduler_tests.cpp
Lines 673 (patched)


s/EXPECT_EQ/ASSERT_EQ?



src/tests/scheduler_tests.cpp
Lines 676 (patched)


s/EXPECT_EQ/ASSERT_EQ?



src/tests/scheduler_tests.cpp
Lines 680 (patched)


Can you add a comment here for posterity similar to what we have in the 
default executor tests:

```cpp
// When running a task, TASK_RUNNING updates for the tasks in a
  // task group can be received in any order.
```



src/tests/scheduler_tests.cpp
Lines 684 (patched)


s/EXPECT_EQ/ASSERT_EQ?

Do you want to continue the test if even the initial status updates don't 
match?



src/tests/scheduler_tests.cpp
Lines 685 (patched)


```cpp
// Acknowledge the TASK_RUNNING updates to receive the next updates.
```



src/tests/scheduler_tests.cpp
Lines 718 (patched)


hmm, why not check of the task state in the update is `TASK_FINISHED`?


- Anand Mazumdar


On March 23, 2017, 11:17 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57807/
> ---
> 
> (Updated March 23, 2017, 11:17 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Benjamin Mahler, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the test `SchedulerTest.TaskGroupRunning`
> to confirm that the agent-side code responsible for launching
> task groups works correctly. Previously, this test only
> verified that the `RunTaskGroupMessage` was sent to the agent.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp 65259b47ec1a2094b23ff448370d30122fd218ca 
> 
> 
> Diff: https://reviews.apache.org/r/57807/diff/6/
> 
> 
> Testing
> ---
> 
> `make check` was done to test that all existing tests succeed when executor 
> authentication is enabled by default in the test cluster code. The 
> ExecutorHttpApiTests are the only tests in which it has been disabled.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>