----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51379/#review146949 -----------------------------------------------------------
Fix it, then Ship it! src/docker/executor.cpp (lines 474 - 475) <https://reviews.apache.org/r/51379/#comment213854> I think the original comment is fine. We care only about the mount namespace here. src/docker/executor.cpp (lines 514 - 515) <https://reviews.apache.org/r/51379/#comment213855> Add a comment: // Make sure HTTP and TCP health checks are run from the container's network namespace. src/health-check/health_checker.hpp (line 48) <https://reviews.apache.org/r/51379/#comment213863> s/specific// src/health-check/health_checker.hpp (line 51) <https://reviews.apache.org/r/51379/#comment213864> The executor UPID to which health check results will be reported. src/health-check/health_checker.hpp (line 52) <https://reviews.apache.org/r/51379/#comment213865> ... of *the* target task. src/health-check/health_checker.hpp (line 53) <https://reviews.apache.org/r/51379/#comment213866> The target task's pid used to enter the specified namespaces. src/health-check/health_checker.hpp (line 55) <https://reviews.apache.org/r/51379/#comment213868> The namespaces to enter prior performing a single health check. src/health-check/health_checker.hpp (line 57) <https://reviews.apache.org/r/51379/#comment213869> remove "specific". src/health-check/health_checker.cpp (line 80) <https://reviews.apache.org/r/51379/#comment213857> s/setNsClone/cloneWithsetns src/health-check/health_checker.cpp (line 85) <https://reviews.apache.org/r/51379/#comment213858> You can easily use `=` here, because we capture all variables available. src/health-check/health_checker.cpp (lines 89 - 97) <https://reviews.apache.org/r/51379/#comment213860> Do we need the `else` clause? I think we can flatten it because the `if` aborts. src/health-check/health_checker.cpp (line 90) <https://reviews.apache.org/r/51379/#comment213861> // This effectively aborts the health check. src/health-check/health_checker.cpp (line 95) <https://reviews.apache.org/r/51379/#comment213859> s/Enter/Entered src/health-check/health_checker.cpp (line 168) <https://reviews.apache.org/r/51379/#comment213862> We don't need to override `clone` if `namespaces` is empty, right? src/launcher/executor.cpp (lines 424 - 429) <https://reviews.apache.org/r/51379/#comment213856> How about: // Make sure command health checks are run from the task's mount // namespace. Otherwise if `rootfs` is specified the command binary // may not be available in the executor. // // NOTE: The command executor shares the network namespace // with its task, hence no need to enter it explicitly. - Alexander Rukletsov On Aug. 26, 2016, 1:17 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51379/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 1:17 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón > Kleiman, Gilbert Song, Jie Yu, and Timothy Chen. > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac > src/health-check/health_checker.hpp > b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 > src/health-check/health_checker.cpp > 45a5fe00a95a6e88b1990c1396e03082feb202bc > src/launcher/executor.cpp 71ede1ea4f4e97fe94bd2bd136f17f231cedbce6 > > Diff: https://reviews.apache.org/r/51379/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
