-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[containerd] Support containerd v2.0.x #11845
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mzaian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I did not remove |
/ok-to-test |
I don't believe we should do this (differ from upstream in this way).
I read most of the containerd discussion; and the change seems correct to me. There is the question of systemd pre v240 where LimitNOfile would need an explicit value, but appart from that it seems sane.
|
Hi @mzaian, I do not recommend upgrading the default version of containerd to v2 for several reasons:
Given the magnitude of this change, I suggest that Kubespray should consider supporting both containerd v2 and v1 both, while keeping v1.x as the default. Once the upstream project transitions to v2, Kubespray can then update the default to v2. /hold (for more discuss) |
Hi @yankay As we align with the upstream Kubernetes. Let's keep this on-hold for a future Kubespray release then. |
- Upstream Dependency: The upstream Kubernetes project does not have any plans to migrate to containerd v2 at this time, as they are still relying on v1.x. You can check the details [here](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.32.md?plain=1#L618).
I don't think this is correct, kubernetes support containerd 2.0 since 1.30 [1]
I think the containerd releases prefixed with api are about the containerd grpc API [2] not containerd itself.
Defaulting to 2.0 2 kubernetes releases after it's supported does not seems that much of a problem IMO
[1]: https://github.com/containerd/containerd/blob/main/RELEASES.md#kubernetes-support
[2]: https://github.com/containerd/containerd/tree/api/v1.7.19/api
|
037b6e9
to
fba3798
Compare
fba3798
to
b730cf6
Compare
Thanks @VannTen /unhold |
Ok good then we can proceed and this will be out with the next release. So we have time to test and fix any issues when they occur. |
Yes @mzaian 🎉 There some bugs with containerd v2.0 and kubenretes , eg: |
So what about the LimitNOFile change ? Also, maybe we should use a action required release note and link to containerd 2.0 release ? |
I will add it again and add a release not for it. |
b730cf6
to
5c8dc3e
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.
Regarding the service file, should we template based on containerd version ?
Aka keep Limit* on 1.7 ?
@@ -30,7 +30,6 @@ RestartSec=5 | |||
# in the kernel. We recommend using cgroups to do container-local accounting. | |||
LimitNPROC={{ containerd_limit_proc_num }} | |||
LimitCORE={{ containerd_limit_core }} | |||
LimitNOFILE={{ containerd_limit_open_file_num }} |
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 think on systemd v240 we should use 1024:524288
as explained in containerd/containerd#8924 (comment)
(Rhel 8 and derivatives are on 239 if I believe Repology)
Not sure how to check for systemd version 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.
How about leaving this out of this PR and I can work separately in investigating this for all other supported operating systems.
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.
Hi @mzaian,
Would it be better to retain LimitNOFILE={{ containerd_limit_open_file_num }}
and adjust the default value of containerd_limit_open_file_num to 1048576, similar to what was done in kubernetes/kops#16329?
This way, users can still customize containerd_limit_open_file_num, and avoid to use infinity
Thank you!
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'd rather follow containerd on that. In particular, this means running a pod locally with an untweaked config (in podman or docker ) could have a different behavior than in a kubespray cluster (if running into the issues when the soft limit is high).
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'd still default to upstream in that case, for the reasons above.
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'm fine with both options? @yankay wdyt?
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.
From the release note of containerd 2.0 https://github.com/containerd/containerd/blob/main/docs/containerd-2.0.md#limitnofile-configuration-has-been-removed , it's a break change.
Administrators on platforms running versions less than systemd 240 should explicitly configure LimitNOFILE=1024:524288 or risk falling back to the kernel default of 4096.
The Kops's option is to keep it not break.
So my idea is to open a new issue #11918 to config the LimitNOFILE
, the kubespray can make the suggestion by the release note, like configuire it to LimitNOFILE=1024:524288
when the system version < 240.
Does it OK to merge the PR :-)
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.
That's not completely accurate regarding kops, since they change it from infinity to 1048576.
Again, I still thinks we should defer to upstream default (and systemd>240, for that matter) and mention it in a action required
note, linking in particular to the containerd discussion.
Regarding systemd version, since it's the default after >240, we could also use LimitNOFILE=1024:524288
on all systemd version, it simply won't have an effect.
For the release note, if we go that route, I would use something like :
action required
LimitNOFile is now 1024:524288 for containerd (upstream defaults). If you relied on a the previous `infinity` value, set `containerd_limit_open_file_num` to a reasonable value (1048576 is the value used by kops, for instance). In particular, somet For more details, see the containerd discussion <link to the PR on containerd> and in particular this comment https://github.com/containerd/containerd/pull/8924#issuecomment-1868165195
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.
That's not completely accurate regarding kops, since they change it from infinity to 1048576. Again, I still thinks we should defer to upstream default (and systemd>240, for that matter) and mention it in a
action required
note, linking in particular to the containerd discussion.Regarding systemd version, since it's the default after >240, we could also use
LimitNOFILE=1024:524288
on all systemd version, it simply won't have an effect.For the release note, if we go that route, I would use something like :
action required LimitNOFile is now 1024:524288 for containerd (upstream defaults). If you relied on a the previous `infinity` value, set `containerd_limit_open_file_num` to a reasonable value (1048576 is the value used by kops, for instance). In particular, somet For more details, see the containerd discussion <link to the PR on containerd> and in particular this comment https://github.com/containerd/containerd/pull/8924#issuecomment-1868165195
Hi @VannTen,
I agree that we should stay consistent with the upstream. In this PR, either removing LimitNOFILE
or setting LimitNOFILE=1048576
would be acceptable.
Then, I would like to submit another PR to automatically determine the systemd version to set this value, rather than relying solely on the release notes. #11918 What do you think?
roles/container-engine/containerd/templates/containerd.service.j2
Outdated
Show resolved
Hide resolved
I can do this but first I will check if they cherry-picked this change to 1.7.x |
5c8dc3e
to
4954219
Compare
Thanks @mzaian Does the PR need any more changes before we can merge it? 🙂 |
4954219
to
8ee2058
Compare
I just added v2.0.2 and the updated runc binary to v1.2.4 |
8ee2058
to
4c91901
Compare
4c91901
to
8a0ecd5
Compare
What type of PR is this?
/kind container-managers
/kind feature
What this PR does / why we need it:
Add hashes for containerd versions 2.0.[0-2]
containerd 2.0.0 release notes https://github.com/containerd/containerd/releases/tag/v2.0.0
containerd 2.0.1 release notes https://github.com/containerd/containerd/releases/tag/v2.0.1
containerd 2.0.2 release notes https://github.com/containerd/containerd/releases/tag/v2.0.2
Update runc binary to v1.2.4
Make containerd 2.0.2 default
Set containerd_limit_open_file_num to 1048576
Which issue(s) this PR fixes:
Fixes #11836
Does this PR introduce a user-facing change?: