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.

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

Add --cpus flag to control cpu resources #27958

Merged
merged 1 commit into from Nov 4, 2016

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Nov 1, 2016

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

cute-animals-illu-002

This fix fixes #27921.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@crosbymichael
Copy link
Contributor

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
Copy link
Contributor

mlaventure commented Nov 1, 2016

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

Copy link
Contributor

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -302,6 +302,7 @@ Create a container
"MemorySwap": 0,
"MemoryReservation": 0,
"KernelMemory": 0,
"NanoCPU": 5e+08,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @stevvooe. The docs has been updated.

@@ -163,6 +163,7 @@ This section lists each version from latest to oldest. Each listing includes a
* Every API response now includes a `Docker-Experimental` header specifying if experimental features are enabled (value can be `true` or `false`).
* The `hostConfig` option now accepts the fields `CpuRealtimePeriod` and `CpuRtRuntime` to allocate cpu runtime to rt tasks when `CONFIG_RT_GROUP_SCHED` is enabled in the kernel.
* The `SecurityOptions` field within the `GET /info` response now includes `userns` if user namespaces are enabled in the daemon.
* The `HostConfig` field now includes `NanoCPUs` that represents CPU CFS (Completely Fair Scheduler) quota in units of 10<sup>-9</sup> CPU shares.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

stevvooe commented Nov 1, 2016

Confirmed calculations against swarmkit and docker executor.

@mlaventure
Copy link
Contributor

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

@stevvooe
Copy link
Contributor

stevvooe commented Nov 2, 2016

@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
Copy link
Member

cpuguy83 commented Nov 2, 2016

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

}
if resources.NanoCPUs > 0 && (!sysInfo.CPUCfsPeriod || !sysInfo.CPUCfsQuota) {
warnings = append(warnings, "Your kernel does not support CPU cfs period/quota or the cgroup is not mounted. NanoCPUs discarded.")
logrus.Warn("Your kernel does not support CPU cfs period/quota or the cgroup is not mounted. NanoCPUs discarded.")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@coolljt0725 coolljt0725 left a comment

Choose a reason for hiding this comment

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

LGTM, but a typo

out = inspectField(c, "test", "HostConfig.CpuQuota")
c.Assert(out, checker.Equals, "0", check.Commentf("CPU CFS quota should be 0"))
out = inspectField(c, "test", "HostConfig.CpuPeriod")
c.Assert(out, checker.Equals, "0", check.Commentf("CPU CFS period shoudl be 0"))
Copy link
Contributor

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
Copy link
Member Author

yongtang commented Nov 3, 2016

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

@thaJeztah thaJeztah added this to the 1.13.0 milestone Nov 3, 2016
@cpuguy83
Copy link
Member

cpuguy83 commented Nov 3, 2016

LGTM but needs a rebase, moving to docs.

@yongtang
Copy link
Member Author

yongtang commented Nov 3, 2016

Thanks @cpuguy83. The PR has been rebased.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -232,6 +233,7 @@ func AddFlags(flags *pflag.FlagSet) *ContainerOptions {
flags.Int64Var(&copts.cpuRealtimePeriod, "cpu-rt-period", 0, "Limit CPU real-time period in microseconds")
flags.Int64Var(&copts.cpuRealtimeRuntime, "cpu-rt-runtime", 0, "Limit CPU real-time runtime in microseconds")
flags.Int64VarP(&copts.cpuShares, "cpu-shares", "c", 0, "CPU shares (relative weight)")
flags.Float64Var(&copts.cpus, "cpus", 0.0, "Number of CPUs")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@thaJeztah
Copy link
Member

@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
Copy link
Contributor

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

@thaJeztah
Copy link
Member

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

@albers
Copy link
Member

albers commented Nov 5, 2016

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

@thaJeztah
Copy link
Member

@albers @tiborvass I opened #28095

@sdurrheimer
Copy link
Contributor

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

vdemeester added a commit that referenced this pull request Nov 7, 2016
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jan 16, 2017
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 <github@gone.nl>
@vincentwoo
Copy link
Contributor

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
Copy link
Member

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

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request May 19, 2017
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 <github@gone.nl>
Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --cpus flag to control cpu resources