Request - containerd to support SIGRTMIN+3 stopsignal

[axelgMS]

What is the problem you're trying to solve

Looks like containerd runtime doesn't support a SIGRTMIN+3 stopsignal ?

Seeing the error below in an AKS cluster after upgrade to AKS 1.19.x (which switch from docker/moby to containerd runtime):

E0421 12:00:00.000 4252 pod_workers.go:191] Error syncing pod XYZ ("mypod(XYZ)"), skipping: failed to "KillContainer" for "ABC" with KillContainerError: "rpc error: code = Unknown desc = failed to parse stop signal "SIGRTMIN+3": **unknown signal "SIGRTMIN+3""**

Describe the solution you'd like

I'd like containerd to handle this type of stopsignal, just like docker does.

[kzys]

containerd's CRI plugin is handling the signal.

containerd/pkg/cri/server/container_stop.go

Line 131 in 261c107

return errors.Wrapf(err, "failed to parse stop signal %q", stopSignal)

@mikebrow @Random-Liu Should we forward the signal to the container as is?

[estesp]

I think it still requires understanding those signal "names"; moby project does that with a special package that manages the signal map (since Go's x/sys does not) with the rtmin/rtmax additions: https://github.com/moby/moby/blob/master/pkg/signal/signal_linux.go

[kzys]

We have a similar map for Windows and RT signals will be added by #4793.

containerd/signals_windows.go

Line 28 in 7b7a230

var signalMap = map[string]syscall.Signal{

[thaJeztah]

I think it still requires understanding those signal "names"; moby project does that with a special package that manages the signal map (since Go's x/sys does not) with the rtmin/rtmax additions: https://github.com/moby/moby/blob/master/pkg/signal/signal_linux.go

If useful, happy to migrate that package to either github.com/moby/sys (or to this repository)

[zhsj]

I think it's much better to forward this SIGRTMIN map to golang.org/x/unix, since it's really fragile to maintain such list for various architectures.

[zhsj]

The history behind the current code in containerd, is from moby/moby#37491 and golang/go#28027. Then the old hard coded map is replaced in #3063 to enable building on mipsx. And runc is also affected, opencontainers/runc#2257

Currently I think the workaround is to use the number like 37 for SIGRTMIN+3.

[zhsj]

Just have a look at the detail of SIGRTMIN, the vaule depends on the implementation detail of libc. Glibc and musl-c have different values. For glibc, it's 34, which is moby currently hard coding. However musl-c, is 35, ref https://git.musl-libc.org/cgit/musl/tree/src/signal/sigrtmin.c.

One the manpage of https://man7.org/linux/man-pages/man7/signal.7.html, it also says

... and adjusts the value of SIGRTMIN suitably (to 34 or 35) ...

Probably people only use SIGRTMIN for systemd in container, and Alpine doesn't support systemd, so nobody notice the SIGRTMIN value is wrong for Alpine container.

[punedhir]

Experiencing the same behaviour. I believe we were implementing the workaround by using the number like 37 for SIGRTMIN+3.
Hope there's a fix soon.

[cpuguy83]

@zhsj In the case of alpine, wouldn't the image builder just need to bump the RTMIN+<N> to make it work for their own container?

There is no (good) way for containerd to know which signal to send based on glibc/musl so we need to pick one and document the decision. It is up to the image authors to make sure it works.
Standardizing on these signal values maybe should be in the image spec so all OCI compliant runtimes handle this the same way.... but we shouldn't hold that up here.
Practically speaking this has been handled in a specific way in Docker and breaking that is going to be (already is) problematic.

[zhsj]

What I wrote above is to explain to myself why these signals can't be defined in x/sys. For docker or containerd I think just hard code RTMAX RTMIN is enough, no need to bring back the whole signal map again. And better a warning in document to the non glibc users.

[kzys]

It is okay to use cgo to have SIGRTMIN? @AkihiroSuda is removing btrfs's cgo dependency though. Alternatively we could have a configuration variable either build-time or runtime.

[AkihiroSuda]

I don't think we should have cgo just for a single constant 😄

Build-time variable sgtm.