mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ian Downes <ian.dow...@gmail.com>
Subject Re: Review Request 50540: Add systemd watchdog support.
Date Tue, 09 Aug 2016 18:33:28 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message