-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
Support parsing signals based on container's platform #5931
Comments
@thaJeztah and @kzys |
I'm not really sure what makes sense to do here. I don't think it makes sense to implement something like this in moby/sys/signal but to implement it in containerd would mean that we'd need to hardcode signal values (since we couldn't use build constraints to use unix or windows official golang packages), which defeats the point of previous work #5693 @kzys. |
The current signal parsing logic is designed to be compatible with Docker. Especially SIGRTMIN handling, it is compatible with neither the host or the container, if they use musl libc.
Regarding LCOW, how much does containerd know the fact that LCOW run Linux VMs? Should it be handled by the shim or an OCI runtime (runhcs)? |
Afaik, it doesn't know that LCOW runs using a VM. However, it does know what platform the pod is running, which was part of my previous solution to parse based on the container's platform (see my closed linked PR above). Moving signal parsing to the shim or runtime would break with the current agreement between containerd and shims though, right? (side note: LCOW containers using containerd use runc as the runtime now 🙂) |
This one is tricky indeed. I know we used the string representation in Docker, so that a user can pass LCOW adds another "mismatch" in that respect, because the host platform doesn't match the container's platform. I know we had some discussions about that with LCOW in the Docker Engine (i.e. does the VM represent the container, or the process inside the VM?). I (personally) like having these translations happen as close to the actual container as possible, to keep some abstraction (I hope I'm able to phrase this in a meaningful way); basically (where possible) do not require "higher level" runtimes (containerd, docker engine) to be aware of platform specifics, and delegate that to (e.g.) runc (as it already will have to deal directly with the kernel). "higher level" runtimes can then generate a (somewhat) platform agnostic spec and assume that the right thing happens. Looks like the runtime spec also includes func parseSignal(rawSignal string) (unix.Signal, error) {
s, err := strconv.Atoi(rawSignal)
if err == nil {
return unix.Signal(s), nil
}
sig := strings.ToUpper(rawSignal)
if !strings.HasPrefix(sig, "SIG") {
sig = "SIG" + sig
}
signal := unix.SignalNum(sig)
if signal == 0 {
return -1, fmt.Errorf("unknown signal %q", rawSignal)
}
return signal, nil
} I must admit I'm not sure where these signals are used directly (in containerd), for example the Line 42 in a0ae24b
|
I agree, that seems like a reasonable mindset @thaJeztah. In particular, CRI attempts to parse signals before calling
Which I'm concerned about since even if we don't parse the signal to As for runc, we could update |
Yes, I'm wondering if it should be considered to make |
Is that something that the community would be okay with? |
Since syscall package is tied to the host platform, migrating to string or int makes sense for supporting cross-platform containers such as LCOW. But this may need to wait containerd 1.6.x or containerd 2.0.x. BTW StopSignal is a string in OCI Image Spec. Regarding LCOW, is the below diagram accurate? If so, would the shim responsible for Windows -> Linux signal conversion?
|
That's fine. We use a fork of containerd today so as long as the change is in upstream main we can just cherry pick it as needed until it's in a release.
@kzys Pretty much correct except that we use the same shim for LCOW and WCOW. Are you asking if today the shim is responsible for that conversion or suggesting to do that? The shim still runs on the host in the LCOW case but runc runs inside the linux VM. |
Would it be acceptable to add a new field on the task API's |
Arf. I wanted to reply to this. Yes, I was thinking along those lines as well; I think with that approach it should be possible to add this in a backward compatible way;
|
I'm going to go ahead and start working on that implementation then unless there are any objections from anyone |
Thanks! Works for me 👍 I see I didn't mention it in my reponses, but for completeness / consideration: I think If the above is implemented, perhaps we need a different name ( |
Let me summarize my thoughts. Here is what we have on LCOW
Option 1: Pass string signals from 1 to 3 Pros
Cons
Option 2: Pass string signals in 1. runhcs-shim converts that to integers since it knows the underlying platform while containers are Linux Pros
Cons
|
I think with the proposed change, they'd still be able to pass the signal as it is today (numeric value) and things continue to work.
Afaics, runc is already dealing with conversion of signal names to numbers (it gets the signal passed as a string (command line arguments are always strings), and accepts both names and numbers. The only difference would be that using "moby/signal" extends the list of known signals to add the SIGRTMIN signals (alternatively, we should consider having those added to golang.org/x/sys) |
moby/signal is slightly "opinionated" than runc or golang.org/x/sys regarding SIGRTMIN handling. The actual value is depending on libc (musl and glibc have the different numeric value). Pushing that to golang.org/x/sys may be hard. Because of that, I'm bit hesitant to have the logic in runc. runc is the reference implementation and other implementations may need to do what runc does to be compatible with, even the conversation logic is not covered by the OCI Runtime Spec. As a result, we may make the container world somewhat incompatible with musl-based Linux distros such as Alpine. While I don't think it would cause a huge production pain, I'm unsure that this is the right solution. |
That's the thing though. This is translating syscall numbers that the container will be handling. The container author knows best here, we just need to send a consistent value. |
Thanks. I've updated the pros/cons above to add the following;
Is this correct? It is bit different from what @thaJeztah said though.
We are going to clarify the conversion logic in the OCI Runtime Spec and other OCI runtimes would follow the change. The only difference is about SIGRTMIN handling and most containers wouldn't be affected though. |
I'm not sure I understand what you mean by this. How would option two allow us to standardize behavior through OCI?
Clarify in what way? |
Sorry, I put the statement in the wrong section. Option 1 allows/enforces us to standardize the behavior through OCI. Right now, OCI Runtime Spec describes the behavior like below;
But Linux manpages describes real-time signals like below; https://www.man7.org/linux/man-pages/man7/signal.7.html
Using moby/sys in runc means that we are going to do what the manpage doesn't recommend.
So I think this should be documented and standardized for other OCI runtimes. It clarifies the handling of real-time signals in the spec. |
Don't we already pass SIGRTMIN+n signals as integers to CRI? |
I'm unsure. containerd's CRI plugin is calling ParseSignal. 4 files (including one test) in containerd are calling the function so far. https://github.com/containerd/containerd/search?q=ParseSignal I think, we need to remove these if runc is responsible for the conversion. |
The only issue is this recommendation was made by glibc but doesn't handle the case where a glibc process sends a signal to a musl process. So if we wanted to handle this perfectly in runc we would need to try to detect which libc a program is using, which is going to be as close to impossible as you can get when you consider static linking as well as symbol stripping (maybe we could check whether it has I wonder how the LXC folks handle this... @brauner How do you handle the fact that SIGRTMIN is different based on the libc used by the process? Or is this something we've all collectively ignored because it's such a silly problem that nobody (myself included) thought to check whether this is an incompatibility that exists. |
LXC uses real time signals only for systemd, i.e. when the container runs systemd and no specific signal has been specified we will send the relevant real-time signal to systemd. Since systemd doesn't build with musl we don't have issues with libc differences. We do signal forwarding in e.g. LXD but it is restricted to well-defined signals such as SIGWINCH, SIGUSR{1,2}, SIGINT etc. not real-time signals. |
Because systemd's shutdown sequence is based on real-time signals I should add. |
Also note that |
The problem exists there too though in so far as it is possible that glibc inside and outside of a container could technically alter SIGRTMIN and differ. So one thing I talked to Lennart about was expressing it in terms of SIGRTMAX- instead of SIGRTMIN+ because presumably SIGRTMAX is stable but it's not perfect either. |
What is the problem you're trying to solve
Containers run that rely on containerd and containerd's packages may be running a platform different than the host. For example, linux containers on windows, which run using a lightweight linux virtual machine. When sending signals to a container, we should use signals for the container's platform, not the host's.
Describe the solution you'd like
We parse signals based on the container's platform.
Additional context
I previously made a PR to accomplish this here, but it fell off and eventually was closed. Recently, this PR was made to remove signal parsing in containerd in favor of using moby/sys platform parsing in here.
The text was updated successfully, but these errors were encountered: