-
Notifications
You must be signed in to change notification settings - Fork 40
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
sysext: add containerd recipe #97
Conversation
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.
@tormath1 Thanks! Left some comments, but all nits. Feel free to ignore them
create_containerd_sysext.sh
Outdated
|
||
# Download containerd | ||
rm -f "containerd-${VERSION}.tgz" | ||
curl -o "containerd-${VERSION}.tgz" -fsSL "https://github.com/containerd/containerd/releases/download/v${VERSION}/containerd-static-${VERSION}-linux-${ARCH}.tar.gz" |
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.
checking the sha256 that is also available in the github repo would be great
[plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options] | ||
SystemdCgroup = true | ||
EOF | ||
sed 's/SystemdCgroup = true/SystemdCgroup = false/g' "${SYSEXTNAME}/usr/share/containerd/config.toml" > "${SYSEXTNAME}/usr/share/containerd/config-cgroupfs.toml" |
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.
systemd-cgroup is the default almost everywhere, any reason to change this? Most/All k8s e2e test run with systemd-cgroup too IIRC.
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.
We actually create a new configuration: config-cgroupfs.toml
for folks still running on cgroups v1. Even if it's not common, this is still a sceneario supported and tested on Flatcar.
By default containerd will use the config.toml
which uses cgroups v2.
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.
Ohhh, makes sense!
create_containerd_sysext.sh
Outdated
{ echo "[Unit]"; echo "Upholds=containerd.service"; } > "${SYSEXTNAME}/usr/lib/systemd/system/multi-user.target.d/10-containerd-service.conf" | ||
mkdir -p "${SYSEXTNAME}/usr/share/containerd" | ||
cat > "${SYSEXTNAME}/usr/share/containerd/config.toml" <<-'EOF' | ||
version = 2 |
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 config works fine, containerd will migrate it to the version 3 format on startup. But we can update it to the version 3 format at some point too.
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.
Ok, I was wondering too if the configuration was still accurate. I can migrate it to version 3 directly - fresh start!
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.
Awesome!
LimitNOFILE=1048576 | ||
LimitNPROC=infinity | ||
LimitCORE=infinity | ||
TasksMax=infinity |
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 were some notes on the release about these settings and some change. I don't remember now, but it might be relevant to check. IIRC, though, it was that it wasn't included by default anymore or something like that.
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.
Correct:
Remove LimitNOFILE from containerd.service (containerd/containerd#8924)
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!
e183d5e
to
dc704fe
Compare
Thanks @rata for the review - your comments have been addressed here: https://github.com/flatcar/sysext-bakery/compare/e183d5ea1b3b524e07555b87e5da7a7f03028fe7..dc704fe9f1bfc2bd7adfab9e6a64421e7aed8a17 |
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.
LGTM. Just nits, again, feel free to ignore them :)
create_containerd_sysext.sh
Outdated
rmdir "${SYSEXTNAME}"/bin | ||
runc_version=$(curl -fsSL "https://raw.githubusercontent.com/containerd/containerd/refs/tags/v${VERSION}/script/setup/runc-version") | ||
echo "Downloading associated runc version: ${runc_version}" | ||
curl -o "${SYSEXTNAME}/usr/bin/runc" -fsSL "https://github.com/opencontainers/runc/releases/download/${runc_version}/runc.${ARCH}" |
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.
Sorry I didn't say this before, but maybe use the sha here too?
runc has a sha file with the sha for all the binaries, so something like this should work:
$ sha256sum --ignore-missing -c runc.sha256sum
runc.amd64: OK
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.
Ah yes forgot about runc - I was not fully awake :D
create_containerd_sysext.sh
Outdated
# set containerd's OOM score | ||
oom_score = -999 | ||
[plugins."io.containerd.cri.v1.runtime".containerd.runtimes.runc] | ||
# setting runc.options unsets parent settings |
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.
Not sure what you mean with this, but maybe this has changed now? containerd/containerd#9982 Config changes are merged instead of override?
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 is the current configuration (shipped in Flatcar) - I am not sure what it means but anyway we can drop this whole section as it's a no-op.
create_containerd_sysext.sh
Outdated
oom_score = -999 | ||
[plugins."io.containerd.cri.v1.runtime".containerd.runtimes.runc] | ||
# setting runc.options unsets parent settings | ||
runtime_type = "io.containerd.runc.v2" |
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 is a no-op now, as the v1 have been removed: https://github.com/containerd/containerd/pull/8262/files
But maybe you want to keep it, both are fine :)
dc704fe
to
331dbbc
Compare
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.
LGTM
Nice idea to use the containerd repo on this tag to check the runc version to use!
this build containerd + runc sysext Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
331dbbc
to
be0c6b7
Compare
this build containerd + runc sysext
This can be useful for users interested to test containerd 2.0 with user namespace support for Kubernetes
Tested on Kubernetes Flatcar cluster with usernamespace feature: