Add PIDs cgroup support to Docker #18697

cyphar · 2015-12-16T04:48:49Z

@jfrazelle gave me the honour of opening this on her behalf 😸.

The PIDs cgroup is vital to allowing the limiting of the number of processes with high granularity (on the level of containers). The other main options are simply not fit for the task:

LIMIT_NPROC has no granularity.
kmemcg just simply doesn't work.
ulimit operates on a user basis, so docker run -u someone will cause issues.

opencontainers/runc#446 has been merged. We're waiting on 1.10 to be released (as well as some issues in runc to be resolved).

cyphar · 2015-12-16T05:00:46Z

I'd like to point out to reviewers that the tests passing doesn't actually indicate if the code works, since the QA test beds don't support the PIDs cgroup. Please try the tests on your own machine (with a 4.3+ kernel with CONFIG_CGROUP_PIDS=y). OpenSUSE Tumbleweed, Arch Linux and a few other distros have CONFIG_CGROUP_PIDS=y by default.

cpuguy83 · 2015-12-16T21:18:44Z

docs/reference/commandline/create.md

What is the meaning of 0 for pids limit?

does nothing

So is 0 and -1 the same?

-1 means "explicitly set the limit to be unlimited, and error out if the cgroup doesn't exist"
0 means "use the default" which results in it being unlimited.
We're trying to iron out this interface in opencontainers/runtime-spec#233.

also thinking it would be nice if there was a sane default for all containers set by the daemon something like 512 (more than enough, but not unlimited)

@cyphar agreed,

@diogomonica I ran several programs and tested various containers and I think 512 is a good number... it will be applied to the cgroup parent which means not every container gets 512, they get a portion, java uses quite a lot so we are trying to provide security while not having people just change it to unlimited

I have been running this patch on several of our servers including prod ones to collect stats I'd be willing to show you the results

Absolutely. Would love to see the results.

ill save you the time, they say 512 is the magic number :P

I can make a gist later it's pretty interesting even redis spawns multiple

Ha! Problem solved.

cpuguy83 · 2015-12-16T21:47:31Z

Looks like the libcontainer vendor is out of date.

jessfraz · 2015-12-16T22:22:46Z

ya its waiting on a pr ;)

jessfraz · 2015-12-19T04:29:59Z

now rebased on #18788, made it a diff PR, so we dont hold up w this PR design discussion since gccgo needs it

thaJeztah · 2015-12-19T18:52:06Z

Looks like there were some issues in RunC opencontainers/runc#58 (comment) and the PR was temporarily reverted until those are solved opencontainers/runc#445

jessfraz · 2015-12-19T18:55:40Z

:tear:

On Dec 19, 2015, 10:54 -0800, Sebastiaan van Stijnnotifications@github.com, wrote:

Looks like there were some issues in RunCopencontainers/runc#58 (comment)(opencontainers/runc#58 (comment) the PR was temporarily reverted until those are solvedopencontainers/runc#445(opencontainers/runc#445)

—
Reply to this email directly orview it on GitHub(#18697 (comment)).

jessfraz · 2015-12-22T04:05:36Z

we are greeennnnnn

thaJeztah · 2016-01-02T00:04:06Z

I think this is now blocked on opencontainers/runc#446?

cyphar · 2016-01-02T03:03:36Z

@thaJeztah Yup, that is correct.

cyphar · 2016-03-08T22:20:50Z

Yay! 🎉

tonistiigi · 2016-03-09T01:49:17Z

api/client/stats_helpers.go

 	NetworkTx        float64
 	BlockRead        float64
 	BlockWrite       float64
+	PidsCurrent      uint64


Where is this being set? I think we need something like this: 103cc23#diff-234033fe3b8730c35e53ce0d8dbda485R119

I can't find that same function on master the stats one in daemon?

On Tuesday, March 8, 2016, Tõnis Tiigi notifications@github.com wrote:

In api/client/stats_helpers.go
#18697 (comment):

@@ -24,6 +24,7 @@ type containerStats struct {
NetworkTx float64
BlockRead float64
BlockWrite float64

PidsCurrent uint64

Where is this being set? I think we need something like this: 103cc23
#diff-234033fe3b8730c35e53ce0d8dbda485R119
103cc23#diff-234033fe3b8730c35e53ce0d8dbda485R119

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/18697/files#r55461314.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

I only meant that setting of this value before pritning seems to be missing on the client side. My commit is based on the containerd branch so the daemon side is different there.

Maybe something got lost in a rebase because I have used it and it works

On Tuesday, March 8, 2016, Tõnis Tiigi notifications@github.com wrote:

In api/client/stats_helpers.go
#18697 (comment):

@@ -24,6 +24,7 @@ type containerStats struct {
NetworkTx float64
BlockRead float64
BlockWrite float64

PidsCurrent uint64

I only meant that setting of this value before pritning seems to be
missing on the client side. My commit is based on the containerd branch so
the daemon side is different there.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/18697/files#r55464136.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

It's right here :)
https://github.com/docker/docker/pull/18697/files#diff-77f23598eb03c37b3f275940922fe494R65

On Tuesday, March 8, 2016, Jessica Frazelle me@jessfraz.com wrote:

Maybe something got lost in a rebase because I have used it and it works

On Tuesday, March 8, 2016, Tõnis Tiigi <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

In api/client/stats_helpers.go
#18697 (comment):

@@ -24,6 +24,7 @@ type containerStats struct {
NetworkTx float64
BlockRead float64
BlockWrite float64

PidsCurrent uint64

I only meant that setting of this value before pritning seems to be
missing on the client side. My commit is based on the containerd branch so
the daemon side is different there.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/18697/files#r55464136.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu
http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

Oh I see yeah that was definitely lost in a rebase :(

On Tuesday, March 8, 2016, Tõnis Tiigi notifications@github.com wrote:

In api/client/stats_helpers.go
#18697 (comment):

@@ -24,6 +24,7 @@ type containerStats struct {
NetworkTx float64
BlockRead float64
BlockWrite float64

PidsCurrent uint64

That's setting cgroup value to the api type on daemon side. PidsCurrent
is client side value used for printing.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/18697/files#r55465554.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

I can fix this if you like.

Thanks!

On Tuesday, March 8, 2016, Aleksa Sarai notifications@github.com wrote:

In api/client/stats_helpers.go
#18697 (comment):

@@ -24,6 +24,7 @@ type containerStats struct {
NetworkTx float64
BlockRead float64
BlockWrite float64

PidsCurrent uint64

I can fix this if you like.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/18697/files#r55476919.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

I'm also planning on adding PidsLimit to the stats as well so we can get the cool current / limit stuff all the other cgroups do. Obviously that'll take a while (libcontainer + engine-api + docker).

I've made a PR fixing this (#21150).

/cc @jfrazelle @calavera @tonistiigi

ghost · 2016-04-15T05:39:55Z

Was this feature not included in the 1.11 released on 13th april?

cyphar · 2016-04-15T08:44:41Z

@mkonakan Yes, this is in Docker 1.11.

thaJeztah · 2016-04-15T09:16:31Z

@mkonakan it requires kernel 4.3 or up though, so may not be available if you're using an older version

ghost · 2016-04-15T18:46:34Z

Cool. Thank you. I am currently on 1.10 only. I could not find this feature is included in the 1.11 release information. So, posted the question here. Will update and check on it. Thank you guys :)

[--pids-limit](moby/moby#18697) [--security-opt=no-new-privileges](moby/moby#20727)

cpuguy83 · 2016-07-22T01:36:54Z

@innocentme1 There is no default, it's just what you set per container.
PIDs cgroup controller docs: https://www.kernel.org/doc/Documentation/cgroup-v1/pids.txt

innocentme1 · 2016-07-22T01:38:30Z

Re-posting as my previous comment seems to be scrambled

@cpuguy83 @cyphar I am currently not on kernel > 4.3 so I could not test things and I also could not find them in any docs. So, expecting your help for below ques

1- Is this PIDs limit enabled by default in Docker? How do I verify it?
2. How do I disable this setting if its default?
3. Whats the process limit count when this setting will detect & prevent?
4. Can I change the default limit for detection? If yes - how?
5. Where can I see the details regarding this on docker docs? or any command etc?

innocentme1 · 2016-07-22T01:40:55Z

@cpuguy83 I would appreciate if I can get answers for each one of the one's I requested so that I can get clear idea on everything. So, if its per container level

Do I have to set everytime I launch a container?
If I do not specify each time , whats the default value it takes ? (Assuming it has feature enabled by default)

cpuguy83 · 2016-07-22T01:42:31Z

@innocentme1 Please read the docs.
This setting is no different than any other setting that docker provides to containers.

cpuguy83 · 2016-07-22T01:44:08Z

p.s., if you would like to discuss further #docker on IRC or forums.docker.com is far more appropriate and easier to help out than a closed GH PR.

innocentme1 · 2016-07-22T04:49:20Z

@cpuguy83 sure. Thank you 👍

matheuss · 2018-01-11T01:16:46Z

Any reason on why this wasn't added to docker update as well?

cyphar · 2018-01-11T01:27:29Z

Not really @matheuss, it should be fairly trivial to add to docker update as well.

cpuguy83 · 2018-01-11T16:04:45Z

@matheuss Here's the PR for it: #32519

GordonTheTurtle added the status/0-triage label Dec 16, 2015

jessfraz force-pushed the pids-cgroup branch from 02eddd9 to d123813 Compare December 16, 2015 17:49

calavera added status/1-design-review and removed status/0-triage labels Dec 16, 2015

jessfraz added impact/changelog impact/api labels Dec 16, 2015

cpuguy83 reviewed Dec 16, 2015
View reviewed changes

jessfraz force-pushed the pids-cgroup branch 6 times, most recently from baa7675 to cdc722f Compare December 19, 2015 04:28

jessfraz force-pushed the pids-cgroup branch 3 times, most recently from ed9904f to 3c9890e Compare December 22, 2015 00:22

jessfraz added this to the 1.10 milestone Dec 22, 2015

jessfraz force-pushed the pids-cgroup branch from 3c9890e to 33a9207 Compare December 23, 2015 21:20

jessfraz force-pushed the pids-cgroup branch 2 times, most recently from a02c26d to a5a1503 Compare January 6, 2016 18:41

LK4D4 mentioned this pull request Jan 6, 2016

cgroup: add PIDs cgroup controller support opencontainers/runc#446

Merged

calavera merged commit dd32445 into moby:master Mar 8, 2016

tonistiigi reviewed Mar 9, 2016
View reviewed changes

thaJeztah mentioned this pull request Mar 12, 2016

client: stats: fill in stats info on client #21150

Merged

jessfraz deleted the pids-cgroup branch March 19, 2016 01:03

tianon mentioned this pull request Apr 19, 2016

Update kernel to 4.4.7 boot2docker/boot2docker#1159

Merged

geminiyellow added a commit to ilivebox/docker-cheat-sheet that referenced this pull request Apr 25, 2016

docs(zh-cn): Added new security features from docker 1.11

62710b7

[--pids-limit](moby/moby#18697) [--security-opt=no-new-privileges](moby/moby#20727)

innocentme1 unassigned jessfraz Jul 22, 2016

innocentme1 mentioned this pull request Aug 4, 2016

Incorrect default PID value? #25412

Closed

estesp mentioned this pull request Oct 18, 2016

userns-remap:: user namespace does not isolate user limits #27444

Closed

thaJeztah mentioned this pull request Feb 25, 2017

docker create does not work for ulimit of nproc usage #31358

Open

This was referenced Mar 27, 2017

rlimit support kubernetes/kubernetes#3595

Open

Allow setting pids-limit on containers kubernetes/kubernetes#43783

Closed

ejholmes mentioned this pull request Oct 10, 2017

Replace nproc with pids limit remind101/empire#1110

Open

thaJeztah mentioned this pull request Nov 11, 2017

Request: Swarm Mode 17.10-ce: Cannot allocate memory #35469

Closed

kolyshkin mentioned this pull request May 22, 2020

pkg/sysinfo.applyPIDSCgroupInfo: optimize #41014

Merged

archfz mentioned this pull request Dec 1, 2021

Overriding default 0 pids limit on daemon config level #43056

Open

Add PIDs cgroup support to Docker #18697

Add PIDs cgroup support to Docker #18697

Uh oh!

Conversation

cyphar commented Dec 16, 2015

Uh oh!

cyphar commented Dec 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Dec 16, 2015

Uh oh!

jessfraz commented Dec 16, 2015

Uh oh!

jessfraz commented Dec 19, 2015

Uh oh!

thaJeztah commented Dec 19, 2015

Uh oh!

jessfraz commented Dec 19, 2015

Uh oh!

jessfraz commented Dec 22, 2015

Uh oh!

thaJeztah commented Jan 2, 2016

Uh oh!

cyphar commented Jan 2, 2016

Uh oh!

cyphar commented Mar 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 15, 2016

Uh oh!

cyphar commented Apr 15, 2016

Uh oh!

thaJeztah commented Apr 15, 2016

Uh oh!

ghost commented Apr 15, 2016

Uh oh!

cpuguy83 commented Jul 22, 2016

Uh oh!

innocentme1 commented Jul 22, 2016

Uh oh!

innocentme1 commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

innocentme1 commented Jul 22, 2016 •

edited

Loading