-
Notifications
You must be signed in to change notification settings - Fork 612
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 systemd in containers with podman-style --systemd
flag
#2785
Conversation
cmd/nerdctl/container_run.go
Outdated
@@ -184,6 +184,7 @@ func setCreateFlags(cmd *cobra.Command) { | |||
cmd.Flags().StringSlice("cap-drop", []string{}, "Drop Linux capabilities") | |||
cmd.RegisterFlagCompletionFunc("cap-drop", capShellComplete) | |||
cmd.Flags().Bool("privileged", false, "Give extended privileges to this container") | |||
cmd.Flags().String("systemd", "false", "Enable systemd integration (default: false)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This differs from the podman flag, which has a default value of true
@@ -173,7 +175,6 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa | |||
return nil, nil, err | |||
} | |||
cOpts = append(cOpts, restartOpts...) | |||
cOpts = append(cOpts, withStop(options.StopSignal, options.StopTimeout, ensuredImage)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/cmd/container/create.go
Outdated
systemdPaths := []string{ | ||
"/usr/sbin/init", | ||
"/sbin/init", | ||
"/usr/local/sbin/init", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any image that has /usr/local/sbin/init
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set these to be the same as podman
:
true enables systemd mode only when the command executed inside the container is systemd, /usr/sbin/init, /sbin/init or /usr/local/sbin/init.
(But, most images I have seen use /sbin/init
)
https://docs.podman.io/en/latest/markdown/podman-run.1.html#systemd-true-false-always
pkg/cmd/container/create.go
Outdated
|
||
systemdPaths := []string{ | ||
"/usr/sbin/init", | ||
"/sbin/init", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if /sbin
should have higher priority over /usr/sbin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, /sbin/init
is more common. I'll move higher
pkg/cmd/container/create.go
Outdated
|
||
// See: https://github.com/containers/podman/issues/15878 | ||
if !privilegedWithoutHostDevices { | ||
return nil, nil, errors.New("If --privileged is used with systemd `--security-opt privileged-without-host-devices` must also be used") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the podman
implementation all /dev/tty*
devices are unmounted to prevent causing host to crash
I could not find an easy to achieve this, so instead I return an error (also prevents causing host to crash). If that functionality is required, maybe it can be added in a future PR
pkg/cmd/container/create.go
Outdated
@@ -27,6 +27,7 @@ import ( | |||
"path" | |||
"path/filepath" | |||
"runtime" | |||
"slices" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work with Go 1.20.
I guess we can drop the support for Go 1.20 though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch to using or statement to maintain compatibility with 1.20
@@ -232,6 +232,8 @@ Security flags: | |||
- :whale: `--cap-add=<CAP>`: Add Linux capabilities | |||
- :whale: `--cap-drop=<CAP>`: Drop Linux capabilities | |||
- :whale: `--privileged`: Give extended privileges to this container | |||
- :nerd_face: `--systemd=(true|false|always)`: Enable systemd compatibility (default: false). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does always
differ from true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more to docs about the options and added a note to nerdctl
specific features in README:
https://github.com/containerd/nerdctl/pull/2785/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R206
{Type: "tmpfs", Source: "tmpfs", Destination: "/var/lib/journal"}, | ||
}), | ||
) | ||
stopSignal = "SIGRTMIN+3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIGTERM
causes restart in systemd
(This functionality is the same as podman)
See:
https://www.freedesktop.org/software/systemd/man/latest/systemd.html#Signals
Thanks,
|
} else if len(ensured.ImageConfig.Cmd) > 0 { | ||
entrypointPath = ensured.ImageConfig.Cmd[0] | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be an easier way to determine the entrypoint executable path, if there is I am open to updating
@@ -84,5 +84,6 @@ while [ $quit -ne 1 ]; do | |||
done | |||
echo "signal quit"`).AssertOK() | |||
base.Cmd("stop", testContainerName).AssertOK() | |||
base.Cmd("inspect", "--format", "{{json .Config.Labels}}", testContainerName).AssertOutContains("SIGQUIT") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw that this test was not checking container labels, so added assert
@@ -41,6 +41,7 @@ var ( | |||
DockerAuthImage = mirrorOf("cesanta/docker_auth:1.7") | |||
FluentdImage = mirrorOf("fluent/fluentd:v1.14-1") | |||
KuboImage = mirrorOf("ipfs/kubo:v0.16.0") | |||
SystemdImage = "ghcr.io/containerd/stargz-snapshotter:0.15.1-kind" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those this image since it has a working systemd and is controlled by containerd
5cd85bf
to
ed090fb
Compare
(with --systemd flag) Signed-off-by: Spencer von der Ohe <[email protected]>
@AkihiroSuda I have added some tests and squashed the commits. Could you please have another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
} | ||
|
||
opts = append(opts, | ||
oci.WithoutMounts("/sys/fs/cgroup"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sazzy4o I found your change while looking into supporting containers with systemd using k8s + containerd. I did the tmpfs mounts for /run, /tmp/, etc., but I was mounting the host's /sys/fs/cgroup as ready-only, which didn't work.
Here, you are removing the mount, which caught my attention. Is this so that systemd creates /sys/fs/cgroup when it initializes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfernandez Yes, this allow systemd to run inside the container and create /sys/fs/cgroup
This was based on the podman --systemd flag
Adds support for systemd to
nerdctl
with a--systemd
based on the flag used in podmanFixes #2784
Usage: