Add `--cpus` flag to control cpu resources by yongtang · Pull Request #27958 · moby/moby

yongtang · 2016-11-01T18:11:49Z

- What I did

This fix tries to address the proposal raised in #27921 and add --cpus flag for docker run/create.

- How I did it

Basically, --cpus will allow user to specify a number (possibly partial) about how many CPUs the container will use. For example, on a 2-CPU system --cpus 1.5 means the container will take 75% (1.5/2) of the CPU share.

This fix adds a NanoCPUs field to HostConfig since swarmkit alreay have a concept of NanoCPUs for tasks. The --cpus flag will translate the number into reused NanoCPUs to be consistent with swarmkit.

Related docs (docker run and Remote APIs) have been updated.

- How to verify it

This fix adds integration tests to cover the changes.

- Description for the changelog

Add --cpus flag to control cpu resources for docker run/create, and add NanoCPUs to HostConfig.

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

This fix fixes #27921.

Signed-off-by: Yong Tang [email protected]

crosbymichael · 2016-11-01T18:31:20Z

Thanks for doing this.

I'm not sure I want to do any of the math on the client side. I was thinking just passing the raw float from the client and doing the calculations on the server. I'll look at your implementation more and think about it a little more if we should use swarms nanocpus as what we transfer or not.

ping @stevvooe @mlaventure wdyt?

mlaventure · 2016-11-01T18:48:10Z

@crosbymichael, I had a quick look, it does seem that @yongtang correctly put the computation on the daemon side.

I'm not familiar with the way Swarm handle its scheduling though, I am pretty neutral on that side.

crosbymichael

LGTM

stevvooe · 2016-11-01T22:21:26Z

docs/reference/api/docker_remote_api_v1.25.md

NanoCpus is a fixed-point number. While scientific notation works fine for fixed point, others may infer that this field is a float. May want to use an integer example instead.

Thanks @stevvooe. The docs has been updated.

stevvooe · 2016-11-01T22:22:19Z

docs/reference/api/docker_remote_api.md

We don't limit this parameter to CFS. I think we can generalize this to any time sharing CPU scheduling. I don't have a suggestion for wording.

Just removing the CFS reference gives a good enough wording, doesn't it?

its implemented via the cfs scheduler but for different platforms this could be different. I would just remove the last CPU shares part as that confuses this feature with cpu-shares

Thanks @crosbymichael @mlaventure @stevvooe. The wording has been changed to:

The HostConfig field now includes NanoCPUs that represents CPU quota
in units of 10^-9 CPUs.

stevvooe · 2016-11-01T22:23:13Z

Confirmed calculations against swarmkit and docker executor.

mlaventure · 2016-11-01T22:43:18Z

@stevvooe Any reason why this computation is duplicated? Sounds like it should be in a utils package of some sort.

stevvooe · 2016-11-02T00:40:23Z

@mlaventure Only because "a little copying is better than a little dependency". We'd like to share more code in swarmkit/agent/exec/container and docker/daemon/executor but don't want to start that without a plan.

cpuguy83 · 2016-11-02T01:07:43Z

Moved to code review since it seems like this is good.

cpuguy83 · 2016-11-02T01:05:05Z

daemon/daemon_unix.go

I know for other options we warn, but I feel like we should error out when we can't set this.

Thanks @cpuguy83. The warning has been changed to error and PR has been updated.

coolljt0725

LGTM, but a typo

coolljt0725 · 2016-11-03T07:28:59Z

integration-cli/docker_cli_run_unix_test.go

one nit, typo here shoudl

yongtang · 2016-11-03T13:56:57Z

Thanks @coolljt0725. The PR has been updated with typo fixed.

cpuguy83 · 2016-11-03T18:39:01Z

LGTM but needs a rebase, moving to docs.

yongtang · 2016-11-03T18:50:21Z

Thanks @cpuguy83. The PR has been rebased.

cpuguy83

LGTM

stevvooe · 2016-11-03T18:58:02Z

runconfig/opts/parse.go

There is a type called nanoCPUs that can be used here. https://github.com/docker/docker/blob/e93f84a48bb394ed181aa48d9400922b292eb15a/cli/command/service/opts.go#L43

@stevvooe In #27921 the flag --cpus was specified as a float number for the number (could be partial) of cpus (@crosbymichael). Think we need a float here?

This is not a float. It is a fixed point fractional number. This should be the same as --limit-cpu in docker service create/update.

@stevvooe I see. Thanks. Let me update the PR shortly.

@stevvooe The PR has been updated. Please take a look and let me know for any issues.

thaJeztah · 2016-11-04T21:16:50Z

@mlaventure @stevvooe I was just discussing with @crosbymichael and realized this is the same thing as --limit-cpu on docker service create.

For consistency, I think it would make sense to rename the flag so that it matches that WDYT?

tiborvass · 2016-11-04T21:52:03Z

@thaJeztah what about deprecating --limit-cpu and have --cpu instead? Shorter.

thaJeztah · 2016-11-04T22:16:40Z

@tiborvass both could work, although we have --limit-memory as well. They are quite descriptive, but I agree it's shorter 😄

albers · 2016-11-05T08:25:56Z

I'm in favour of the descriptive variants. Because of their common prefix, they appear grouped in help output, documentation and completions.

thaJeztah · 2016-11-05T17:25:17Z

@albers @tiborvass I opened #28095

sdurrheimer · 2016-11-06T11:26:58Z

@thaJeztah Will do when we will have finished with container, image and system subcommands.

Follow up docs update for PR #27958

Pull request moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm` - moby#27702 added `--network` to `docker build` - moby#25962 added `--attachable` to `docker network create` - moby#27998 added `--compose-file` to `docker stack deploy` - moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby#26061 added `--init` to `docker run` and `docker create` - moby#26941 added `--init-path` to `docker run` and `docker create` - moby#27958 added `--cpus` on `docker run` / `docker create` - moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby#27596 added `--force` to `docker service update` - moby#27857 added `--hostname` to `docker service create` - moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby#28076 added `--tty` on `docker service create` / `docker service update` - moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <[email protected]>

vincentwoo · 2017-02-15T01:03:26Z

Hi everyone, any chance we could add NanoCPUs to the container update endpoint as well? Right now container updating supports setting CpuPeriod and CpuQuota already. If --cpus or NanoCPUs are the new golden-path option for setting these options, I think they should be present in update as well!

thaJeztah · 2017-02-15T06:29:06Z

@vincentwoo I think that could make sense (as we support other resource options for docker update). Can you open an issue for it?

Pull request moby/moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm` - moby/moby#27702 added `--network` to `docker build` - moby/moby#25962 added `--attachable` to `docker network create` - moby/moby#27998 added `--compose-file` to `docker stack deploy` - moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby/moby#26061 added `--init` to `docker run` and `docker create` - moby/moby#26941 added `--init-path` to `docker run` and `docker create` - moby/moby#27958 added `--cpus` on `docker run` / `docker create` - moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby/moby#27596 added `--force` to `docker service update` - moby/moby#27857 added `--hostname` to `docker service create` - moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby/moby#28076 added `--tty` on `docker service create` / `docker service update` - moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db Component: cli

GordonTheTurtle added the status/0-triage label Nov 1, 2016

yongtang mentioned this pull request Nov 1, 2016

Add --cpus flag to control cpu resources #27921

Open

yongtang force-pushed the 27921-cpus branch 3 times, most recently from a9a3fc3 to 17ec99c Compare November 1, 2016 18:29

crosbymichael approved these changes Nov 1, 2016

View reviewed changes

stevvooe reviewed Nov 1, 2016

View reviewed changes

yongtang force-pushed the 27921-cpus branch from 17ec99c to 78a280b Compare November 1, 2016 23:55

cpuguy83 added status/2-code-review and removed status/0-triage labels Nov 2, 2016

cpuguy83 requested changes Nov 2, 2016

View reviewed changes

yongtang force-pushed the 27921-cpus branch from 78a280b to fe92f2f Compare November 2, 2016 02:08

coolljt0725 reviewed Nov 3, 2016

View reviewed changes

integration-cli/docker_cli_run_unix_test.go Outdated

Copy link

Contributor

coolljt0725 Nov 3, 2016

Choose a reason for hiding this comment

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

one nit, typo here shoudl

yongtang force-pushed the 27921-cpus branch from fe92f2f to dc002d1 Compare November 3, 2016 13:56

thaJeztah added this to the 1.13.0 milestone Nov 3, 2016

cpuguy83 added status/3-docs-review and removed status/2-code-review labels Nov 3, 2016

yongtang force-pushed the 27921-cpus branch from dc002d1 to 7b6dcf0 Compare November 3, 2016 18:49

cpuguy83 approved these changes Nov 3, 2016

View reviewed changes

stevvooe reviewed Nov 3, 2016

View reviewed changes

yongtang mentioned this pull request Nov 4, 2016

Follow up docs update for PR #27958 #28075

Merged

albers mentioned this pull request Nov 5, 2016

Add bash completion for docker {run,create} --cpus #28093

Merged

thaJeztah mentioned this pull request Nov 5, 2016

Rename docker run "--cpus" to "--limit-cpu" #28095

Closed

vdemeester added a commit that referenced this pull request Nov 7, 2016

Merge pull request #28075 from yongtang/27958-docs-follow-up

b814024

Follow up docs update for PR #27958

sdurrheimer mentioned this pull request Nov 10, 2016

Add zsh completion for 'docker {run,create} --cpus' #28254

Merged

This was referenced Dec 20, 2016

How to provide cpu-quota and cpu-period values while creating a service in swarm mode? #29579

Closed

[epic] add more options to service create / service update #25303

Open

thaJeztah added impact/api impact/changelog impact/cli labels Jan 16, 2017

thaJeztah mentioned this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13 #30186

Merged

vincentwoo mentioned this pull request Feb 8, 2017

Document new CPU scheduling flags docker/docs#586

Merged

vincentwoo mentioned this pull request Feb 15, 2017

Please support NanoCPUs option in container update API endpoint #31032

Closed

yongtang mentioned this pull request Feb 18, 2017

Add --cpus support for docker update #31148

Merged

neolit123 mentioned this pull request Jul 9, 2020

kubelet fails to create kubepods cgroup on larger machine with 512 processors with systemd CgroupDriver kubernetes/kubernetes#92855

Closed

thaJeztah mentioned this pull request Jan 12, 2021

Docker Engine API - docker inspect NanoCPUs response name capitalized differently than docs and prev API version #41868

Closed

This was referenced Feb 10, 2021

api/docs: fix NanoCPUs casing in swagger #42009

Merged

[20.10 backport] api/docs: fix NanoCPUs casing in swagger #42012

Merged

XK4MiLX mentioned this pull request May 8, 2023

Run docker containers with the --cpu limits RunOnFlux/flux#1001

Closed

Comments

Conversation

yongtang commented Nov 1, 2016

Uh oh!

crosbymichael commented Nov 1, 2016

Uh oh!

mlaventure commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael left a comment

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!

stevvooe commented Nov 1, 2016

Uh oh!

mlaventure commented Nov 1, 2016

Uh oh!

stevvooe commented Nov 2, 2016

Uh oh!

cpuguy83 commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolljt0725 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yongtang commented Nov 3, 2016

Uh oh!

cpuguy83 commented Nov 3, 2016

Uh oh!

yongtang commented Nov 3, 2016

Uh oh!

cpuguy83 left a comment

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!

thaJeztah commented Nov 4, 2016

Uh oh!

tiborvass commented Nov 4, 2016

Uh oh!

thaJeztah commented Nov 4, 2016

Uh oh!

albers commented Nov 5, 2016

Uh oh!

thaJeztah commented Nov 5, 2016

Uh oh!

sdurrheimer commented Nov 6, 2016

Uh oh!

vincentwoo commented Feb 15, 2017

Uh oh!

thaJeztah commented Feb 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

mlaventure commented Nov 1, 2016 •

edited

Loading