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.

Sign up for GitHub

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

Jump to bottom

The removal of container root directory should be after poststop run successfully. #4363

Open
abel-von opened this issue Aug 1, 2024 · 4 comments · May be fixed by #4364
Open

The removal of container root directory should be after poststop run successfully. #4363

abel-von opened this issue Aug 1, 2024 · 4 comments · May be fixed by #4364

Comments

@abel-von
Copy link

abel-von commented Aug 1, 2024 •
edited
Loading

Description

Hi, We recently added a runc hook to manage our special devices, and ran into a problem.
If our post stop hook run failed(maybe because the resource is still not ready to be recycled), we expect the containerd or kubelet could retry runc delete and when the hook can return success, runc delete succeed, and all the resources will be recycled, with nothing residual.

but actually we found that when the second time containerd call runc delete, the container root directory is removed and a NotFound error returned, so that containerd will consider the container removed normally. but actually the post stop hook never be called successfully.

I am wondering maybe we can change the removal of root directory to after the hook called.

--- a/libcontainer/state_linux.go
+++ b/libcontainer/state_linux.go
@@ -54,13 +54,16 @@ func destroy(c *Container) error {
                        return fmt.Errorf("unable to remove container's IntelRDT group: %w", err)
                }
        }
+       c.initProcess = nil
+       if err := runPoststopHooks(c); err != nil {
+               return fmt.Errorf("unable to run post stop hooks: %w", err)
+       }
+       c.state = &stoppedState{c: c}
+
        if err := os.RemoveAll(c.stateDir); err != nil {
                return fmt.Errorf("unable to remove container state dir: %w", err)
        }
-       c.initProcess = nil
-       err := runPoststopHooks(c)
-       c.state = &stoppedState{c: c}
-       return err
+       return nil
 }

Steps to reproduce the issue

  1. write a runc hook to return error, and config it into runtime of containerd
  2. start a container
  3. delete a container

Describe the results you received and expected

expected:
deletion of the container should fail until the hook can be executed succeessfuly.
received:
container is removed, but the hook is never called with a success, so with resources residual.

What version of runc are you using?

The newest version

Host OS information

I think all os is effected.

Host kernel information

all kernel versions
@abel-von abel-von linked a pull request Aug 2, 2024 that will close this issue
Draft
@lifubang
Copy link
Member

lifubang commented Aug 2, 2024

In runtime-spec, it says:
The poststop hooks MUST be called after the container is deleted but before the delete operation returns.

Please see:
https://github.com/opencontainers/runtime-spec/blob/v1.2.0/config.md?plain=1#L622

@lifubang
Copy link
Member

lifubang commented Aug 2, 2024

So I think it's not a bug. If you want change this behavior, you should open a PR in runtime-spec first.

BTW, there is another bug for postStop hooks, do you have some interests to fix it:
The poststart hooks MUST be invoked by the runtime. If any poststart hook fails, the runtime MUST log a warning, but the remaining hooks and lifecycle continue as if the hook had succeeded.

Please see:
https://github.com/opencontainers/runtime-spec/blob/v1.2.0/runtime.md?plain=1#L73

But maybe the behavior would be redefined in the future, because there is a similar bug for poststart, you can see:
opencontainers/runtime-spec#1262 (review)

@abel-von
Copy link
Author

abel-von commented Aug 2, 2024 •
edited
Loading

The poststop hooks MUST be invoked by the runtime. If any poststop hook fails, the runtime MUST log a warning, but the remaining hooks and lifecycle continue as if the hook had succeeded.

This implies that all poststop hook can not fail, because even it is failed the platform can not do retry.

But maybe the behavior would be redefined in the future, because there is a similar bug for poststart, you can see:
opencontainers/runtime-spec#1262 (review)

Maybe we can make a correspond change to the poststop too.

@rata
Copy link
Member

rata commented Aug 2, 2024 •
edited
Loading

Steps to reproduce the issue

1. write a runc hook to return error, and config it into runtime of containerd

How do you do that?

Please make the steps easy to follow, provide as much info as possible so someone can just follow some steps, instead of search how to do X or Y. The whole point of the steps to repro is to avoid that.

I didn't know there was a way to ask containerd to set OCI hooks without an external tool (like a wrapper for the oci-runtime that adds the hooks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix post stop hook not always called successfully abel-von/runc
3 participants
@rata @lifubang @abel-von