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

runtime: fail when a poststop hook fails #1265

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abel-von
Copy link

@abel-von abel-von commented Aug 2, 2024

If a poststop hook run failed(maybe because the resource is still not ready to be recycled), we expect that the delete operation could fail and be retried util all poststop hooks run succeed.

In the current runtime spec, when a poststop hook failed, runtime can not get the error, so that docker or kubernetes also can not get the error, this is easy to leave the resource residual.

I think this is similar to the situation of poststart hook. #1262

Also please refer to the PR in runc:
opencontainers/runc#4364

If a poststop hook run failed(maybe because the resource is still not ready to be recycled),
we expect that the delete operation could fail and be retried util all
poststop hooks run succeed.

Signed-off-by: Abel Feng <[email protected]>
@giuseppe
Copy link
Member

giuseppe commented Aug 5, 2024

after this change poststop hooks are now required to be idempotent, even existing ones, as they can be called multiple times. Won't existing hooks fail in most cases when called multiple times. Possibly they can misbehave and remove the wrong resources.

I wonder if this behavior should be configurable to not affect existing installations. We could have a flag for each hook to say whether a failure should be ignored or reported immediately (with the default being ignore as it is currently documented)

@kolyshkin
Copy link
Contributor

I'm against changing the current spec, since this adds a new requirement to existing hooks, as pointed out by @giuseppe above.

Re: retry logic, I think that if that is necessary, a hook can implement it internally, on its own.

Making it configurable, in my view, is introducing the unnecessary complexity, but if we really need runtime to fail when the poststop hook fails, the only way to do so is to add such an option (with the backward-compatible default of "not fail").

So, the question is: do we have some real use cases for this functionality? My guess is we don't.

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.

3 participants