Add pids-limit support in docker update #32519

darkowlzz · 2017-04-11T11:53:05Z

Added a new flag --pids-limit in docker update command.
Value of the pidsLimit flag variable is set as PidsLimit in
containertypes.Resources, which is used to apply the update in
hostConfig and the real world (running containers).
Added TestUpdatePIDsLimit integration test for the new flag.

Fixes #32443

Signed-off-by: Sunny Gogoi indiasuny000@gmail.com

- What I did

Added a new flag, --pids-limit , in the command docker update.

- How I did it

Details are in commit body.

- How to verify it

Verify by starting a container with --pids-limit=4 and then updating pids-limit of the container
to a new value with docker update --pids-limit=<newValue> <containerName>.
Once updated, the result can be seen in docker inspect <containerName> and in the file
/sys/fs/cgroup/pids/pids.max within the container.

- Description for the changelog

Add pids-limit support in docker update

- A picture of a cute animal (not mandatory but encouraged)

darkowlzz · 2017-04-11T11:58:21Z

daemon/update_linux.go

Need help with this part. From my findings, I need to add PidsLimit here to update the running containers (real world). But libcontainered.Resources has no attribute named PidsLimit. Hence, the build would fail for now.

I found that the libcontainered resources attributes are defined in github.com/docker/containerd/api/grpc/types, but I'm not sure how to generate api-pb.go once I edit api.proto.

Can someone help me with this?

I think @mlaventure would be the best person to assist 👍

So I read proto3 docs and installed protoc with Go protocol buffer plugin.
Ran protoc --go_out=./ api.proto from vendor/github.com/docker/containerd/api/grpc/types. It generated api.pb.go with PidsLimit in UpdateResource and a method GetPidsLimit() for the same type. Along with this, it also modified a lot of other parts of the file. Not sure if we need so many changes.

I'll wait for some assistance on how to generate api.pb.go that properly fits here 🙂

@darkowlzz you need to open a PR against the v0.2.x branch of https://github.com/containerd/containerd to add that field into api.proto. Once that PR is merged you'll need to vendor containerd into docker (as part of this PR)

thaJeztah · 2017-04-11T12:24:17Z

cli/command/container/update.go

Before I forget to comment; this needs an "annotation" so that the flag is hidden when connecting to an older daemon version;

flags.SetAnnotation("cpus", "version", []string{"1.30"})

Added "annotation" for "pids-limit" with version 1.30.

@thaJeztah do you want me to change annotation version for "cpus"? Not sure why your code snippet has "cpus" with version 1.30, when the same line exists with 1.29 😅

oh, sorry, no that was just a copy/paste error on my side

btw: Shouldn't the default be -1 and a value of 0 errors out?

darkowlzz · 2017-04-15T11:05:13Z

After vendoring new gRPC APIs, build is no longer failing but when I update pids-limit, HostConfig gets updated with new limit but not the file /sys/fs/cgroup/pids/pids.max in the container. I couldn't figure out what am I missing. Till here PidsLimit is passed with the new value. And then nothing happens. /sys/fs/cgroup/pids/pids.max doesn't change.

@thaJeztah & @mlaventure can you guys help here? Couldn't find exactly where cgroup files are updated.

mlaventure · 2017-04-15T20:19:54Z

@darkowlzz the path would be something like /sys/fs/cgroup/pids/docker/<container-id>/pids.max

Also, you need to update vendor.conf with the last containerd commit and run vndr

darkowlzz · 2017-04-16T19:26:28Z

@mlaventure oops! 😅 fixed the vendoring.

And about the pids-limit file, yes, /sys/fs/cgroup/pids/docker/<container-id>/pids.max is the file on host machine, that reflects /sys/fs/cgroup/pids/pids.max in the container. But I'm stuck with pids.max file not getting updated even after passing PidsLimit to UpdateContainer().

On checked runc code (rev 9c2d8d184e5da67c95d601382adf14862e4f2228, from vendor.conf), I found that runc/libcontainer/cgroups/fs/pids.go has the code that updates pids.max. But for some reason, that code is not in the vendored runc in docker repo. But at the same time runc/libcontainer/cgroup/fs/blkio.go and other similar files are also not vendored but their corresponding files do get updated 🤷‍♂️ confused. Need help in understanding how it's working 😞

mlaventure

@darkowlzz sorry for the delay, DockerCon happened :)

I think it doesn't work for you because you forgot to also update hack/dockerfile/binaries-commits :)

The runc code for pids doesn't get vendored in because it is not directly used by the engine code (there's no reference to either of the type or routine found in that package within the engine). The use happen indirectly via containerd

mlaventure · 2017-04-24T16:24:21Z

cli/command/container/update.go

btw: Shouldn't the default be -1 and a value of 0 errors out?

darkowlzz · 2017-04-26T07:40:45Z

@mlaventure Changed the default pidsLimit value to -1 for max value by default 😅

Updated hack/dockerfile/binaries-commits with new containerd commit hash. To install new containerd, ran hack/dockerfile/install-binaries.sh containerd. This failed due to recent changes in containerd import path docker/containerd -> containerd/containerd. Changed install-binaries.sh to fix this and it installed the new containerd successfully.

Checked vendored containerd/api/grpc/types/api.pb.go to ensure the new code is present.

Rebuilt docker hack/make.sh binary install-binary run.

Checked containerd version in docker info
containerd version: a47bf53545fed63fe49646b289c36b57c5386b06

Created a new alpine container with --pids-limit set to 20 and then updated to 15. Updated value is visible in docker inspect. But it still doesn't update in /sys/fs/cgroup/pids/pids.max or /sys/fs/cgroup/pids/docker/<container-id>/pids.max.

Anything else that is missing?

mlaventure · 2017-04-26T17:40:18Z

@darkowlzz everything is now fine on both docker and containerd. It looks like runc needs to be updated though :/

I made a PR to add it here

darkowlzz · 2017-04-26T19:43:22Z

@mlaventure 😱 great! thanks a lot :)

darkowlzz · 2017-04-28T21:40:17Z

yay! runc changes got merged 🎉 @mlaventure

In vendor.conf, I see a fork of runc (docker/runc) is being used. Would be it safe to update that with the main opencontainers/runc repo?

mlaventure · 2017-05-01T14:50:35Z

@darkowlzz yes, we are just waiting for runc master to be merged into containerd see containerd/containerd#778

It's waiting on the integration with docker to be done and tested.

cpuguy83 · 2017-06-09T01:50:32Z

Looks like we need a rebase, and move the CLI bits to github.com/docker/cli now that it has been moved out of this repo.

thaJeztah · 2018-06-25T19:02:07Z

Sounds like the best option; not sure if we can change that for other properties that have the same issue though 😞

olljanat · 2018-12-21T18:49:58Z

@darkowlzz this one needs to be rebased

cpuguy83 · 2019-02-01T21:50:39Z

Rebased, made pids limit a pointer so users can update to unlimited by setting 0.

thaJeztah

LGTM

kolyshkin

test case can be simlplified a lot wrt exec

integration/container/update_linux_test.go

- Adds updating PidsLimit in UpdateContainer(). - Adds setting PidsLimit in toContainerResources(). Signed-off-by: Sunny Gogoi <indiasuny000@gmail.com> Signed-off-by: Brian Goff <cpuguy83@gmail.com>

thaJeztah · 2019-02-23T14:20:54Z

experimental failing on a flaky test #38521

23:06:11 FAIL: docker_cli_start_test.go:189: DockerSuite.TestStartReturnCorrectExitCode
23:06:11 
23:06:11 docker_cli_start_test.go:194:
23:06:11     c.Assert(err, checker.NotNil)
23:06:11 ... value = nil

powerpc failing on another flaky #38720

23:45:34 FAIL: docker_cli_daemon_test.go:118: DockerDaemonSuite.TestDaemonRestartUnlessStopped
23:45:34 
23:45:34 [dc3c44c677a52] waiting for daemon to start
23:45:34 [dc3c44c677a52] daemon started
23:45:34 
23:45:34 docker_cli_daemon_test.go:157:
23:45:34     // both stopped
23:45:34     testRun(map[string]bool{"top1": false, "top2": false, "exit": false}, "")
23:45:34 docker_cli_daemon_test.go:140:
23:45:34     c.Assert(strings.Contains(out, name), check.Equals, shouldRun, check.Commentf(format, prefix, name))
23:45:34 ... obtained bool = true
23:45:34 ... expected bool = false
23:45:34 ... container "exit" is running
23:45:34 
23:45:34 [dc3c44c677a52] exiting daemon
23:45:37

I'll merge

GordonTheTurtle added the status/0-triage label Apr 11, 2017

darkowlzz commented Apr 11, 2017

View reviewed changes

vdemeester added status/1-design-review and removed status/0-triage labels Apr 11, 2017

vdemeester requested review from cpuguy83, thaJeztah and mlaventure April 11, 2017 12:20

thaJeztah reviewed Apr 11, 2017

View reviewed changes

mlaventure added the status/needs-vendoring label Apr 12, 2017

darkowlzz mentioned this pull request Apr 12, 2017

api.proto: Add pidsLimit containerd/containerd#726

Merged

darkowlzz force-pushed the 32443-docker-update-pids-limit branch from f28efd8 to 1ba6ea4 Compare April 14, 2017 21:10

darkowlzz force-pushed the 32443-docker-update-pids-limit branch 2 times, most recently from 46daeb4 to 3d04429 Compare April 16, 2017 13:11

mlaventure reviewed Apr 24, 2017

View reviewed changes

cli/command/container/update.go Outdated

Copy link

Contributor

mlaventure Apr 24, 2017

Choose a reason for hiding this comment

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

btw: Shouldn't the default be -1 and a value of 0 errors out?

GordonTheTurtle assigned yongtang Apr 26, 2017

darkowlzz force-pushed the 32443-docker-update-pids-limit branch from 3d04429 to 6ce52a4 Compare April 26, 2017 07:26

thaJeztah added the impact/cli label May 5, 2017

darkowlzz mentioned this pull request Jun 12, 2017

[WIP] Add --pids-limit flag to docker update docker/cli#181

Closed

darkowlzz force-pushed the 32443-docker-update-pids-limit branch from 6ce52a4 to ecc87e3 Compare June 12, 2017 08:33

thaJeztah added impact/api impact/changelog labels Jun 15, 2017

derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

cpuguy83 force-pushed the 32443-docker-update-pids-limit branch from a7e8b80 to baec56b Compare February 1, 2019 21:49

cpuguy83 force-pushed the 32443-docker-update-pids-limit branch 2 times, most recently from 0fc8179 to 7c89e85 Compare February 1, 2019 22:08

cpuguy83 added rebuild/* labels Feb 21, 2019

thaJeztah approved these changes Feb 21, 2019

View reviewed changes

thaJeztah added rebuild/* and removed status/failing-ci Indicates that the PR in its current state fails the test suite rebuild/janky labels Feb 21, 2019

GordonTheTurtle removed the rebuild/* label Feb 21, 2019

thaJeztah added status/4-merge and removed status/2-code-review labels Feb 21, 2019

kolyshkin requested changes Feb 21, 2019

View reviewed changes

integration/container/update_linux_test.go Outdated Show resolved Hide resolved

cpuguy83 force-pushed the 32443-docker-update-pids-limit branch 3 times, most recently from b19462d to 94d9d4d Compare February 21, 2019 21:14

Add pids-limit support in docker update

74eb258

- Adds updating PidsLimit in UpdateContainer(). - Adds setting PidsLimit in toContainerResources(). Signed-off-by: Sunny Gogoi <indiasuny000@gmail.com> Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the 32443-docker-update-pids-limit branch from 94d9d4d to 74eb258 Compare February 21, 2019 22:17

thaJeztah merged commit dd94555 into moby:master Feb 23, 2019

This was referenced Feb 24, 2019

Fix container update resetting pidslimit on older API clients #38791

Merged

Add new PidsLimit options to API version history #38792

Merged

Adding support for memory swap settings for services #37872

Open

thaJeztah mentioned this pull request Mar 21, 2019

Add --pids-limit flag to docker update docker/cli#1765

Merged

plattenschieber mentioned this pull request Apr 16, 2019

Add support for --pids-limit and --ulimit options in docker update docker/for-linux#322

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add pids-limit support in docker update #32519

Add pids-limit support in docker update #32519

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add pids-limit support in docker update #32519

Add pids-limit support in docker update #32519

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!