container: delete the container on poststart hooks failures#2051
container: delete the container on poststart hooks failures#2051giuseppe wants to merge 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 enhances the container lifecycle management by introducing a critical cleanup mechanism. Previously, a container might remain in an inconsistent state if its 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
Activity
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}
| { | ||
| libcrun_error_t tmp_err = NULL; | ||
| container_delete_internal (context, def, id, true, true, &tmp_err); | ||
| crun_error_release (&tmp_err); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
The current implementation silently ignores potential errors from container_delete_internal. While the primary error from the hook failure is correctly propagated, it would be more robust to log a warning if the container deletion fails. This would provide better visibility for debugging cleanup issues without altering the main error handling flow.
{
libcrun_error_t tmp_err_storage = NULL;
libcrun_error_t *tmp_err = &tmp_err_storage;
if (container_delete_internal (context, def, id, true, true, tmp_err) < 0)
crun_error_write_warning_and_release (context->output_handler_arg, &tmp_err);
return ret;
}
jnovy
left a comment
jnovy
left a comment
There was a problem hiding this comment.
LGTM, just minor things to fix.
tests/test_hooks.py
Outdated
|
|
||
| try: | ||
| run_crun_command(["start", cid]) | ||
| logger.info("start succeeded but poststart hook should have failed") |
There was a problem hiding this comment.
Maybe logger.error would be more appropriate here?
tests/test_hooks.py
Outdated
|
|
||
| try: | ||
| run_crun_command(["state", cid]) | ||
| logger.info("container still exists after poststart hook failure") |
tests/test_hooks.py
Outdated
| return 0 | ||
|
|
||
| except Exception as e: | ||
| logger.info("test failed: %s", e) |
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
delete-container-poststart-failure
branch
from
6214f67 to
c4f3a89
Compare
|
@jnovy fair! Fixed now |
|
Needs a rebase (after merging #2045) |
3 participants
No description provided.