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

Fix netns leak on container creation and exit code 1 on SIGTERM. #24082

Merged
merged 3 commits into from
Oct 1, 2024
Merged

Fix netns leak on container creation and exit code 1 on SIGTERM. #24082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2de82d5
libpod: ensure we are not killed during netns creation
Luap99 Sep 26, 2024
0bbef4b
libpod: rework shutdown handler flow
Luap99 Sep 26, 2024
5de7b7c
libpod: remove shutdown.Unregister()
Luap99 Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libpod/container_internal_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -67,6 +68,9 @@ func (c *Container) prepare() error {
tmpStateLock sync.Mutex
)

shutdown.Inhibit()
defer shutdown.Uninhibit()

wg.Add(2)

go func() {
Expand Down
5 changes: 0 additions & 5 deletions libpod/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
37 changes: 10 additions & 27 deletions libpod/shutdown/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -75,6 +83,7 @@ func Start() error {
}
handlerLock.Unlock()
shutdownInhibit.Unlock()
os.Exit(exitCode)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this prevent a lot of defer functions from running? I know I avoided it deliberately when I wrote this

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean yes but if we exit what do we want to defer here? And most importantly the current handler also just exited so there should not be any functional difference I would say

Copy link
Member

Choose a reason for hiding this comment

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

IIRC we have a bunch of things running in defer that do things like removing files to clean up after ourselves

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure but this PR doesn't change the behavior here. Critical sections where we cannot leak need to use shutdown.Inhibit() which is how I fixed the issue with the netns leak.

return
}
}()
Expand Down Expand Up @@ -131,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
}
8 changes: 7 additions & 1 deletion pkg/api/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down