libpod: Execute poststop hooks locally #864
Conversation
wking
force-pushed
the
local-post-delete-hooks
branch
from
21c986e to
af512d0
Compare
wking
force-pushed
the
local-post-delete-hooks
branch
from
af512d0 to
cc81973
Compare
|
#857 landed, so I've rebased this onto master. |
libpod/container_internal.go
Outdated
| @@ -552,6 +554,11 @@ func (c *Container) reinit(ctx context.Context) error { | |||
| if err := c.runtime.ociRuntime.deleteContainer(c); err != nil { | |||
There was a problem hiding this comment.
@wking It might be worth making a wrapper for this that calls deleteContainer() and then postDeleteHooks(), given that we want to always call postrun hooks deleting the container in runc.
wking
force-pushed
the
local-post-delete-hooks
branch
from
cc81973 to
ac567d6
Compare
|
I've pushed cc819730 -> ac567d6b with a rebase onto master and a |
|
Test failures are infra issues |
|
Rebase please |
Instead of delegating to the runtime, since some runtimes do not seem to handle these reliably [1]. [1]: containers#730 (comment) Signed-off-by: W. Trevor King <[email protected]>
wking
force-pushed
the
local-post-delete-hooks
branch
from
ac567d6 to
31dead9
Compare
Rebased with ac567d6b -> 31dead9 (no conflicts). |
|
@mheon PTAL |
|
I have a few concerns around ordering - do poststop hooks need to execute while the container is still mounted? If they do, we might want to move delete earlier in the process for container and pod removal. |
|
Yes I think poststop should be run while the container storage is intact. It looks like oci-systemd-hook is removing ROOTFS/etc/machine-id. (Not sure why this is necessary.) oci-register-machine unregistered (Sent a dbus message) the container with systemd-machinectl, but we are removing this hook. It causes much more problems then it is worth. oci-umount does not do poststop. I am a little surprised at how little we do in poststop. |
The spec requires poststop hooks to run after deletion, so I think the timing of these is correct as this PR stands. If you want hooks that fire before deletion, I think we need to use an extension stage. We've talked about a postexit stage to fire from |
|
Pk. Lets merge |
|
📌 Commit 31dead9 has been approved by |
|
@wking I'm mostly concerned about ordering relative to us unmounting the container's storage. I don't believe the OCI spec mandates when that will happen, and there may be hooks that assume that the container's root filesystem is still accessible at this stage. If so, we may want to move the deletion phase earlier in |
Right, it just talks about deleting resources allocated during
Why would you want to run a hook after |
|
@wking Fair enough - it does sound rather unreasonable to expect container resources to still exist after deletion. This does make things simpler for us, too. |
|
☀️ Test successful - status-papr |
github-actions
bot
added
the
locked - please file new issue/PR
Builds on #857, review that first.
Execute the poststop hooks ourselves instead of delegating to the runtime, since some runtimes do not seem to handle these reliably.
This may be enough for #730, but I'm not sure.
Also, while the
hookspackage is well-covered by unit tests and this pull requests adds unit tests forContainer.postDeleteHooks, we don't seem to have integration tests exercising the whole system. fdcf633 (#155) added a hook test, but itsfileutils.CopyFilecall has no error checking and (as far as I can see) no sourcehooks.jsonto copy. I expect we want to patch up that test, but I'm leaving it to follow-up (or parallel) work.