Skip to content
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.

Sign up for GitHub

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

Jump to bottom

libcontainer: map PidsLimit to systemd's TasksMax property #1917

Merged
merged 1 commit into from
Nov 13, 2018
Merged

libcontainer: map PidsLimit to systemd's TasksMax property #1917

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions libcontainer/cgroups/systemd/apply_systemd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,12 @@ func (m *Manager) Apply(pid int) error {
newProp("BlockIOWeight", uint64(c.Resources.BlkioWeight)))
}

if c.Resources.PidsLimit > 0 {
properties = append(properties,
newProp("TasksAccounting", true),
newProp("TasksMax", uint64(c.Resources.PidsLimit)))
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this work if we have an older version of systemd that doesnt support TasksAccounting or TasksMax ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's why TasksAccounting is only added along TasksMax if PidsLimit is greater than zero, instead of adding it unconditionally at the same time we add MemoryAccounting and co. That way, on (quite) old distros, the only breakage is that you won't be able to use "pids-limit".

On the other hand, TasksMax/TasksAccounting was added in systemd 227, which was released back in 2015-10-07. According to Distrowatch, Ubuntu 16.04 LTS has 229 (I think 14.04 LTS still uses upstart?), Debian "stretch" already has 232, and OpenSUSE 42.2 uses 228.

Older releases aren't probably well-suited to be used as a container platform for other, more important reasons (i.e. buggy aufs/overlay support), so I think we're playing safe here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this works the same as supplying a property in a *.service file, then it should be harmless to include -- older systemd versions should simply warn that it's unknown and ignore it (by design, specifically to handle this very quandry).

See https://www.freedesktop.org/software/systemd/man/systemd.unit.html, especially:

Unit files may contain additional options on top of those listed here. If systemd encounters an unknown option, it will write a warning log message but continue loading the unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, it fails with an error like this:

# docker run -ti --pids-limit 111  busybox /bin/sh
/usr/bin/docker-current: Error response from daemon: invalid header field value "oci runtime error: container_linux.go:247: starting container process caused \"process_linux.go:258: applying cgroup configuration for process caused \\\"Cannot set property TasksAccounting, or unknown property.\\\"\"\n".

This comes from a RHEL7.2 VM with systemd-219-19.el7.x86_64.

Of course, you can still create containers if you don't use "pids-limit":

# docker run -ti  busybox /bin/sh
/ # uname -a
Linux b216dfef5457 3.10.0-514.26.2.el7.x86_64 #1 SMP Fri Jun 30 05:26:04 UTC 2017 x86_64 GNU/Linux
/ #

And I still think distros that old aren't actually suitable as reliable container platforms for reasons way more important than missing TasksMax/TasksAccounting.

// We have to set kernel memory here, as we can't change it once
// processes have been attached to the cgroup.
if c.Resources.KernelMemory != 0 {
Expand Down