[polarathene]

This is the equivalent of moby/moby#45534 (see for additional insights and review discussion), and replaces an earlier PR attempt: #7566 (original author is not responding to feedback)

Closes: #7566

---

Remove LimitNOFILE from containerd.service to rely on the systemd v240 implicit default of 1024:524288. On supported platforms with systemd prior to v240, packagers are expected to patch the service with an explicit LimitNOFILE=1024:524288.

• 1024 soft limit is an implicit default, avoiding unexpected breakage. Software that needs a higher limit should request to raise the soft limit for its process.
• 524288 hard limit is an implicit default since systemd v240 and is adequate for most processes (half of the historical limit from fs.nr_open of 1048576), while 4096 is the implicit default from the kernel (often too low).
• The hard limit may not exceed fs.nr_open (which a value of infinity will resolve to). On most systems with systemd v240 or newer, this will resolve to an excessive size of 2^30 (over 1 billion).
• When set to infinity (usually as the soft limit) software may experience significantly increased resource usage, resulting in a performance regression or runtime failures that are difficult to troubleshoot.

[k8s-ci-robot]

Hi @polarathene. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dmcgowan]

Instead of just removing the line, should their be a comment for cases it might still be need or used?

[polarathene]

should their be a comment for cases it might still be need or used?

For the equivalent PR at moby, it was requested to drop the line completely without leaving any additional context. I brought up some points where it could be beneficial to keep some context around.

If maintainers of containerd have a different opinion, let me know and I'll adjust the commit 👍

---

Additional Information for LimitNOFILE / RLIMIT_NOFILE / ulimit -n

• LimitNOFILE has a sane default of 1024:524288 since systemd v240. systemd maintainers looked into what the majority of software required and settled on an appropriate hard limit, while the soft limit is kept for legacy/compatibility reasons (often the soft limit being too high caused various software to misbehave).
• Supported platforms with systemd earlier than v240 should still use an explicit LimitNOFILE=1024:524288. Not doing so is likely to fallback to kernel hard limit default of 4096.
• Some distros like Debian with systemd v240+ opt-out via a patch from systemd increasing fs.nr_open beyond the default 1048576 which reduces the concern of using infinity (but you really shouldn't have the soft limit set to infinity).

Setting a higher limit: It's incredibly unlikely that anyone needs to adjust the hard limit explicitly beyond 524288 except for systems where the need is obvious, and the sysadmin would have the expertise to already know of this setting and all the others that can be required to overcome various bottlenecks at scale. I looked into this extensively and I'm rather confident with all the supporting information I gathered to support that statement 😅

Setting a lower limit: Likewise, there really shouldn't be a need to lower the hard limit or adjust the soft limit (containerd will raise this implicitly to the hard limit as a feature of Go since Aug 2022, child processes will lower the soft limit back). In the event there is, it's behaving differently than you'd experience without running the process in a container, so knowledge of what adjustments are required should be covered by that software's own documentation.

[metux]

Since such files are likely to be modified by distros (and only apply to those using systemd at all), I wonder whether we should carry this file at all. IMHO it really belongs into the distro domain

[polarathene]

I wonder whether we should carry this file at all. IMHO it really belongs into the distro domain

The majority of linux distros use systemd, the project should ship a standardized config to avoid problems like the current one this PR is resolving from being repeated across each individual package maintainer.

I've seen that stance and problems with other software like the vmware/open-vm-tools project. It'd just cause more problems if you decide to drop the config entirely.

---

I'm not aware of any distros that are modifying this config, do you have any in mind? You will find various third-party advice online to modify the config directly or apply an override to workaround this exact problem, but often the solution is incorrect and not understood well, it just minimized the issue for them.

[metux]

The majority of linux distros use systemd,

DistroWatch currently lists 322 non-systemd distros. Including those specifically made as container host (eg. Rancher).

I've seen that stance and problems with other software like the vmware/open-vm-tools project.
It'd just cause more problems if you decide to drop the config entirely.

There's lots of other hackish or broken stuff in there (even guessing host arch wrong, etc), but besides,
which problems exactly ? There's much distro specific stuff in here - that's why eg. deb's shipping its entirely own version.

Certainly, it would be better if we had a way to generate these files from some more high level data source, where
upstream can define the common things and distros easily inject their specifics - but haven't seen such thing yet.
(personally, I won't spend any second on that, since I'm never using systemd anyways)

I'm not aware of any distros that are modifying this config, do you have any in mind?

Practically all have to, at least for the broken ExecStart line:
/usr/local is completely wrong for distro packages - that's reserved for locally installed (means: manually compiled by the operator - outside the package manager's realm).

And the explicit modprobe'ing is
a) only relevant if one's using it at all
b) only working it the kernel has it at all (sub-case of a) and running w/ full host privileges
c) obsolete since kmod handles it itself (and only useful on systems that somehow managed to break kmod)

[cpuguy83]

Instead of just removing the line, should their be a comment for cases it might still be need or used?

@dmcgowan

I don't think so. In cases where it is needed it can be trivially patched in, either by patching the actual file when building a package or via systemd drop-in at runtime.
In most places this line does nothing useful, in some places (growing number) the line is actively harmful.

@metux Its still useful to users not running from distro packages.

[polarathene]

DistroWatch currently lists 322 non-systemd distros. Including those specifically made as container host (eg. Rancher).

186 with systemd vs 66 non-systemd active linux distros (queried via DistroWatch advanced search):

• There is some overlap, such as both listing MX Linux.
• Popularity ranked, the first non-systemd distro after MX Linux is antiX (rank 15). Within the top 10 there are a few familiar names like Alpine (26), Devuan (42) and Slackware (44), even Puppy (rank 17, I used near 20 years ago).
• Contrast to systemd distros popularity rank, Mint (2), EndeavourOS (3) Debian (4), Manjaro (5), Ubuntu (6), Pop!_OS (7), Fedora (8), OpenSUSE (9). This is just the top 10, plenty more recognizable names to many Linux users than there would be the non-systemd.

Regardless of how you came across your 322 metric, I'd assume the number with systemd is larger (if we don't limit to Linux and Active filters, the result is 596 distros), and having over 50% does count as the majority. Although when I made that statement, it was with more pragmatic intentions of well-known distros, especially those you'd generally encounter as a Docker host.

Certainly, it would be better if we had a way to generate these files from some more high level data source, where
upstream can define the common things and distros easily inject their specifics - but haven't seen such thing yet.

It's fine as a reference. Usually changes can be applied by the packager with a variety of options, including natively with systemd drop-ins.

which problems exactly?

Sorry, by modifying the config I was specifically referring to the context of the PR LimitNOFILE, not so much anything else in containerd.service. I'll share the open-vm-tools problem I experienced as an issue when this kind of config is not maintained by the project when it's a good case of when it should be (or at the very least provide reference to official documentation / advice). I'll then wrap up with why it's relevant in the context of this PR for containerd for those same concerns.

In VMware open-vm-tools project, they have packagers manage the init scripts and the quality varies even among popular distros. You'll find the open-vm-tools repo full of bug reports with many actually being related to this as some packagers aren't doing what's required or have race conditions. Some of that has been introduced over time (such as KDE adopting systemd to manage xdg autostart), but those problems existed without getting resolved by the distros packagers for 1-2 years IIRC.

It's far better to have a central reference, especially when changes occur that require that init script to be modified. Each downstream distro packager needs to become aware of the change and resolve it (potentially from the user report providing a solution that may have already been handled by another packager prior), many times you'll get a report though and none of downstream has time to investigate and resolve it.

In this case I investigated the problem for open-vm-tools, after many users were chiming in for over a year about the issue. I provided solutions but did not see any distro packager adopt them (or if they were made aware, I can't say I'd be interested in notifying them all). That'd probably go much more smoothly though if there was a common upstream config provided by open-vm-tools, and we'd probably get a more consistent UX as a result across the distros. Upstream (open-vm-tools) wasn't interested in dealing with it themselves since they didn't manage those configs and deferred to downstream to figure out and collaborate amongst themselves.

Thankfully, I was able to get in touch with systemd devs and identify an improvement there that resolved the issue. Downside being that VM guests would need to run that version of systemd or newer to benefit vs updating the open-vm-tools package which might have been more broadly available.

In the context of this PR, LimitNOFILE=infinity is harmful. It was misunderstood when contributed and third-party advice since then to work around often exhibits the same lack of understanding, only resolving it by minimizing the issue. It's niche knowledge, especially to most who encounter the issue, thus they don't have time to know any better. Maintaining the config in the project however, allows people like myself to put in that time and contribute a fix that benefits many more, that would otherwise not reach as wide of an audience. This is a bug that's existed for years, yet I doubt deferring entirely to downstream packagers would have been any better, a while back it was necessary to have an explicit LimitNOFILE due to the implicit default being 4096 which can be too low quite easily.

[neersighted]

Note to maintainers:

For this to be fully correct, the Go runtime should be 1.19+ as that is the version that introduced Go raising it's own soft-limit. We should note this in the changelog, or even add dummy file to keep containerd compiling with older Go versions (if it does still compile with 1.18 on the release branches) to prevent subtle misbehavior caused by no longer explicitly raising the soft-limit.

[metux]

Note to maintainers:

• For this to be fully correct, the Go runtime should be 1.19+ as that is the version that introduced Go raising it's own soft-limit. We should note this in the changelog, or even add dummy file to keep containerd compiling with older Go versions (if it does still compile with 1.18 on the release branches) to prevent subtle misbehavior caused by no longer explicitly raising the soft-limit.

Since go.mod explicitly demands 1.20, I doubt that we ever need to care about pre-1.19 anymore :p

[neersighted]

That was merely a suggestion with older toolchain versions; it's definitely possible that e.g. a hyperscaler might try to compile with an older Go in order to support the long-term support branch of their container OS.

[mikebrow]

Instead of just removing the line, should their be a comment for cases it might still be need or used?

@dmcgowan

I don't think so. In cases where it is needed it can be trivially patched in, either by patching the actual file when building a package or via systemd drop-in at runtime. In most places this line does nothing useful, in some places (growing number) the line is actively harmful.

if the line is actively harmful.. maybe make a statement explaining the harm of uncommenting it..

[polarathene]

maybe make a statement explaining the harm of uncommenting it

Covered in the commit message (same content as the PR description), notably the last line about excessive soft limit.

Originally in the equivalent PR for moby I had opted to keep the line with the expected implicit default used explicitly. It included a comment for context:

# Ensure a high enough limit for the daemon, while keeping a sane soft limit for child processes.
# The daemon will implicitly raise the soft limit to the hard limit (since Go 1.19),
# while restoring the soft limit to the original value for child processes.
LimitNOFILE=1024:524288

It communicates the intent rather than the harm from excessive values, which for maintainers would have at least had git blame for that context. However they chose to avoid a transition to removing it in future and preferred to drop the config line straight away.

EDIT: I already touched on this earlier in this PR discussion 😅

---

For maintainers at least, I've ensured a solid "paper trail" for traceability, with plenty of information to justify why it should be changed away from infinity. I don't expect it to be introduced again, but I have expressed concern about user awareness of the change (since some override it with bad advice online as a "works for me" workaround).

[estesp]

Note that this PR needs the commit repushed with the canonical form of <90 char or less subject> followed by blank line, followed by any length descriptive message, followed by blank line and then the "Signed-off-by: "; without this the full CI checks are not running due to:

FAIL - commit subject exceeds 90 characters

[dmcgowan]

Looks good to me, can you update the commit according to "- FAIL - commit subject exceeds 90 characters". Looks like an empty line would do the trick after the subject line.

[polarathene]

Hi, just a friendly ping to confirm everything is all sorted for this PR?

• Are there any remaining concerns blocking merge?
• Is it queued for an upcoming release?

[thaJeztah]

Instead of just removing the line, should their be a comment for cases it might still be need or used?

@neersighted any preferences on that? I recall we used to have some commented-out options with a comment explaining why (and having a commented-out line was convenient during packaging, as we could just use sed to un-comment the line on distros that still needed it)

otherwise LGTM