----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50540/#review144525 -----------------------------------------------------------
configure.ac (lines 615 - 616) <https://reviews.apache.org/r/50540/#comment210536> What are the implications of linking this in for people that have systemd but don't want to manage Mesos with systemd? Should this be a configure option? src/Makefile.am (line 2145) <https://reviews.apache.org/r/50540/#comment210537> Formatting should be 8 ts which I don't think this is. Use "set ts=8" in Vim. src/Makefile.am (line 2193) <https://reviews.apache.org/r/50540/#comment210538> Ditto, formatting. src/linux/systemd.hpp (lines 20 - 21) <https://reviews.apache.org/r/50540/#comment210539> We alphabetize includes so process.hpp comes before subprocess.hpp. See the style guide. src/linux/systemd.hpp (line 36) <https://reviews.apache.org/r/50540/#comment210540> const reference? Does the period need to change for different calls? Why not make it a static instance variable? src/linux/systemd.hpp (line 38) <https://reviews.apache.org/r/50540/#comment210541> Check style, I think it's to include comment on close of #ifdef, e.g., #endif // HAVE_LIBSYSTEMD src/linux/systemd.cpp (line 31) <https://reviews.apache.org/r/50540/#comment211366> Do you need this conditional compilation *within* the linux/systemd.cpp file, or is inclusion of the entire file conditional? src/linux/systemd.cpp (line 195) <https://reviews.apache.org/r/50540/#comment211367> ditto src/linux/systemd.cpp (line 196) <https://reviews.apache.org/r/50540/#comment211369> Is there any behavior defined if the variable is set but with no value? As opposed to being unset? src/linux/systemd.cpp (line 199) <https://reviews.apache.org/r/50540/#comment210545> use os::getenv() src/linux/systemd.cpp (line 200) <https://reviews.apache.org/r/50540/#comment210546> use numify<>() src/linux/systemd.cpp (line 201) <https://reviews.apache.org/r/50540/#comment210547> Watchdog* watchdog What's the lifetime of this object!? Who (and when) releases it? src/linux/systemd.cpp (line 212) <https://reviews.apache.org/r/50540/#comment211368> ditto src/linux/systemd.cpp (line 213) <https://reviews.apache.org/r/50540/#comment211370> This would be const& but see earlier. src/tests/linux/systemd_test_helper.cpp (lines 19 - 21) <https://reviews.apache.org/r/50540/#comment211393> split these src/tests/linux/systemd_test_helper.cpp (line 34) <https://reviews.apache.org/r/50540/#comment211373> Is this universally true across all distributions? src/tests/linux/systemd_test_helper.cpp (line 35) <https://reviews.apache.org/r/50540/#comment211372> Can't assume cgroups are mounted here. Tests use TEST_CGROUPS_HIERARCHY? src/tests/linux/systemd_tests.cpp (lines 17 - 22) <https://reviews.apache.org/r/50540/#comment211374> alphabetize please. src/tests/linux/systemd_tests.cpp (line 44) <https://reviews.apache.org/r/50540/#comment211375> Is there any way to create the service files outside of the system directory to avoid polluting it (especially if the test crashes)? And, systemd-test-helper is a pretty generic name... src/tests/linux/systemd_tests.cpp (line 45) <https://reviews.apache.org/r/50540/#comment211377> import this and std::cout etc. src/tests/linux/systemd_tests.cpp (line 47) <https://reviews.apache.org/r/50540/#comment211376> std::endl on preceeding line? src/tests/linux/systemd_tests.cpp (lines 65 - 68) <https://reviews.apache.org/r/50540/#comment211394> Use stout's os::write(path, message). src/tests/linux/systemd_tests.cpp (line 71) <https://reviews.apache.org/r/50540/#comment211384> Don't use informational logging, use assertions with logging: ASSERT(false) << "Ooops!"; src/tests/linux/systemd_tests.cpp (line 79) <https://reviews.apache.org/r/50540/#comment211385> ditto src/tests/linux/systemd_tests.cpp (line 87) <https://reviews.apache.org/r/50540/#comment211387> no, no! bad... what can you set up to wait on to determine when it's been started? src/tests/linux/systemd_tests.cpp (line 89) <https://reviews.apache.org/r/50540/#comment211379> Use path::join() src/tests/linux/systemd_tests.cpp (line 90) <https://reviews.apache.org/r/50540/#comment211383> ditto src/tests/linux/systemd_tests.cpp (lines 101 - 109) <https://reviews.apache.org/r/50540/#comment211388> Use os::read(tmpout) to get a Result<string> and avoid all of this. src/tests/linux/systemd_tests.cpp (lines 112 - 113) <https://reviews.apache.org/r/50540/#comment211389> Use stout's `bool strings::contains()` src/tests/linux/systemd_tests.cpp (line 116) <https://reviews.apache.org/r/50540/#comment211391> ditto src/tests/linux/systemd_tests.cpp (lines 122 - 123) <https://reviews.apache.org/r/50540/#comment211390> Does this validate that the service has actually been stopped or just that systemd accepted the stop command? src/tests/linux/systemd_tests.cpp (line 126) <https://reviews.apache.org/r/50540/#comment211392> What happens if there's an assertion during the test!? Because this is outside the test sandbox it'll leak. See earlier comment about writing and referencing this from the sandbox. - Ian Downes On July 27, 2016, 4:38 p.m., Lawrence Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50540/ > ----------------------------------------------------------- > > (Updated July 27, 2016, 4:38 p.m.) > > > Review request for mesos, David Robinson and Ian Downes. > > > Bugs: MESOS-5376 > https://issues.apache.org/jira/browse/MESOS-5376 > > > Repository: mesos > > > Description > ------- > > Add systemd watchdog support. > > > Diffs > ----- > > configure.ac d2136909b7305498ae901a5ea00133142b77f9e6 > src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 > src/linux/systemd.hpp 91134f1d4b100759e45931bd09ca4e1e1aeaaf8a > src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 > src/slave/main.cpp 4624392d30cf391015dcd63f447fe2414a47a16a > src/tests/linux/systemd_test_helper.hpp PRE-CREATION > src/tests/linux/systemd_test_helper.cpp PRE-CREATION > src/tests/linux/systemd_test_helper_main.cpp PRE-CREATION > src/tests/linux/systemd_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/50540/diff/ > > > Testing > ------- > > Tested by sending SIGSTOP to running mesos and verifying via journalctl that > it was killed by the watchdog. > > The test I wrote for this does the following: > - build up a unit file as a string and create a unit file in > /etc/systemd/system/systemd-test-helper.service > - reload the systemd daemon and start the newly discovered helper service > - wait a bit (30s) to make sure the watchdog has had a chance to kill the > service > - use systemctl status systemd-test-helper to check that the service is still > running > - clean up the unit file. > > TODO: create a similar test, but send a SIGSTOP to the service and ensure > that it has been killed by watchdog. > > > Thanks, > > Lawrence Wu > >
