Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a -w option to warn on reaping children #107

Merged
merged 1 commit into from
Feb 18, 2018

Conversation

zimbatm
Copy link
Contributor

@zimbatm zimbatm commented Jan 19, 2018

The common case for well designed software should be to not produce any
zombie processes.

This adds an option to warn in the logs when reaping of zombies is
happening so that it can be monitored as part of the software quality
lifecycle.

@zimbatm
Copy link
Contributor Author

zimbatm commented Jan 19, 2018

Implements my suggestion in #106. Arguable this should be the default but it could introduce issues in tools that process the container outputs.

src/tini.c Outdated
if (warn_on_reap > 0) {
PRINT_WARNING("Reaped child with pid: '%i'", current_pid);
} else {
PRINT_DEBUG("Reaped child with pid: '%i'", current_pid);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep the debug above so that it logs unconditionally (including for the "main child"), but have a PRINT_WARNING here dedicated to the case where the child is not the main child (i.e. this branch you're in).

We can also make the the warning more explicit. E.g. "Reaped orphaned process with pid: '%i'".

@krallin
Copy link
Owner

krallin commented Jan 19, 2018

Thanks! Can we get an integration test for this in https://github.com/krallin/tini/blob/master/test/run_inner_tests.py to protect against regressions down the line?

(I'm happy to help you with it / implement the test if you'd rather not do that yourself)

@zimbatm
Copy link
Contributor Author

zimbatm commented Jan 19, 2018

Sounds good. I will resume this work next week, I wanted to have feedback before investing more into this.

@krallin
Copy link
Owner

krallin commented Jan 19, 2018

Thanks!

@zimbatm zimbatm force-pushed the warn-on-reap branch 2 times, most recently from 696ea61 to acf4868 Compare January 20, 2018 14:24
@zimbatm
Copy link
Contributor Author

zimbatm commented Jan 20, 2018

@krallin updated the tests but I think I've got the test matrix wrong and two axis are interfering with each-other.

@zimbatm
Copy link
Contributor Author

zimbatm commented Jan 24, 2018

ping :)

@@ -158,6 +158,7 @@ def main():
Command(functional_base_cmd + ["-z"], fail_cmd).run(retcode=127 if args_disabled else 1)
Command(functional_base_cmd + ["-h"], fail_cmd).run(retcode=127 if args_disabled else 0)
Command(functional_base_cmd + ["zzzz"], fail_cmd).run(retcode=127)
Command(functional_base_cmd + ["-w"], fail_cmd).run(retcode=127)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be retcode=127 if args_disabled else 0: the MINIMAL=1 build does not accept arguments at all (it's the one Docker uses for their embedded version of Tini).

@krallin
Copy link
Owner

krallin commented Jan 24, 2018

Hey @zimbatm — I'm sorry, I'm a little delayed on this since I'm at a work event off-site this week (and beginning of next). I added a few comments for now regarding the failure you experienced.

@zimbatm
Copy link
Contributor Author

zimbatm commented Jan 25, 2018

no worries @krallin . Let me know what you think of the latest changes.

Well designed software should not produce any zombie or re-parenting
processes.

This adds an option to warn in the logs when reaping of zombies is
happening so that it can be monitored and fixed in subsequent releases
of the software.
@krallin krallin merged commit 3da8adc into krallin:master Feb 18, 2018
@zimbatm zimbatm deleted the warn-on-reap branch February 18, 2018 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants