From 2de82d523b8b6104b978817b47ad3c06f42cf00f Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 26 Sep 2024 15:39:20 +0200 Subject: [PATCH 1/3] libpod: ensure we are not killed during netns creation When we are killed during netns setup it will leak the netns path as it was not commited in the db. This is rather common if you run systemctl stop on a podman systemd unit. Of course we cannot protect against SIGKILL but in systemd case we get SIGTERM and we really should not exit in a critical section like this. Fixes #24044 Signed-off-by: Paul Holzinger --- libpod/container_internal_linux.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index aaeb75d3cc..38119a604c 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -19,6 +19,7 @@ import ( "github.com/containers/common/pkg/cgroups" "github.com/containers/common/pkg/config" "github.com/containers/podman/v5/libpod/define" + "github.com/containers/podman/v5/libpod/shutdown" "github.com/containers/podman/v5/pkg/rootless" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" @@ -67,6 +68,9 @@ func (c *Container) prepare() error { tmpStateLock sync.Mutex ) + shutdown.Inhibit() + defer shutdown.Uninhibit() + wg.Add(2) go func() { From 0bbef4b830aedcd8a5c8d463e7fd0dc84d95c05d Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 26 Sep 2024 16:14:30 +0200 Subject: [PATCH 2/3] libpod: rework shutdown handler flow Currently podman run -d can exit 0 if we send SIGTERM during startup even though the contianer was never started. That just doesn't make any sense is horribly confusing for a external job manager like systemd. The original motivation was to exit 0 for the podman.service in commit ca7376bb11. That does make sense but it should only do so for the service and only if the server did indeed gracefully shutdown. So we rework how the exit logic works, do not let the handler perform the exit. Instead the shutdown package does the exit after all handlers are run, this solves the issue of ordering. Then we default to exit code 1 like we did before and allow the service exit handler to overwrite the exit code 0 in case of a graceful shutdown. Signed-off-by: Paul Holzinger --- libpod/runtime.go | 5 ----- libpod/shutdown/handler.go | 11 ++++++++++- pkg/api/server/server.go | 8 +++++++- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/libpod/runtime.go b/libpod/runtime.go index 73c607ad48..1aea7b4fe6 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -218,11 +218,6 @@ func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...R if runtime.store != nil { _, _ = runtime.store.Shutdown(false) } - // For `systemctl stop podman.service` support, exit code should be 0 - if sig == syscall.SIGTERM { - os.Exit(0) - } - os.Exit(1) return nil }); err != nil && !errors.Is(err, shutdown.ErrHandlerExists) { logrus.Errorf("Registering shutdown handler for libpod: %v", err) diff --git a/libpod/shutdown/handler.go b/libpod/shutdown/handler.go index d7fd5b5323..ac41899e0e 100644 --- a/libpod/shutdown/handler.go +++ b/libpod/shutdown/handler.go @@ -28,10 +28,18 @@ var ( shutdownInhibit sync.RWMutex logrus = logrusImport.WithField("PID", os.Getpid()) ErrNotStarted = errors.New("shutdown signal handler has not yet been started") + // exitCode used to exit once we are done with all signal handlers, by default 1 + exitCode = 1 ) +// SetExitCode when we exit after we ran all shutdown handlers, it should be positive. +func SetExitCode(i int) { + exitCode = i +} + // Start begins handling SIGTERM and SIGINT and will run the given on-signal -// handlers when one is called. This can be cancelled by calling Stop(). +// handlers when one is called and then exit with the exit code of 1 if not +// overwritten with SetExitCode(). This can be cancelled by calling Stop(). func Start() error { if sigChan != nil { // Already running, do nothing. @@ -75,6 +83,7 @@ func Start() error { } handlerLock.Unlock() shutdownInhibit.Unlock() + os.Exit(exitCode) return } }() diff --git a/pkg/api/server/server.go b/pkg/api/server/server.go index 4c07390d0d..41dff40cef 100644 --- a/pkg/api/server/server.go +++ b/pkg/api/server/server.go @@ -197,7 +197,13 @@ func (s *APIServer) Serve() error { s.setupPprof() if err := shutdown.Register("service", func(sig os.Signal) error { - return s.Shutdown(true) + err := s.Shutdown(true) + if err == nil { + // For `systemctl stop podman.service` support, exit code should be 0 + // but only if we did indeed gracefully shutdown + shutdown.SetExitCode(0) + } + return err }); err != nil { return err } From 5de7b7c3f379987faa9cb06a58be8914f2d56cbe Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 26 Sep 2024 16:20:56 +0200 Subject: [PATCH 3/3] libpod: remove shutdown.Unregister() It is never used and needed so let's just remove some dead code. Signed-off-by: Paul Holzinger --- libpod/shutdown/handler.go | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/libpod/shutdown/handler.go b/libpod/shutdown/handler.go index ac41899e0e..e06e818c7c 100644 --- a/libpod/shutdown/handler.go +++ b/libpod/shutdown/handler.go @@ -140,29 +140,3 @@ func Register(name string, handler func(os.Signal) error) error { return nil } - -// Unregister un-registers a given shutdown handler. -func Unregister(name string) error { - handlerLock.Lock() - defer handlerLock.Unlock() - - if handlers == nil { - return nil - } - - if _, ok := handlers[name]; !ok { - return nil - } - - delete(handlers, name) - - newOrder := []string{} - for _, checkName := range handlerOrder { - if checkName != name { - newOrder = append(newOrder, checkName) - } - } - handlerOrder = newOrder - - return nil -}