fix(hooks): log warning when poststop hooks fail#2045
fix(hooks): log warning when poststop hooks fail#2045kolyshkin merged 1 commit intocontainers:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where poststop hook failures with positive exit codes were silently ignored. The change to check for ret != 0 instead of ret < 0 in src/libcrun/container.c is the right fix. The addition of new tests in tests/test_hooks.py effectively validates this new behavior. I've suggested a small refactoring in the test file to reduce code duplication and improve maintainability. Overall, this is a good fix.
tests/test_hooks.py
Outdated
| def test_poststop_hook_failure_warning(): | ||
| """Test that a failing poststop hook logs a warning but container still succeeds.""" | ||
| conf = base_config() | ||
| add_all_namespaces(conf) | ||
| conf['process']['args'] = ['/init', 'true'] | ||
|
|
||
| hook = {"path": "/bin/false"} | ||
| conf['hooks'] = {"poststop": [hook]} | ||
|
|
||
| try: | ||
| out, _ = run_and_get_output(conf) | ||
| except Exception as e: | ||
| logger.error("container failed unexpectedly: %s", e) | ||
| return -1 | ||
|
|
||
| if "poststop hook failed with exit code" not in out: | ||
| logger.error("expected warning about poststop hook failure, got: %s", out) | ||
| return -1 | ||
|
|
||
| return 0 | ||
|
|
||
|
|
||
| def test_multiple_poststop_hooks_failure(): | ||
| """Test that multiple failing poststop hooks do not prevent container cleanup.""" | ||
| conf = base_config() | ||
| add_all_namespaces(conf) | ||
| conf['process']['args'] = ['/init', 'true'] | ||
|
|
||
| hook1 = {"path": "/bin/false"} | ||
| hook2 = {"path": "/bin/false"} | ||
| conf['hooks'] = {"poststop": [hook1, hook2]} | ||
|
|
||
| try: | ||
| out, _ = run_and_get_output(conf) | ||
| except Exception as e: | ||
| logger.error("container failed unexpectedly: %s", e) | ||
| return -1 | ||
|
|
||
| if "poststop hook failed with exit code" not in out: | ||
| logger.error("expected warning about poststop hook failure, got: %s", out) | ||
| return -1 | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
These two test functions, test_poststop_hook_failure_warning and test_multiple_poststop_hooks_failure, are nearly identical, leading to code duplication. To improve maintainability, you can extract the common logic into a helper function. This will make the tests cleaner and easier to manage in the future.
def _test_failing_poststop_hooks(hooks):
"""Helper for testing that failing poststop hooks log a warning but the container succeeds."""
conf = base_config()
add_all_namespaces(conf)
conf['process']['args'] = ['/init', 'true']
conf['hooks'] = {"poststop": hooks}
try:
out, _ = run_and_get_output(conf)
except Exception as e:
logger.error("container failed unexpectedly: %s", e)
return -1
if "poststop hook failed with exit code" not in out:
logger.error("expected warning about poststop hook failure, got: %s", out)
return -1
return 0
def test_poststop_hook_failure_warning():
"""Test that a failing poststop hook logs a warning but container still succeeds."""
return _test_failing_poststop_hooks([{"path": "/bin/false"}])
def test_multiple_poststop_hooks_failure():
"""Test that multiple failing poststop hooks do not prevent container cleanup."""
return _test_failing_poststop_hooks([{"path": "/bin/false"}, {"path": "/bin/false"}])
|
TMT tests failed. @containers/packit-build please check. |
|
@eriksjolund PTAL |
src/libcrun/container.c
Outdated
| suppressed at default verbosity. Ensure the warning is | ||
| always visible through crun_error_write_warning_and_release. */ | ||
| if (ret > 0) | ||
| crun_make_error (err, 0, "poststop hook failed with exit code: %d", ret); |
There was a problem hiding this comment.
we set the error only when the function is also reporting the failure.
If we want to set err, then we need to return a negative value
There was a problem hiding this comment.
Another thing: If there are multiple poststop hooks, they are not treated the same because keep_going is true. The last hook is treated differently as it influences the return value from do_hooks().
See
Lines 827 to 833 in c07aadc
jnovy
force-pushed
the
fix/issue-1946
branch
2 times, most recently
from
4792bbd to
49f6e1f
Compare
src/libcrun/container.c
Outdated
| FILE *out = context->output_handler_arg; | ||
| if (out == NULL) | ||
| out = stderr; | ||
| fprintf (out, "poststop hook failed with exit code: %d\n", ret); |
There was a problem hiding this comment.
please use libcrun_warning here (or libcrun_error)
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
it misses the |
When do_hooks() returns a positive exit code (hook process exited non-zero), the error object is not set, so the existing check for ret < 0 silently ignores the failure. Change the condition to ret != 0 and, for positive return values, directly write the warning message to the output stream without using crun_make_error(), which would violate the convention that err is only set when the function returns a negative value. For ret < 0 (internal errors where err is already set by do_hooks), the existing crun_error_write_warning_and_release() path is preserved. Add two tests verifying that single and multiple failing poststop hooks produce the expected warning message. Fixes: containers#1946 Signed-off-by: Jindrich Novy <jnovy@redhat.com>
|
@eriksjolund While it's true that if (keep_going)
libcrun_warning ("error executing hook `%s` (exit code: %d)", hooks[i]->path, ret);So no failure is silently lost — every failing hook produces a visible warning at the time it fails. This also aligns with the OCI runtime spec (lifecycle step 13):
The current implementation satisfies both requirements: warnings are emitted per-hook, and execution continues. The final return value being from the last hook is acceptable because the caller ( |
4 participants
Poststop hook errors were silently ignored when the hook returned a positive exit code. The error check only caught negative return values, missing the case where hooks fail with a non-zero exit status.
Change the condition from ret < 0 to ret != 0 and create a proper error message for positive exit codes so crun_error_write_warning_and_release has something to print. Note that do_hooks() also logs via libcrun_warning() but that is suppressed at default verbosity, so the explicit crun_error_write_warning_and_release call ensures the warning is always visible.
Fixes: #1946