Re: Review Request 55901: Added support for COMMAND health checks to the default executor.

2017-03-23 Thread Gastón Kleiman 

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

(Updated March 23, 2017, 11:39 p.m.)


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


Summary (updated)
-

Added support for COMMAND health checks to the default executor.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs
-

  src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867 
  src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23 
  src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694 
  src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171 


Diff: https://reviews.apache.org/r/55901/diff/19/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux and macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-03-16 Thread Gastón Kleiman 

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

(Updated March 16, 2017, 2:31 p.m.)


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


Changes
---

Rebased + updated accept type.


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


Repository: mesos


Description (updated)
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867 
  src/checks/health_checker.cpp 7efefe9455df20a038a2c9d4f67c0c0cd69c301e 
  src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d 
  src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171 


Diff: https://reviews.apache.org/r/55901/diff/15/

Changes: https://reviews.apache.org/r/55901/diff/14-15/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-03-16 Thread Vinod Kone 

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


Fix it, then Ship it!





src/checks/health_checker.cpp
Lines 505-506 (patched)


We changed the `Accept` type for streaming responses recently (though the 
old type is still accepted for backwards compat)

```
Accept : stringify(ContentType::RECORDIO);
Message-Accept: stringify(ContentType::ContentType::PROTOBUF)
```


- Vinod Kone


On March 15, 2017, 3:05 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated March 15, 2017, 3:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See the summary.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
>   src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
>   src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171 
> 
> 
> Diff: https://reviews.apache.org/r/55901/diff/14/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-03-15 Thread Gastón Kleiman 

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

(Updated March 15, 2017, 3:05 p.m.)


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


Changes
---

Rebase + added a TODO.


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


Repository: mesos


Description (updated)
---

See the summary.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
  src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
  src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171 


Diff: https://reviews.apache.org/r/55901/diff/14/

Changes: https://reviews.apache.org/r/55901/diff/13-14/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-03-15 Thread Gastón Kleiman 


> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp
> > Lines 617 (patched)
> > 
> >
> > do you want to add a TODO here to not re-use the ContainerID?
> 
> Gastón Kleiman wrote:
> I don't know if we should commit this with that TODO, or if I should wait 
> until MESOS-7120 is resolved, and then update this patch to not re-use the 
> ContainerID.

Added a TODO and addressed it in https://reviews.apache.org/r/57647/.


- Gastón


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


On March 6, 2017, 4:23 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated March 6, 2017, 4:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
>   src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
>   src/tests/health_check_tests.cpp 56e90747f2c943daee675738428d8ddeeafde36d 
> 
> 
> Diff: https://reviews.apache.org/r/55901/diff/13/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-03-06 Thread Gastón Kleiman 

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

(Updated March 6, 2017, 4:23 p.m.)


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


Changes
---

Rebased + removed env inheritance.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
  src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
  src/tests/health_check_tests.cpp 56e90747f2c943daee675738428d8ddeeafde36d 


Diff: https://reviews.apache.org/r/55901/diff/13/

Changes: https://reviews.apache.org/r/55901/diff/12-13/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-17 Thread Gastón Kleiman 

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

(Updated Feb. 17, 2017, 11:59 a.m.)


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


Changes
---

Addressed feedback.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
  src/launcher/default_executor.cpp d97324b0e5eb3f145ee93aa0821205c5eefbfce0 
  src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-16 Thread Gastón Kleiman 

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

(Updated Feb. 16, 2017, 2:59 p.m.)


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


Changes
---

Addresed Vinod's feedback.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
  src/launcher/default_executor.cpp d97324b0e5eb3f145ee93aa0821205c5eefbfce0 
  src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-16 Thread Gastón Kleiman 


> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 664
> > 
> >
> > won't we be losing the info about why wait failed?

Yes, but the health check timed out anyway, we call `WaitNestedContainer`, only 
to ensure that the next `LaunchNestedContainerSession` call won't fail, so I 
don't think that a user would care about the details of the 
`WaitNestedContainer` call. We could log the response `VLOG(1)` or `VLOG(2)`, 
to make it easier to debug potential Mesos bugs.

In our offline discussions we also realized that reusing the `ContainerID` is 
not always safe, so we'll have to replace this wait call with a wait + 
cleanupContainer.


> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 616
> > 
> >
> > is this deferred only because you want to access `taskId`? why not just 
> > pass taskId to the lambda directly and not-defer?

Because the compiler complains about not being able to capture a non-variable:

```
../../src/checks/health_checker.cpp:584:26: error: capture of non-variable 
‘mesos::internal::checks::HealthCheckerProcess::taskId’
```


> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 654
> > 
> >
> > do you want to add a TODO here to not re-use the ContainerID?

I don't know if we should commit this with that TODO, or if I should wait until 
MESOS-7120 is resolved, and then update this patch to not re-use the 
ContainerID.


- Gastón


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


On Feb. 10, 2017, 6:40 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 10, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-15 Thread Vinod Kone 

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




src/checks/health_checker.hpp (line 183)


What is it in the case of Docker containerizer? I think it is worth 
mentioning.



src/checks/health_checker.cpp (line 579)


is this deferred only because you want to access `taskId`? why not just 
pass taskId to the lambda directly and not-defer?



src/checks/health_checker.cpp (line 582)


s/of a command/of command/



src/checks/health_checker.cpp (line 608)


s/not returned/timed out/



src/checks/health_checker.cpp (line 617)


do you want to add a TODO here to not re-use the ContainerID?



src/checks/health_checker.cpp (line 619)


why does this wait has repair but not the one in 
`__nestedCommandHealthCheck` ?



src/checks/health_checker.cpp (line 620)


s/WaitForNestedContainer/WaitNestedContainer/



src/checks/health_checker.cpp (lines 620 - 621)


s/no matter if/irrespective of whether/



src/checks/health_checker.cpp (line 625)


s/WaitFor/Wait/



src/checks/health_checker.cpp (line 627)


won't we be losing the info about why wait failed?



src/tests/health_check_tests.cpp (line 2122)


s/is/in/


- Vinod Kone


On Feb. 10, 2017, 6:40 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 10, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-10 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [55900, 56288, 55901]

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 Feb. 10, 2017, 6:40 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 10, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-10 Thread Gastón Kleiman 


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 653-654
> > 
> >
> > Why not `defer`?

Added `defer` where it corresponds.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 635
> > 
> >
> > Could you please specify what this function returns? Status or exit 
> > code?

This depends on what containerizer the agent is using. In the case of the Mesos 
containerizer, it currently returns a status code.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 667
> > 
> >
> > What do you get from the agent call: status or exit code directly? It's 
> > probably not that important here, but it is for checks.

This depends on what containerizer the agent is using. In the case of the Mesos 
containerizer, it currently returns a status code.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 565-567
> > 
> >
> > This should probably be indented, no?

Fixed.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 555
> > 
> >
> > s/until/until after/ ?

Fixed.


> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 615
> > 
> >
> > s/this future is completed//we finish processing current timeout./
> > 
> > It's not clear what "this future" means.

Fixed.


- Gastón


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


On Feb. 10, 2017, 6:40 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 10, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS, because of MESOS-7050.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-10 Thread Gastón Kleiman 

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

(Updated Feb. 10, 2017, 6:40 p.m.)


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


Changes
---

Split `waitNestedContainer` into two + use defer instead of plain lambdas.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-10 Thread Gastón Kleiman 

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

(Updated Feb. 10, 2017, 11:25 a.m.)


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


Changes
---

Addressed feedback.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 

Diff: https://reviews.apache.org/r/55901/diff/


Testing (updated)
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-10 Thread Gastón Kleiman 

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




src/checks/health_checker.cpp (line 657)


I wanted to make the errors here similar to/consistent with the errors 
logged by the default executor:

https://github.com/apache/mesos/blob/c2388a511c775dd6f392961b06fd7738bf051dbc/src/launcher/default_executor.cpp#L618-L624



src/checks/health_checker.cpp (line 667)


It depends on what containerizer is being used. With the 
`MesosContainerizer`, we get the exit status as returned by `waitpid`. I added 
a comment in the header file.


- Gastón Kleiman


On Feb. 8, 2017, 1:27 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 8, 2017, 1:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-09 Thread Alexander Rukletsov 

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


Fix it, then Ship it!




LGTM modulo possible collision with the previous check on timeout. Please 
confirm such collision is not possible.


src/checks/health_checker.cpp (lines 338 - 342)


How about `checkResult = commandCheckViaAgent ? nestedCommandHealthCheck() 
: commandHealthCheck();` ?



src/checks/health_checker.cpp (line 555)


s/until/until after/ ?



src/checks/health_checker.cpp (lines 565 - 567)


This should probably be indented, no?



src/checks/health_checker.cpp (lines 575 - 580)


After thinking some time I think I understand why we treat any HTTP status 
code other than 200 as health check failure. I think it still makes sense to 
capture this in a comment explaining that if the agent cannot launch a nested 
container for _any_ reason, it is considered a health check failure. We should 
also mention this when we update the documentaiton.



src/checks/health_checker.cpp (line 615)


s/this future is completed//we finish processing current timeout./

It's not clear what "this future" means.



src/checks/health_checker.cpp (lines 623 - 628)


I think what you're trying to say is that it is fine to complete the future 
returned from this call if `waitForNestedContainer` fails, because 1) debug 
containers are not checkpointed and 2) `waitForNestedContainer` fails iff agent 
fails as well hence cleaning up the container.

Could you please rephrase the comment mentioning both your expectation (no 
wait retry is fine) and reasoning?



src/checks/health_checker.cpp (line 635)


Could you please specify what this function returns? Status or exit code?



src/checks/health_checker.cpp (lines 653 - 654)


Why not `defer`?



src/checks/health_checker.cpp (lines 654 - 670)


I would extract this lambda into a separate callback for readability.



src/checks/health_checker.cpp (line 657)


Maybe saying "Agent returned" instead of "Received"?



src/checks/health_checker.cpp (line 666)


please `CHECK` that `response->has_wait_nested_container()`.



src/checks/health_checker.cpp (lines 666 - 670)


```
  return (
response->wait_nested_container().has_exit_status()
  ? response->wait_nested_container().exit_status()
  : None());
```



src/checks/health_checker.cpp (line 667)


What do you get from the agent call: status or exit code directly? It's 
probably not that important here, but it is for checks.


- Alexander Rukletsov


On Feb. 8, 2017, 1:27 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 8, 2017, 1:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-09 Thread Gastón Kleiman 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.hpp, line 137
> > 
> >
> > we don't typically do `const` for POD types.
> > 
> > s/_agentSpawnsCommandContainer/
> 
> Gastón Kleiman wrote:
> Looks lke the regex was truncated ;-).
> 
> Vinod Kone wrote:
> How about "localCommandCheck" (vs "remoteCommandCheck")?
> 
> Alexander Rukletsov wrote:
> We are not consistent about `const` that's true, but I think we are 
> starting to use it more and more often. Unfortunately, I don't have extensive 
> data at hand, here are some links:
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/slave.hpp#L970
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp#L55
> 
> Vinod Kone wrote:
> Making a POD member variable const is different from making it a const in 
> function argument. We definitely do the former but not the latter IIRC.
> 
> Alexander Rukletsov wrote:
> Agree.

I'm going with `commandCheckViaAgent` and not marking it `const`.


- Gastón


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


On Feb. 8, 2017, 1:27 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 8, 2017, 1:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-08 Thread Alexander Rukletsov 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, lines 426-429
> > 
> >
> > why a `.repair()` here?
> 
> Gastón Kleiman wrote:
> To fail the future/check with a nice descriptive message that will later 
> be logged.
> 
> Vinod Kone wrote:
> The message returned by `http::connect` should be good enough? Do we use 
> this pattern elsewhere esp. with connect? Most uses of repair I have seen in 
> the code base, transform the future not really to add extra logging 
> information.
> 
> Alexander Rukletsov wrote:
> We use `.repair` for adjusting the message or changing the error type, 
> which is technically the same. Here are some examples from the code:
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/3rdparty/libprocess/src/http.cpp#L1766-L1769
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/master/http.cpp#L4695-L4697
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/slave/containerizer/mesos/io/switchboard.cpp#L1632-L1638
> 
> Now, the question is whether connection failure message is good enough? 
> Gastón, could you trigger a failure path and check what error message will be 
> returned?
> 
> Gastón Kleiman wrote:
> This is how it looks like with the repair:
> 
> ```
> W0207 10:42:19.659122  9361 health_checker.cpp:314] Health check failed 1 
> times consecutively: COMMAND health check failed: Unable to establish 
> connection with the agent: Failed to connect to 192.99.40.208:31338: 
> Connection refused
> ```
> 
> Without the repair it would look like this:
> 
> ```
> W0207 10:42:19.659122  9361 health_checker.cpp:314] Health check failed 1 
> times consecutively: COMMAND health check failed: Failed to connect to 
> 192.99.40.208:31338: Connection refused
> ```

I'd vote for keeping `.repair`.


- Alexander


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


On Feb. 8, 2017, 1:27 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 8, 2017, 1:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-08 Thread Gastón Kleiman 

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

(Updated Feb. 8, 2017, 1:27 p.m.)


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


Changes
---

Fixed broken rebase.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Alexander Rukletsov 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.hpp, line 137
> > 
> >
> > we don't typically do `const` for POD types.
> > 
> > s/_agentSpawnsCommandContainer/
> 
> Gastón Kleiman wrote:
> Looks lke the regex was truncated ;-).
> 
> Vinod Kone wrote:
> How about "localCommandCheck" (vs "remoteCommandCheck")?
> 
> Alexander Rukletsov wrote:
> We are not consistent about `const` that's true, but I think we are 
> starting to use it more and more often. Unfortunately, I don't have extensive 
> data at hand, here are some links:
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/slave.hpp#L970
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp#L55
> 
> Vinod Kone wrote:
> Making a POD member variable const is different from making it a const in 
> function argument. We definitely do the former but not the latter IIRC.

Agree.


- Alexander


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Gastón Kleiman 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, lines 426-429
> > 
> >
> > why a `.repair()` here?
> 
> Gastón Kleiman wrote:
> To fail the future/check with a nice descriptive message that will later 
> be logged.
> 
> Vinod Kone wrote:
> The message returned by `http::connect` should be good enough? Do we use 
> this pattern elsewhere esp. with connect? Most uses of repair I have seen in 
> the code base, transform the future not really to add extra logging 
> information.
> 
> Alexander Rukletsov wrote:
> We use `.repair` for adjusting the message or changing the error type, 
> which is technically the same. Here are some examples from the code:
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/3rdparty/libprocess/src/http.cpp#L1766-L1769
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/master/http.cpp#L4695-L4697
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/slave/containerizer/mesos/io/switchboard.cpp#L1632-L1638
> 
> Now, the question is whether connection failure message is good enough? 
> Gastón, could you trigger a failure path and check what error message will be 
> returned?

This is how it looks like with the repair:

```
W0207 10:42:19.659122  9361 health_checker.cpp:314] Health check failed 1 times 
consecutively: COMMAND health check failed: Unable to establish connection with 
the agent: Failed to connect to 192.99.40.208:31338: Connection refused
```

Without the repair it would look like this:

```
W0207 10:42:19.659122  9361 health_checker.cpp:314] Health check failed 1 times 
consecutively: COMMAND health check failed: Failed to connect to 
192.99.40.208:31338: Connection refused
```


- Gastón


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Gastón Kleiman 


> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 618-624
> > 
> >
> > 1) You have no interest in both `future` and `status`. Omit the names 
> > for clarity, please.
> > 
> > 2) Why do we need to call `waitForNestedContainer` here instead of 
> > simply returning `failure`?

Added a comment clarifying this.


> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 653
> > 
> >
> > Don't you need to check that `wait_nested_container().exit_status()` 
> > exists? If `exit_status` is not set in the proto, don't you have to check 
> > this explicitly and return `None`?

Very good catch, thanks!


> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 585-588
> > 
> >
> > Mention `taskId`?

Updated all the comments I could find.


- Gastón


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Gastón Kleiman 

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

(Updated Feb. 7, 2017, 11:37 p.m.)


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


Changes
---

Added a comment in the new test.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Gastón Kleiman 


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 130
> > 
> >
> > s/createRequest/createV1AgentAPIRequest?
> > remove `inline`
> > 
> > This looks like a utility function not really related to the health 
> > checker library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?
> 
> Gastón Kleiman wrote:
> I renamed the function. Vinod suggested to make it `inline` what's wrong 
> with that?
> 
> This function as-is is specific to how we perform requests in this file 
> (`keep_alive = false`, protobuf serialization). Since it is used only in two 
> places, I'm tempted to removed it and put the code inline in the 
> corresponding methods.

I removed these functions.


- Gastón


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Gastón Kleiman 

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

(Updated Feb. 7, 2017, 11:32 p.m.)


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


Changes
---

Addressed Alex's comments.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Alexander Rukletsov 

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




src/checks/health_checker.cpp (line 512)


You can use `->` for options instead of `.get()`.



src/checks/health_checker.cpp (lines 585 - 588)


Mention `taskId`?



src/checks/health_checker.cpp (line 594)


How about "WAIT_NESTED_CONTAINER API call..."



src/checks/health_checker.cpp (lines 618 - 624)


1) You have no interest in both `future` and `status`. Omit the names for 
clarity, please.

2) Why do we need to call `waitForNestedContainer` here instead of simply 
returning `failure`?



src/checks/health_checker.cpp (lines 640 - 641)


Is this formatting correct? Is it what clang-format suggests?



src/checks/health_checker.cpp (line 646)


"... while waiting..."?



src/checks/health_checker.cpp (line 653)


Don't you need to check that `wait_nested_container().exit_status()` 
exists? If `exit_status` is not set in the proto, don't you have to check this 
explicitly and return `None`?


- Alexander Rukletsov


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-07 Thread Alexander Rukletsov 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, lines 426-429
> > 
> >
> > why a `.repair()` here?
> 
> Gastón Kleiman wrote:
> To fail the future/check with a nice descriptive message that will later 
> be logged.
> 
> Vinod Kone wrote:
> The message returned by `http::connect` should be good enough? Do we use 
> this pattern elsewhere esp. with connect? Most uses of repair I have seen in 
> the code base, transform the future not really to add extra logging 
> information.

We use `.repair` for adjusting the message or changing the error type, which is 
technically the same. Here are some examples from the code:
https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/3rdparty/libprocess/src/http.cpp#L1766-L1769
https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/master/http.cpp#L4695-L4697
https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/slave/containerizer/mesos/io/switchboard.cpp#L1632-L1638

Now, the question is whether connection failure message is good enough? Gastón, 
could you trigger a failure path and check what error message will be returned?


- Alexander


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


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-06 Thread Vinod Kone 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.hpp, line 137
> > 
> >
> > we don't typically do `const` for POD types.
> > 
> > s/_agentSpawnsCommandContainer/
> 
> Gastón Kleiman wrote:
> Looks lke the regex was truncated ;-).
> 
> Vinod Kone wrote:
> How about "localCommandCheck" (vs "remoteCommandCheck")?
> 
> Alexander Rukletsov wrote:
> We are not consistent about `const` that's true, but I think we are 
> starting to use it more and more often. Unfortunately, I don't have extensive 
> data at hand, here are some links:
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/slave.hpp#L970
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp#L55

Making a POD member variable const is different from making it a const in 
function argument. We definitely do the former but not the latter IIRC.


- Vinod


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


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-06 Thread Alexander Rukletsov 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.hpp, line 137
> > 
> >
> > we don't typically do `const` for POD types.
> > 
> > s/_agentSpawnsCommandContainer/
> 
> Gastón Kleiman wrote:
> Looks lke the regex was truncated ;-).
> 
> Vinod Kone wrote:
> How about "localCommandCheck" (vs "remoteCommandCheck")?

We are not consistent about `const` that's true, but I think we are starting to 
use it more and more often. Unfortunately, I don't have extensive data at hand, 
here are some links:
https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/slave.hpp#L970
https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp#L55


- Alexander


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


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-06 Thread Alexander Rukletsov 


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2154-2155
> > 
> >
> > Formatting.
> > 
> > Also it is probably a good idea to explicitly say you don't care about 
> > further updates.
> 
> Gastón Kleiman wrote:
> Fixed the formatting. We don't have such a comment in most of the other 
> tests in this file.

Oh, under say I mean `.WillRepeatedly(Return());`


- Alexander


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


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-06 Thread Alexander Rukletsov 


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2118-2119
> > 
> >
> > Why do we need to expilictly create `containerizer`?
> 
> Gastón Kleiman wrote:
> Because the implicit `containerizer` is started in local mode. 
> `LaunchNestedContainerSession` (used by cmd health checks) tries to start a 
> IO switchboard, which doesn't work in local mode yet.

Please capture this in a comment.


- Alexander


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


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-03 Thread Gastón Kleiman 

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

(Updated Feb. 3, 2017, 7:52 p.m.)


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


Changes
---

Split into two patches.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-03 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [55900, 55901]

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 Feb. 3, 2017, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-03 Thread Gastón Kleiman 


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2118-2119
> > 
> >
> > Why do we need to expilictly create `containerizer`?

Because the implicit `containerizer` is started in local mode. 
`LaunchNestedContainerSession` (used by cmd health checks) tries to start a IO 
switchboard, which doesn't work in local mode yet.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.hpp, line 207
> > 
> >
> > How about calling it `commandCheckViaAgent`?

Good idea, done.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 130
> > 
> >
> > s/createRequest/createV1AgentAPIRequest?
> > remove `inline`
> > 
> > This looks like a utility function not really related to the health 
> > checker library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?

I renamed the function. Vinod suggested to make it `inline` what's wrong with 
that?

This function as-is is specific to how we perform requests in this file 
(`keep_alive = false`, protobuf serialization). Since it is used only in two 
places, I'm tempted to removed it and put the code inline in the corresponding 
methods.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, line 2107
> > 
> >
> > Why not spelling out `cmd`?

Renamed.


> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2154-2155
> > 
> >
> > Formatting.
> > 
> > Also it is probably a good idea to explicitly say you don't care about 
> > further updates.

Fixed the formatting. We don't have such a comment in most of the other tests 
in this file.


- Gastón


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


On Feb. 3, 2017, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-03 Thread Gastón Kleiman 

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

(Updated Feb. 3, 2017, 5:15 p.m.)


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


Changes
---

Addressed feedback.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-03 Thread Alexander Rukletsov 

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




src/checks/health_checker.hpp (line 94)


Could you please add a todo here saying this will go away and link a jira 
ticket?



src/checks/health_checker.hpp (line 207)


How about calling it `commandCheckViaAgent`?



src/checks/health_checker.cpp (line 130)


s/createRequest/createV1AgentAPIRequest?
remove `inline`

This looks like a utility function not really related to the health checker 
library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?



src/checks/health_checker.cpp (lines 146 - 170)


What is the value of using these helpers? I'd say having 
`connection.send(createRequest(url, call), false);` is clearer and self 
explanatory rather than `post(connection, url, call)`. And you don't need any 
comments then : )



src/checks/health_checker.cpp (line 224)


You can use `{}` instead.



src/checks/health_checker.cpp (lines 434 - 436)


Use `CHECK_SOME` instead.



src/checks/health_checker.cpp (lines 456 - 482)


Could you please link a JIRA here?



src/checks/health_checker.cpp (line 593)


Please keep the order of the definitions in sync with the order of 
declarations.



src/checks/health_checker.cpp (line 746)


Let's pull this (together with another below) into a separate patch.



src/tests/health_check_tests.cpp (line 2107)


Why not spelling out `cmd`?



src/tests/health_check_tests.cpp (lines 2118 - 2119)


Why do we need to expilictly create `containerizer`?



src/tests/health_check_tests.cpp (lines 2154 - 2155)


Formatting.

Also it is probably a good idea to explicitly say you don't care about 
further updates.



src/tests/health_check_tests.cpp (lines 2157 - 2161)


1. I think you can also check that task's env vars are available in the 
health check -> pass exit status in the task's command.
2. Let's avoid using obscure `populateTasks()` here. We'll get rid of it 
altogether in the nearest future.



src/tests/health_check_tests.cpp (lines 2185 - 2186)


check that `healthy` is set please.


- Alexander Rukletsov


On Feb. 2, 2017, 5:02 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 2, 2017, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-02 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [55900, 55901]

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 Feb. 2, 2017, 5:02 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 2, 2017, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-02 Thread Gastón Kleiman 

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

(Updated Feb. 2, 2017, 5:02 p.m.)


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


Changes
---

Addressed feedback + rebase.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-01 Thread Vinod Kone 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, lines 426-429
> > 
> >
> > why a `.repair()` here?
> 
> Gastón Kleiman wrote:
> To fail the future/check with a nice descriptive message that will later 
> be logged.

The message returned by `http::connect` should be good enough? Do we use this 
pattern elsewhere esp. with connect? Most uses of repair I have seen in the 
code base, transform the future not really to add extra logging information.


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.hpp, line 137
> > 
> >
> > we don't typically do `const` for POD types.
> > 
> > s/_agentSpawnsCommandContainer/
> 
> Gastón Kleiman wrote:
> Looks lke the regex was truncated ;-).

How about "localCommandCheck" (vs "remoteCommandCheck")?


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 480
> > 
> >
> > launch nested container session returns a streaming response, how come 
> > you are calling `post()` helper here which expects a non-streaming response?
> > 
> > probably one of the reasons why your test is hanging.
> 
> Gastón Kleiman wrote:
> The `post()` helper delegates to `process::http::request()`, which takes 
> boolean flag (`streamedResponse`). If this flag is set to `false`, libprocess 
> will convert a PIPE (streaming) response into a BODY (non-streaming) 
> response. This means that the `Future` returned by the helper will not be 
> completed until the server closes the connection.
> 
> Relevant links:
>  - 
> https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/include/process/http.hpp#L953-L955
>  - 
> https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/src/http.cpp#L1191-L1197
>  - 
> https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/src/http.cpp#L1005-L1022
> 
> In these particular cases, it means that the `Future` will not be 
> completed until the container exits, which is exactly what we need.
> 
> 
> Regarding the test hanging in Linux, some further debugging seem to 
> indicate that test hangs on `process::Clock::settle()`, because there's a 
> race + deadlock in `RateLimiter` that leaves a process stuck in `RUNNING`. 
> I'll dig deeper on Monday, but here's some evidence:
> 
> # Snippet from a successful test run that "leaks" a running process
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HealthCheckTest
> [ RUN  ] HealthCheckTest.DefaultExecutorCmdHealthCheck
> 
> [...]
> 
> E0127 11:18:06.607446  8665 limiter.hpp:123]  LIMITER _acquire
> E0127 11:18:06.607471  8665 limiter.hpp:129]  LIMITER There are 1 
> promises
> E0127 11:18:06.608064  8665 limiter.hpp:186]  LIMITER destroy
> 
>  DEADLOCK DETECTED! 
> You are waiting on process __limiter__(1419)@192.99.40.208:41947 that it 
> is currently executing.
> 
> [...]
> 
> [   OK ] HealthCheckTest.DefaultExecutorCmdHealthCheck (592 ms)
> [--] 1 test from HealthCheckTest (593 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (603 ms total)
> [  PASSED  ] 1 test.
> 
> Repeating all tests (iteration 474) . . .
> ```
> 
> # Snippet from a hung run
> 
> ```
> [...]
> E0127 11:18:06.878636  8640 cluster.cpp:358] !!! Settling the clock
> E0127 11:18:06.878646  8640 process.cpp:3491] !!! Attempting to acquire 
> mutex
> E0127 11:18:06.878654  8640 process.cpp:3494] !!! !runq.empty()?
> E0127 11:18:06.878659  8640 process.cpp:3502] !!! running.load() > 0?
> E0127 11:18:06.878667  8640 process.cpp:3504] !!! 1 processes still 
> running
> E0127 11:18:06.878676  8640 process.cpp:3509]  Process: 
> __authentication_router__(1) state: 3
> E0127 11:18:06.878684  8640 process.cpp:3509]  Process: 
> __basic_authenticator__(1893) state: 3
> E0127 11:18:06.878691  8640 process.cpp:3509]  Process: 
> __basic_authenticator__(1894) state: 3
> E0127 11:18:06.878697  8640 process.cpp:3509]  Process: 
> __basic_authenticator__(1895) state: 3
> E0127 11:18:06.878705  8640 process.cpp:3509]  Process: __gc__ state: 
> 3
> E0127 11:18:06.878711  8640 process.cpp:3509]  Process: 
> __limiter__(1419) state: 2
> E0127 11:18:06.878718  8640 process.cpp:3509]  Process: __processes__ 
> state: 3
> E0127 11:18:06.878725  8640 process.cpp:3509]

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-02-01 Thread Gastón Kleiman 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 480
> > 
> >
> > launch nested container session returns a streaming response, how come 
> > you are calling `post()` helper here which expects a non-streaming response?
> > 
> > probably one of the reasons why your test is hanging.
> 
> Gastón Kleiman wrote:
> The `post()` helper delegates to `process::http::request()`, which takes 
> boolean flag (`streamedResponse`). If this flag is set to `false`, libprocess 
> will convert a PIPE (streaming) response into a BODY (non-streaming) 
> response. This means that the `Future` returned by the helper will not be 
> completed until the server closes the connection.
> 
> Relevant links:
>  - 
> https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/include/process/http.hpp#L953-L955
>  - 
> https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/src/http.cpp#L1191-L1197
>  - 
> https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/src/http.cpp#L1005-L1022
> 
> In these particular cases, it means that the `Future` will not be 
> completed until the container exits, which is exactly what we need.
> 
> 
> Regarding the test hanging in Linux, some further debugging seem to 
> indicate that test hangs on `process::Clock::settle()`, because there's a 
> race + deadlock in `RateLimiter` that leaves a process stuck in `RUNNING`. 
> I'll dig deeper on Monday, but here's some evidence:
> 
> # Snippet from a successful test run that "leaks" a running process
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HealthCheckTest
> [ RUN  ] HealthCheckTest.DefaultExecutorCmdHealthCheck
> 
> [...]
> 
> E0127 11:18:06.607446  8665 limiter.hpp:123]  LIMITER _acquire
> E0127 11:18:06.607471  8665 limiter.hpp:129]  LIMITER There are 1 
> promises
> E0127 11:18:06.608064  8665 limiter.hpp:186]  LIMITER destroy
> 
>  DEADLOCK DETECTED! 
> You are waiting on process __limiter__(1419)@192.99.40.208:41947 that it 
> is currently executing.
> 
> [...]
> 
> [   OK ] HealthCheckTest.DefaultExecutorCmdHealthCheck (592 ms)
> [--] 1 test from HealthCheckTest (593 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (603 ms total)
> [  PASSED  ] 1 test.
> 
> Repeating all tests (iteration 474) . . .
> ```
> 
> # Snippet from a hung run
> 
> ```
> [...]
> E0127 11:18:06.878636  8640 cluster.cpp:358] !!! Settling the clock
> E0127 11:18:06.878646  8640 process.cpp:3491] !!! Attempting to acquire 
> mutex
> E0127 11:18:06.878654  8640 process.cpp:3494] !!! !runq.empty()?
> E0127 11:18:06.878659  8640 process.cpp:3502] !!! running.load() > 0?
> E0127 11:18:06.878667  8640 process.cpp:3504] !!! 1 processes still 
> running
> E0127 11:18:06.878676  8640 process.cpp:3509]  Process: 
> __authentication_router__(1) state: 3
> E0127 11:18:06.878684  8640 process.cpp:3509]  Process: 
> __basic_authenticator__(1893) state: 3
> E0127 11:18:06.878691  8640 process.cpp:3509]  Process: 
> __basic_authenticator__(1894) state: 3
> E0127 11:18:06.878697  8640 process.cpp:3509]  Process: 
> __basic_authenticator__(1895) state: 3
> E0127 11:18:06.878705  8640 process.cpp:3509]  Process: __gc__ state: 
> 3
> E0127 11:18:06.878711  8640 process.cpp:3509]  Process: 
> __limiter__(1419) state: 2
> E0127 11:18:06.878718  8640 process.cpp:3509]  Process: __processes__ 
> state: 3
> E0127 11:18:06.878725  8640 process.cpp:3509]  Process: __reaper__(1) 
> state: 3
> E0127 11:18:06.878731  8640 process.cpp:3509]  Process: 
> crammd5-authenticator(474) state: 3
> E0127 11:18:06.878738  8640 process.cpp:3509]  Process: files state: 3
> E0127 11:18:06.878744  8640 process.cpp:3509]  Process: help state: 3
> E0127 11:18:06.878751  8640 process.cpp:3509]  Process: 
> hierarchical-allocator(474) state: 3
> E0127 11:18:06.878757  8640 process.cpp:3509]  Process: 
> in-memory-storage(474) state: 3
> E0127 11:18:06.878764  8640 process.cpp:3509]  Process: 
> local-authorizer(947) state: 3
> E0127 11:18:06.878772  8640 process.cpp:3509]  Process: logging 
> state: 3
> E0127 11:18:06.878777  8640 process.cpp:3509]  Process: master state: 
> 3
> E0127 11:18:06.878792  8640 process.cpp:3509]  Process: metrics 
> state: 3
> E0127 11:18:06.878800  8640 process.cpp:3509]  Process: profiler 
> state: 3
> E0127 11:18:06.878806  8640

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-01-28 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [55900, 55901]

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 Jan. 28, 2017, 12:39 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Jan. 28, 2017, 12:39 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-01-28 Thread Gastón Kleiman 

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

(Updated Jan. 28, 2017, 12:39 p.m.)


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


Changes
---

Addressed Vinod's feedback.


Summary (updated)
-

Added support for command health checks to the default executor.


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


Repository: mesos


Description (updated)
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 
  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50 

Diff: https://reviews.apache.org/r/55901/diff/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

2017-01-28 Thread Gastón Kleiman 


> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 480
> > 
> >
> > launch nested container session returns a streaming response, how come 
> > you are calling `post()` helper here which expects a non-streaming response?
> > 
> > probably one of the reasons why your test is hanging.

The `post()` helper delegates to `process::http::request()`, which takes 
boolean flag (`streamedResponse`). If this flag is set to `false`, libprocess 
will convert a PIPE (streaming) response into a BODY (non-streaming) response. 
This means that the `Future` returned by the helper will not be completed until 
the server closes the connection.

Relevant links:
 - 
https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/include/process/http.hpp#L953-L955
 - 
https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/src/http.cpp#L1191-L1197
 - 
https://github.com/apache/mesos/blob/2d0195eed54feac41485fb1503dc4004e5500c81/3rdparty/libprocess/src/http.cpp#L1005-L1022

In these particular cases, it means that the `Future` will not be completed 
until the container exits, which is exactly what we need.


Regarding the test hanging in Linux, some further debugging seem to indicate 
that test hangs on `process::Clock::settle()`, because there's a race + 
deadlock in `RateLimiter` that leaves a process stuck in `RUNNING`. I'll dig 
deeper on Monday, but here's some evidence:

# Snippet from a successful test run that "leaks" a running process

```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HealthCheckTest
[ RUN  ] HealthCheckTest.DefaultExecutorCmdHealthCheck

[...]

E0127 11:18:06.607446  8665 limiter.hpp:123]  LIMITER _acquire
E0127 11:18:06.607471  8665 limiter.hpp:129]  LIMITER There are 1 promises
E0127 11:18:06.608064  8665 limiter.hpp:186]  LIMITER destroy

 DEADLOCK DETECTED! 
You are waiting on process __limiter__(1419)@192.99.40.208:41947 that it is 
currently executing.

[...]

[   OK ] HealthCheckTest.DefaultExecutorCmdHealthCheck (592 ms)
[--] 1 test from HealthCheckTest (593 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (603 ms total)
[  PASSED  ] 1 test.

Repeating all tests (iteration 474) . . .
```

# Snippet from a hung run

```
[...]
E0127 11:18:06.878636  8640 cluster.cpp:358] !!! Settling the clock
E0127 11:18:06.878646  8640 process.cpp:3491] !!! Attempting to acquire mutex
E0127 11:18:06.878654  8640 process.cpp:3494] !!! !runq.empty()?
E0127 11:18:06.878659  8640 process.cpp:3502] !!! running.load() > 0?
E0127 11:18:06.878667  8640 process.cpp:3504] !!! 1 processes still running
E0127 11:18:06.878676  8640 process.cpp:3509]  Process: 
__authentication_router__(1) state: 3
E0127 11:18:06.878684  8640 process.cpp:3509]  Process: 
__basic_authenticator__(1893) state: 3
E0127 11:18:06.878691  8640 process.cpp:3509]  Process: 
__basic_authenticator__(1894) state: 3
E0127 11:18:06.878697  8640 process.cpp:3509]  Process: 
__basic_authenticator__(1895) state: 3
E0127 11:18:06.878705  8640 process.cpp:3509]  Process: __gc__ state: 3
E0127 11:18:06.878711  8640 process.cpp:3509]  Process: __limiter__(1419) 
state: 2
E0127 11:18:06.878718  8640 process.cpp:3509]  Process: __processes__ 
state: 3
E0127 11:18:06.878725  8640 process.cpp:3509]  Process: __reaper__(1) 
state: 3
E0127 11:18:06.878731  8640 process.cpp:3509]  Process: 
crammd5-authenticator(474) state: 3
E0127 11:18:06.878738  8640 process.cpp:3509]  Process: files state: 3
E0127 11:18:06.878744  8640 process.cpp:3509]  Process: help state: 3
E0127 11:18:06.878751  8640 process.cpp:3509]  Process: 
hierarchical-allocator(474) state: 3
E0127 11:18:06.878757  8640 process.cpp:3509]  Process: 
in-memory-storage(474) state: 3
E0127 11:18:06.878764  8640 process.cpp:3509]  Process: 
local-authorizer(947) state: 3
E0127 11:18:06.878772  8640 process.cpp:3509]  Process: logging state: 3
E0127 11:18:06.878777  8640 process.cpp:3509]  Process: master state: 3
E0127 11:18:06.878792  8640 process.cpp:3509]  Process: metrics state: 3
E0127 11:18:06.878800  8640 process.cpp:3509]  Process: profiler state: 3
E0127 11:18:06.878806  8640 process.cpp:3509]  Process: registrar(474) 
state: 3
E0127 11:18:06.878813  8640 process.cpp:3509]  Process: 
standalone-master-detector(1420) state: 3
E0127 11:18:06.878820  8640 process.cpp:3509]  Process: system state: 3
E0127 11:18:06.878828  8640 process.cpp:3509]  Process: version state: 3
E0127 11:18:06.878834  8640 process.cpp:3509]  Process: whitelist(474) 
state: 3
E0127 11:18:06.878840  8640 process.cpp:3491] !!! Attempting to acquire mutex
E0127 11:18:06.878846  8640