----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review163363 -----------------------------------------------------------
src/checks/health_checker.hpp (line 137) <https://reviews.apache.org/r/55901/#comment234769> we don't typically do `const` for POD types. s/_agentSpawnsCommandContainer/ src/checks/health_checker.cpp (line 129) <https://reviews.apache.org/r/55901/#comment234776> s/build/create/ src/checks/health_checker.cpp (lines 145 - 157) <https://reviews.apache.org/r/55901/#comment234773> can you inline these? src/checks/health_checker.cpp (lines 426 - 429) <https://reviews.apache.org/r/55901/#comment234777> why a `.repair()` here? src/checks/health_checker.cpp (line 435) <https://reviews.apache.org/r/55901/#comment234798> const & ? src/checks/health_checker.cpp (line 447) <https://reviews.apache.org/r/55901/#comment234780> s/this/This/ src/checks/health_checker.cpp (line 449) <https://reviews.apache.org/r/55901/#comment234781> s/Agent/agent/ src/checks/health_checker.cpp (line 480) <https://reviews.apache.org/r/55901/#comment234799> 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. src/checks/health_checker.cpp (lines 481 - 485) <https://reviews.apache.org/r/55901/#comment234782> The identation seems wrong? do we do it like this elsehwhere? i would've done so ``` .after(checkTimeout, defer(self(), ...) ``` src/checks/health_checker.cpp (line 531) <https://reviews.apache.org/r/55901/#comment234789> s/has not returned/timed out/ src/checks/health_checker.cpp (lines 534 - 536) <https://reviews.apache.org/r/55901/#comment234790> why repair? src/checks/health_checker.cpp (line 538) <https://reviews.apache.org/r/55901/#comment234797> why not just return failure? src/checks/health_checker.cpp (line 562) <https://reviews.apache.org/r/55901/#comment234793> s/callResponse/response/ src/launcher/default_executor.cpp (line 112) <https://reviews.apache.org/r/55901/#comment234794> s/resume/Resume/ src/launcher/default_executor.cpp (line 130) <https://reviews.apache.org/r/55901/#comment234795> s/stop/Stop/ - Vinod Kone On Jan. 25, 2017, 12:33 a.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55901/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2017, 12:33 a.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 > ------- > > [WIP] Added support for command health checks to the default executor. > > This is still a work in progress, but I am posting it in order to get some > feedback on the general approach. > > Open questions/issues: > > - The code that handles communication with the Agent is on the > `DefaultExecutor`, so the health checker calls `evolve` when serializing the > API calls. Should it use the v1 protos instead, and skip the `evolve` call? > - CMD health checks for command tasks inherit the env from the executor, > but allow overriding env variables. I don't think that inheriting the > executor env makes sense for cmd health checks launched by the > `DefaultExecutor` - each task can have its own env, so the health check > command should inherit the task env and merge it with the one in the check's > `CommandInfo`. > I added some code that takes the task env from `TaskInfo` and merges it > check's env, but I don't think that this is an ideal approach, since it will > miss env variables set/modified by containerizers/hooks/isolators. Do you > think that it would make sense for all Debug Containers to inherit the > environment of the parent container? > - There's a TODO for stopping/resuming health checking when the > `DefaultExecutor` is disconnected from the agent. After talking to AlexR, I > believe that we should do this for all types of checks, not just for `CMD`. > - The test that I introduced passes on Linux, but not on macOS, I still > have to do some more debugging. > > > Diffs > ----- > > src/checks/health_checker.hpp 6e558f2061a9e31157c47d31cb64b3a8568aace3 > src/checks/health_checker.cpp 50aa2858e807b27bbab58a3618f5200cfe4eca9e > src/launcher/default_executor.cpp a03794934adb93868734f8cf00b337a1bff9b5ab > src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269 > > 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 > >
