From 8de2e0ec213ef912dd794dffdf73329d1fbed629 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 30 Apr 2025 17:59:00 +0200 Subject: [PATCH 1/3] sigproxy: ignore if container already removed If the container is already removed do not log a warning as this happens in parallel so it is possible the container was already removed. The flake was shown in https://github.com/containers/podman/pull/26017. Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/terminal/sigproxy_commn.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/domain/infra/abi/terminal/sigproxy_commn.go b/pkg/domain/infra/abi/terminal/sigproxy_commn.go index df9aee00ad..705808d5e7 100644 --- a/pkg/domain/infra/abi/terminal/sigproxy_commn.go +++ b/pkg/domain/infra/abi/terminal/sigproxy_commn.go @@ -33,7 +33,8 @@ func ProxySignals(ctr *libpod.Container) { } if err := ctr.Kill(uint(syscallSignal)); err != nil { - if errors.Is(err, define.ErrCtrStateInvalid) { + // If the container is no longer running/removed do not log it as error. + if errors.Is(err, define.ErrCtrStateInvalid) || errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved) { logrus.Infof("Ceasing signal forwarding to container %s as it has stopped", ctr.ID()) } else { logrus.Errorf("forwarding signal %d to container %s: %v", s, ctr.ID(), err) From a4d00672492df8b7e773802fae5fc08365e0eb5c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 30 Apr 2025 18:10:14 +0200 Subject: [PATCH 2/3] pkg/signal: rework CatchAll() behavior Instead of catching all signals and then ignoring them inside the loop again just don't register them in Notify() to begin with. Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/terminal/sigproxy_commn.go | 3 --- pkg/domain/infra/tunnel/runtime.go | 3 --- pkg/signal/signal_common.go | 7 +++++-- pkg/signal/signal_linux.go | 4 ++-- pkg/signal/signal_linux_mipsx.go | 4 ++-- pkg/signal/signal_unix.go | 4 ++-- pkg/signal/signal_unsupported.go | 4 ++-- 7 files changed, 13 insertions(+), 16 deletions(-) diff --git a/pkg/domain/infra/abi/terminal/sigproxy_commn.go b/pkg/domain/infra/abi/terminal/sigproxy_commn.go index 705808d5e7..629b2e9c33 100644 --- a/pkg/domain/infra/abi/terminal/sigproxy_commn.go +++ b/pkg/domain/infra/abi/terminal/sigproxy_commn.go @@ -28,9 +28,6 @@ func ProxySignals(ctr *libpod.Container) { go func() { for s := range sigBuffer { syscallSignal := s.(syscall.Signal) - if signal.IsSignalIgnoredBySigProxy(syscallSignal) { - continue - } if err := ctr.Kill(uint(syscallSignal)); err != nil { // If the container is no longer running/removed do not log it as error. diff --git a/pkg/domain/infra/tunnel/runtime.go b/pkg/domain/infra/tunnel/runtime.go index 05f8b00787..2a8aa8b258 100644 --- a/pkg/domain/infra/tunnel/runtime.go +++ b/pkg/domain/infra/tunnel/runtime.go @@ -46,9 +46,6 @@ func remoteProxySignals(ctrID string, killFunc func(string) error) { go func() { for s := range sigBuffer { syscallSignal := s.(syscall.Signal) - if signal.IsSignalIgnoredBySigProxy(syscallSignal) { - continue - } signalName, err := signal.ParseSysSignalToName(syscallSignal) if err != nil { logrus.Infof("Ceasing signal %v forwarding to container %s as it has stopped: %s", s, ctrID, err) diff --git a/pkg/signal/signal_common.go b/pkg/signal/signal_common.go index 1061058291..34e5b494e5 100644 --- a/pkg/signal/signal_common.go +++ b/pkg/signal/signal_common.go @@ -46,11 +46,14 @@ func ParseSignalNameOrNumber(rawSignal string) (syscall.Signal, error) { return -1, fmt.Errorf("invalid signal: %s", basename) } -// CatchAll catches all signals and relays them to the specified channel. +// CatchAll catches all signals (except the ones that make no sense to handle/forward, +// see isSignalIgnoredBySigProxy()) and relays them to the specified channel. func CatchAll(sigc chan os.Signal) { handledSigs := make([]os.Signal, 0, len(SignalMap)) for _, s := range SignalMap { - handledSigs = append(handledSigs, s) + if !isSignalIgnoredBySigProxy(s) { + handledSigs = append(handledSigs, s) + } } signal.Notify(sigc, handledSigs...) } diff --git a/pkg/signal/signal_linux.go b/pkg/signal/signal_linux.go index 5b07ccdd59..0df442b93f 100644 --- a/pkg/signal/signal_linux.go +++ b/pkg/signal/signal_linux.go @@ -89,8 +89,8 @@ var SignalMap = map[string]syscall.Signal{ "RTMAX": sigrtmax, } -// IsSignalIgnoredBySigProxy determines whether sig-proxy should ignore syscall signal -func IsSignalIgnoredBySigProxy(s syscall.Signal) bool { +// isSignalIgnoredBySigProxy determines whether sig-proxy should ignore syscall signal +func isSignalIgnoredBySigProxy(s syscall.Signal) bool { // Ignore SIGCHLD and SIGPIPE - these are most likely intended for the podman command itself. // SIGURG was added because of golang 1.14 and its preemptive changes causing more signals to "show up". // https://github.com/containers/podman/issues/5483 diff --git a/pkg/signal/signal_linux_mipsx.go b/pkg/signal/signal_linux_mipsx.go index f587c09a36..852bdf3b39 100644 --- a/pkg/signal/signal_linux_mipsx.go +++ b/pkg/signal/signal_linux_mipsx.go @@ -89,8 +89,8 @@ var SignalMap = map[string]syscall.Signal{ "RTMAX": sigrtmax, } -// IsSignalIgnoredBySigProxy determines whether sig-proxy should ignore syscall signal -func IsSignalIgnoredBySigProxy(s syscall.Signal) bool { +// isSignalIgnoredBySigProxy determines whether sig-proxy should ignore syscall signal +func isSignalIgnoredBySigProxy(s syscall.Signal) bool { // Ignore SIGCHLD and SIGPIPE - these are most likely intended for the podman command itself. // SIGURG was added because of golang 1.14 and its preemptive changes causing more signals to "show up". // https://github.com/containers/podman/issues/5483 diff --git a/pkg/signal/signal_unix.go b/pkg/signal/signal_unix.go index 6e1ed540ea..d5d22268ab 100644 --- a/pkg/signal/signal_unix.go +++ b/pkg/signal/signal_unix.go @@ -87,8 +87,8 @@ var SignalMap = map[string]syscall.Signal{ "RTMAX": sigrtmax, } -// IsSignalIgnoredBySigProxy determines whether sig-proxy should ignore syscall signal -func IsSignalIgnoredBySigProxy(s syscall.Signal) bool { +// isSignalIgnoredBySigProxy determines whether sig-proxy should ignore syscall signal +func isSignalIgnoredBySigProxy(s syscall.Signal) bool { // Ignore SIGCHLD and SIGPIPE - these are most likely intended for the podman command itself. // SIGURG was added because of golang 1.14 and its preemptive changes causing more signals to "show up". // https://github.com/containers/podman/issues/5483 diff --git a/pkg/signal/signal_unsupported.go b/pkg/signal/signal_unsupported.go index 96c0c8f990..3c0e187ff0 100644 --- a/pkg/signal/signal_unsupported.go +++ b/pkg/signal/signal_unsupported.go @@ -87,8 +87,8 @@ var SignalMap = map[string]syscall.Signal{ "RTMAX": sigrtmax, } -// IsSignalIgnoredBySigProxy determines whether to sig-proxy should ignore syscall signal +// isSignalIgnoredBySigProxy determines whether to sig-proxy should ignore syscall signal // keep the container running or not. In unsupported OS this should not ignore any syscall signal. -func IsSignalIgnoredBySigProxy(s syscall.Signal) bool { +func isSignalIgnoredBySigProxy(_ syscall.Signal) bool { return false } From 941a6d0c054d865b151c6a549633b8eeb2b0c94f Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 30 Apr 2025 18:19:16 +0200 Subject: [PATCH 3/3] pkg/signal: ignore SIGTOP for signal proxy It makes no sense to forward it, SIGSTOP cannot be handled by userspace (like SIGKILL) and it didn't do anything before so this just makes it more explicit. Signed-off-by: Paul Holzinger --- pkg/signal/signal_linux.go | 8 +++++++- pkg/signal/signal_linux_mipsx.go | 8 +++++++- pkg/signal/signal_unix.go | 8 +++++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/pkg/signal/signal_linux.go b/pkg/signal/signal_linux.go index 0df442b93f..b0f3824979 100644 --- a/pkg/signal/signal_linux.go +++ b/pkg/signal/signal_linux.go @@ -94,5 +94,11 @@ func isSignalIgnoredBySigProxy(s syscall.Signal) bool { // Ignore SIGCHLD and SIGPIPE - these are most likely intended for the podman command itself. // SIGURG was added because of golang 1.14 and its preemptive changes causing more signals to "show up". // https://github.com/containers/podman/issues/5483 - return s == syscall.SIGCHLD || s == syscall.SIGPIPE || s == syscall.SIGURG + // SIGSTOP cannot be ignored/forwarded by userspace. + switch s { + case syscall.SIGCHLD, syscall.SIGPIPE, syscall.SIGURG, syscall.SIGSTOP: + return true + default: + return false + } } diff --git a/pkg/signal/signal_linux_mipsx.go b/pkg/signal/signal_linux_mipsx.go index 852bdf3b39..77492d57d0 100644 --- a/pkg/signal/signal_linux_mipsx.go +++ b/pkg/signal/signal_linux_mipsx.go @@ -94,5 +94,11 @@ func isSignalIgnoredBySigProxy(s syscall.Signal) bool { // Ignore SIGCHLD and SIGPIPE - these are most likely intended for the podman command itself. // SIGURG was added because of golang 1.14 and its preemptive changes causing more signals to "show up". // https://github.com/containers/podman/issues/5483 - return s == syscall.SIGCHLD || s == syscall.SIGPIPE || s == syscall.SIGURG + // SIGSTOP cannot be ignored/forwarded by userspace. + switch s { + case syscall.SIGCHLD, syscall.SIGPIPE, syscall.SIGURG, syscall.SIGSTOP: + return true + default: + return false + } } diff --git a/pkg/signal/signal_unix.go b/pkg/signal/signal_unix.go index d5d22268ab..88b0a55c04 100644 --- a/pkg/signal/signal_unix.go +++ b/pkg/signal/signal_unix.go @@ -92,5 +92,11 @@ func isSignalIgnoredBySigProxy(s syscall.Signal) bool { // Ignore SIGCHLD and SIGPIPE - these are most likely intended for the podman command itself. // SIGURG was added because of golang 1.14 and its preemptive changes causing more signals to "show up". // https://github.com/containers/podman/issues/5483 - return s == syscall.SIGCHLD || s == syscall.SIGPIPE || s == syscall.SIGURG + // SIGSTOP cannot be ignored/forwarded by userspace. + switch s { + case syscall.SIGCHLD, syscall.SIGPIPE, syscall.SIGURG, syscall.SIGSTOP: + return true + default: + return false + } }