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

tests/int/hooks: fix failed hooks test #4352

Closed
wants to merge 1 commit into from

Conversation

kolyshkin
Copy link
Contributor

When reviewing PR #4348, I noticed that it removed the call to parent.terminate when a hook has failed. Yet, this test case did not catch that issue.

Add the check that the container was never fully started after a hook failure.

When reviewing PR 4348, I noticed that it removed the call to
parent.terminate when a hook has failed. Yet, this test case did
not catch that issue.

Add the check that the container was never fully started after a hook
failure.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin marked this pull request as ready for review July 18, 2024 14:50
Comment on lines +43 to +44
# Check the container was never started.
runc delete "test_hook-$hook"
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify here what happens that the container status is not 0 but can be started anyways? I mean clarify in the comment. As it doesn't seem intuitive :-)

Copy link
Member

Choose a reason for hiding this comment

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

ping @kolyshkin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I rechecked and this PR does not test the issue it's supposed to test.

IOW, with the terminate code removed, like this:

diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 6a91df01..c6883c56 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -360,9 +360,6 @@ func (c *Container) start(process *Process) (retErr error) {
                        }
 
                        if err := c.config.Hooks.Run(configs.Poststart, s); err != nil {
-                               if err := ignoreTerminateErrors(parent.terminate()); err != nil {
-                                       logrus.Warn(fmt.Errorf("error running poststart hook: %w", err))
-                               }
                                return err
                        }
                }

It does not fail. I'm not quite sure why, but obviously this PR is useless.

@lifubang
Copy link
Member

I noticed that it removed the call to parent.terminate when a hook has failed.

Without #4348 , another missing parent.terminate mentioned in #4355 :
https://github.com/opencontainers/runc/blob/v1.2.0-rc.2/libcontainer/container_linux.go#L357-L360

@kolyshkin kolyshkin closed this Sep 18, 2024
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.

4 participants