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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PIDs cgroup support to Docker #18697

Merged
merged 1 commit into from Mar 8, 2016

Conversation

Projects
None yet
@cyphar
Contributor

cyphar commented Dec 16, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Dec 16, 2015

Contributor

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.

Contributor

cyphar commented Dec 16, 2015

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.

Show outdated Hide outdated docs/reference/commandline/create.md
@@ -69,6 +69,7 @@ Creates a new container.
-P, --publish-all=false Publish all exposed ports to random ports
-p, --publish=[] Publish a container's port(s) to the host
--pid="" PID namespace to use
--pids-limit=0 Tune container pids limit (set -1 for unlimited)

This comment has been minimized.

@cpuguy83

cpuguy83 Dec 16, 2015

Contributor

What is the meaning of 0 for pids limit?

@cpuguy83

cpuguy83 Dec 16, 2015

Contributor

What is the meaning of 0 for pids limit?

This comment has been minimized.

@jessfraz

jessfraz Dec 16, 2015

Contributor

does nothing

@jessfraz

jessfraz Dec 16, 2015

Contributor

does nothing

This comment has been minimized.

@cpuguy83

cpuguy83 Dec 16, 2015

Contributor

So is 0 and -1 the same?

@cpuguy83

cpuguy83 Dec 16, 2015

Contributor

So is 0 and -1 the same?

This comment has been minimized.

@cyphar

cyphar Dec 17, 2015

Contributor

-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.

@cyphar

cyphar Dec 17, 2015

Contributor

-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.

This comment has been minimized.

@jessfraz

jessfraz Dec 17, 2015

Contributor

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)

@jessfraz

jessfraz Dec 17, 2015

Contributor

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)

This comment has been minimized.

@jessfraz

jessfraz Dec 18, 2015

Contributor

@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

@jessfraz

jessfraz Dec 18, 2015

Contributor

@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

This comment has been minimized.

@jessfraz

jessfraz Dec 19, 2015

Contributor

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

@jessfraz

jessfraz Dec 19, 2015

Contributor

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

This comment has been minimized.

@diogomonica

diogomonica Dec 19, 2015

Contributor

Absolutely. Would love to see the results.

@diogomonica

diogomonica Dec 19, 2015

Contributor

Absolutely. Would love to see the results.

This comment has been minimized.

@jessfraz

jessfraz Dec 19, 2015

Contributor

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

@jessfraz

jessfraz Dec 19, 2015

Contributor

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

This comment has been minimized.

@diogomonica

diogomonica Dec 19, 2015

Contributor

Ha! Problem solved.

@diogomonica

diogomonica Dec 19, 2015

Contributor

Ha! Problem solved.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 16, 2015

Contributor

Looks like the libcontainer vendor is out of date.

Contributor

cpuguy83 commented Dec 16, 2015

Looks like the libcontainer vendor is out of date.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Dec 16, 2015

Contributor

ya its waiting on a pr ;)

Contributor

jessfraz commented Dec 16, 2015

ya its waiting on a pr ;)

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Dec 19, 2015

Contributor

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

Contributor

jessfraz commented Dec 19, 2015

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 19, 2015

Member

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

Member

thaJeztah commented Dec 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Dec 19, 2015

Contributor

: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)).

Contributor

jessfraz commented Dec 19, 2015

: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

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Dec 22, 2015

Contributor

we are greeennnnnn

Contributor

jessfraz commented Dec 22, 2015

we are greeennnnnn

@jessfraz jessfraz added this to the 1.10 milestone Dec 22, 2015

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 2, 2016

Member

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

Member

thaJeztah commented Jan 2, 2016

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

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jan 2, 2016

Contributor

@thaJeztah Yup, that is correct.

Contributor

cyphar commented Jan 2, 2016

@thaJeztah Yup, that is correct.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jan 8, 2016

Contributor

The failing builds look like a cascading failure (one build fails, and then the assumptions of a clean working environment with no other containers is broken for the rest of the builds). In fact, the build which causes the issue may not even fail (only the removal of the container need fail). I'll take a look at this next week.

Contributor

cyphar commented Jan 8, 2016

The failing builds look like a cascading failure (one build fails, and then the assumptions of a clean working environment with no other containers is broken for the rest of the builds). In fact, the build which causes the issue may not even fail (only the removal of the container need fail). I'll take a look at this next week.

@crosbymichael crosbymichael removed this from the 1.10.0 milestone Jan 8, 2016

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 8, 2016

Contributor

Because we are still looking at issues with the libcontainer changes this does not need to be in the 1.10 milestone and this feature is only available is 4.3+ kernels its not mainstream for most users at the moment. Lets take care of the libcontainer changes first.

Contributor

crosbymichael commented Jan 8, 2016

Because we are still looking at issues with the libcontainer changes this does not need to be in the 1.10 milestone and this feature is only available is 4.3+ kernels its not mainstream for most users at the moment. Lets take care of the libcontainer changes first.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jan 12, 2016

Contributor

We going 馃崗 馃摋 馃挌! Gogogogo. 馃槃 馃帀
/ping @jfrazelle

Contributor

cyphar commented Jan 12, 2016

We going 馃崗 馃摋 馃挌! Gogogogo. 馃槃 馃帀
/ping @jfrazelle

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jan 12, 2016

Contributor

going to wait for libcontainer tag :)

On Mon, Jan 11, 2016 at 5:15 PM, Aleksa Sarai notifications@github.com
wrote:

We going [image: 馃崗] [image: 馃摋] [image:
馃挌]! Gogogogo. [image: 馃槃] [image: 馃帀]
/ping @jfrazelle https://github.com/jfrazelle


Reply to this email directly or view it on GitHub
#18697 (comment).

Contributor

jessfraz commented Jan 12, 2016

going to wait for libcontainer tag :)

On Mon, Jan 11, 2016 at 5:15 PM, Aleksa Sarai notifications@github.com
wrote:

We going [image: 馃崗] [image: 馃摋] [image:
馃挌]! Gogogogo. [image: 馃槃] [image: 馃帀]
/ping @jfrazelle https://github.com/jfrazelle


Reply to this email directly or view it on GitHub
#18697 (comment).

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jan 13, 2016

Contributor

@jfrazelle Okay, so all vendored commits for libcontainer will fail until opencontainers/runc#470 is merged (hint-hint 馃槈). We should get that merged before we tag libcontainer.

Contributor

cyphar commented Jan 13, 2016

@jfrazelle Okay, so all vendored commits for libcontainer will fail until opencontainers/runc#470 is merged (hint-hint 馃槈). We should get that merged before we tag libcontainer.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jan 13, 2016

Contributor

totally :)

On Wed, Jan 13, 2016 at 8:48 AM, Aleksa Sarai notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle Okay, so all vendored commits
for libcontainer will fail until opencontainer/runc#470 is merged
(hint-hint [image: 馃槈]). We should get that merged before we tag
libcontainer.


Reply to this email directly or view it on GitHub
#18697 (comment).

Contributor

jessfraz commented Jan 13, 2016

totally :)

On Wed, Jan 13, 2016 at 8:48 AM, Aleksa Sarai notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle Okay, so all vendored commits
for libcontainer will fail until opencontainer/runc#470 is merged
(hint-hint [image: 馃槈]). We should get that merged before we tag
libcontainer.


Reply to this email directly or view it on GitHub
#18697 (comment).

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jan 26, 2016

Contributor

@jfrazelle Okay, libcontainer has been revved to 0.0.7. AFAIK all of the runc bugs have been fixed, so you're good to revendor, rebase and all that jazz.

Contributor

cyphar commented Jan 26, 2016

@jfrazelle Okay, libcontainer has been revved to 0.0.7. AFAIK all of the runc bugs have been fixed, so you're good to revendor, rebase and all that jazz.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jan 26, 2016

Contributor

Woooohoooooo

On Monday, January 25, 2016, Aleksa Sarai notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle Okay, libcontainer has been
revved to 0.0.7. AFAIK all of the runc bugs have been fixed, so you're
good to revendor, rebase and all that jazz.


Reply to this email directly or view it on GitHub
#18697 (comment).

Contributor

jessfraz commented Jan 26, 2016

Woooohoooooo

On Monday, January 25, 2016, Aleksa Sarai notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle Okay, libcontainer has been
revved to 0.0.7. AFAIK all of the runc bugs have been fixed, so you're
good to revendor, rebase and all that jazz.


Reply to this email directly or view it on GitHub
#18697 (comment).

@jessfraz jessfraz added this to the 1.11.0 milestone Jan 27, 2016

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Feb 2, 2016

Contributor

I will rebase after #19752 is merged ;)

Contributor

jessfraz commented Feb 2, 2016

I will rebase after #19752 is merged ;)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 17, 2016

Member

ping @cyphar @jfrazelle #20187 was just merged \o/

Member

thaJeztah commented Feb 17, 2016

ping @cyphar @jfrazelle #20187 was just merged \o/

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Feb 20, 2016

Contributor

@jfrazelle Looks like some of the testing hooks' signatures have changed in the past 3 months. :P

Contributor

cyphar commented Feb 20, 2016

@jfrazelle Looks like some of the testing hooks' signatures have changed in the past 3 months. :P

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Feb 26, 2016

Contributor

@jfrazelle If you're really busy with other stuff, I'd be happy to carry this PR. 馃樃

Contributor

cyphar commented Feb 26, 2016

@jfrazelle If you're really busy with other stuff, I'd be happy to carry this PR. 馃樃

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Feb 26, 2016

Contributor

I'll be back and can update Monday or even Sunday but up to you

On Friday, February 26, 2016, Aleksa Sarai notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle If you're really busy with
other stuff, I'd be happy to carry this PR. [image: 馃樃]


Reply to this email directly or view it on GitHub
#18697 (comment).

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

Contributor

jessfraz commented Feb 26, 2016

I'll be back and can update Monday or even Sunday but up to you

On Friday, February 26, 2016, Aleksa Sarai notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle If you're really busy with
other stuff, I'd be happy to carry this PR. [image: 馃樃]


Reply to this email directly or view it on GitHub
#18697 (comment).

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

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Feb 26, 2016

Contributor

@jfrazelle Ah okay, yeah if you can do it next week that's fine. I know you've been swamped with all of the seccomp and security profile stuff. :P

Contributor

cyphar commented Feb 26, 2016

@jfrazelle Ah okay, yeah if you can do it next week that's fine. I know you've been swamped with all of the seccomp and security profile stuff. :P

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Feb 26, 2016

Contributor

Actually I've been on vacation lol :P I haven't done anything hahaha

On Friday, February 26, 2016, Aleksa Sarai notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle Ah okay, yeah if you can do it
next week that's fine. I know you've been swamped with all of the seccomp
and security profile stuff. :P


Reply to this email directly or view it on GitHub
#18697 (comment).

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

Contributor

jessfraz commented Feb 26, 2016

Actually I've been on vacation lol :P I haven't done anything hahaha

On Friday, February 26, 2016, Aleksa Sarai notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle Ah okay, yeah if you can do it
next week that's fine. I know you've been swamped with all of the seccomp
and security profile stuff. :P


Reply to this email directly or view it on GitHub
#18697 (comment).

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

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 1, 2016

Contributor

rebased!

Contributor

jessfraz commented Mar 1, 2016

rebased!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 1, 2016

Member

@jfrazelle

19:22:51 + go test -test.timeout=120m -check.v github.com/docker/docker/integration-cli
19:23:25 # github.com/docker/docker/integration-cli
19:23:25 ./docker_cli_run_unix_test.go:968: not enough arguments in call to inspectField
19:23:25 ./docker_cli_run_unix_test.go:968: assignment count mismatch: 2 = 1
Member

thaJeztah commented Mar 1, 2016

@jfrazelle

19:22:51 + go test -test.timeout=120m -check.v github.com/docker/docker/integration-cli
19:23:25 # github.com/docker/docker/integration-cli
19:23:25 ./docker_cli_run_unix_test.go:968: not enough arguments in call to inspectField
19:23:25 ./docker_cli_run_unix_test.go:968: assignment count mismatch: 2 = 1
Show outdated Hide outdated docs/reference/commandline/create.md
@@ -74,6 +74,7 @@ Creates a new container.
-P, --publish-all Publish all exposed ports to random ports
-p, --publish=[] Publish a container's port(s) to the host
--pid="" PID namespace to use
--pids-limit=0 Tune container pids limit (set -1 for unlimited), kernel >= 4.3

This comment has been minimized.

@cyphar

cyphar Mar 5, 2016

Contributor

The default value is confusing. Isn't the default "unlimited"?

@cyphar

cyphar Mar 5, 2016

Contributor

The default value is confusing. Isn't the default "unlimited"?

Show outdated Hide outdated docs/reference/commandline/run.md
@@ -74,6 +74,7 @@ parent = "smn_cli"
-P, --publish-all Publish all exposed ports to random ports
-p, --publish=[] Publish a container's port(s) to the host
--pid="" PID namespace to use
--pids-limit=0 Tune container pids limit (set -1 for unlimited), kernel >= 4.3

This comment has been minimized.

@cyphar

cyphar Mar 5, 2016

Contributor

Same here.

@cyphar

cyphar Mar 5, 2016

Contributor

Same here.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 7, 2016

Contributor

Docs LGTM

Contributor

calavera commented Mar 7, 2016

Docs LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 7, 2016

Member

docs LGTM, thanks!

@jfrazelle can we squash the commits, or any reason to keep them separate?

Member

thaJeztah commented Mar 7, 2016

docs LGTM, thanks!

@jfrazelle can we squash the commits, or any reason to keep them separate?

pids limit support
update bash commpletion for pids limit

update check config for kernel

add docs for pids limit

add pids stats

add stats to docker client

Signed-off-by: Jessica Frazelle <acidburn@docker.com>
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 8, 2016

Contributor

squashed

Contributor

jessfraz commented Mar 8, 2016

squashed

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 8, 2016

Contributor

Almost greerreeeeeennnnn

Contributor

jessfraz commented Mar 8, 2016

Almost greerreeeeeennnnn

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
Member

thaJeztah commented Mar 8, 2016

8530707

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 8, 2016

Contributor

All 馃挌 Merging!

Contributor

calavera commented Mar 8, 2016

All 馃挌 Merging!

calavera added a commit that referenced this pull request Mar 8, 2016

Merge pull request #18697 from jfrazelle/pids-cgroup
Add PIDs cgroup support to Docker

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

8 checks passed

docker/dco-signed All commits signed
Details
documentation success 2 tests run, 0 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 16008 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 2893 has succeeded
Details
janky Jenkins build Docker-PRs 24801 has succeeded
Details
userns Jenkins build Docker-PRs-userns 7103 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 22890 has succeeded
Details
windowsTP4 Jenkins build Docker-PRs-WoW-TP4 2240 has succeeded
Details
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Mar 8, 2016

Contributor

Yay! 馃帀

Contributor

cyphar commented Mar 8, 2016

Yay! 馃帀

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

This comment has been minimized.

@tonistiigi

tonistiigi Mar 9, 2016

Member

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

@tonistiigi

tonistiigi Mar 9, 2016

Member

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

This comment has been minimized.

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

This comment has been minimized.

@tonistiigi

tonistiigi Mar 9, 2016

Member

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.

@tonistiigi

tonistiigi Mar 9, 2016

Member

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.

This comment has been minimized.

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

This comment has been minimized.

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

This comment has been minimized.

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

This comment has been minimized.

@cyphar

cyphar Mar 9, 2016

Contributor

I can fix this if you like.

@cyphar

cyphar Mar 9, 2016

Contributor

I can fix this if you like.

This comment has been minimized.

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

@jessfraz

jessfraz Mar 9, 2016

Contributor

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

This comment has been minimized.

@cyphar

cyphar Mar 9, 2016

Contributor

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).

@cyphar

cyphar Mar 9, 2016

Contributor

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).

This comment has been minimized.

@cyphar

cyphar Mar 12, 2016

Contributor

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

/cc @jfrazelle @calavera @tonistiigi

@cyphar

cyphar Mar 12, 2016

Contributor

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

/cc @jfrazelle @calavera @tonistiigi

@jessfraz jessfraz deleted the jessfraz:pids-cgroup branch Mar 19, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 15, 2016

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

ghost commented Apr 15, 2016

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

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Apr 15, 2016

Contributor

@mkonakan Yes, this is in Docker 1.11.

Contributor

cyphar commented Apr 15, 2016

@mkonakan Yes, this is in Docker 1.11.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 15, 2016

Member

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

Member

thaJeztah commented Apr 15, 2016

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

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 15, 2016

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 :)

ghost commented Apr 15, 2016

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 :)

@tianon tianon referenced this pull request Apr 19, 2016

Merged

Update kernel to 4.4.7 #1159

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

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 22, 2016

Contributor

@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

Contributor

cpuguy83 commented Jul 22, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@innocentme1

innocentme1 Jul 22, 2016

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?

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

This comment has been minimized.

Show comment
Hide comment
@innocentme1

innocentme1 Jul 22, 2016

@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)

innocentme1 commented Jul 22, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 22, 2016

Contributor

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

Contributor

cpuguy83 commented Jul 22, 2016

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

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 22, 2016

Contributor

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.

Contributor

cpuguy83 commented Jul 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@innocentme1

innocentme1 Jul 22, 2016

@cpuguy83 sure. Thank you 馃憤

@cpuguy83 sure. Thank you 馃憤

@matheuss

This comment has been minimized.

Show comment
Hide comment
@matheuss

matheuss Jan 11, 2018

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

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

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jan 11, 2018

Contributor

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

Contributor

cyphar commented Jan 11, 2018

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

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 11, 2018

Contributor

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

Contributor

cpuguy83 commented Jan 11, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment