-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Conversation
a9a3fc3
to
17ec99c
Compare
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? |
@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. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Confirmed calculations against swarmkit and docker executor. |
@stevvooe Any reason why this computation is duplicated? Sounds like it should be in a |
@mlaventure Only because "a little copying is better than a little dependency". We'd like to share more code in |
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.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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")) |
There was a problem hiding this comment.
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
Thanks @coolljt0725. The PR has been updated with typo fixed. |
LGTM but needs a rebase, moving to docs. |
Thanks @cpuguy83. The PR has been rebased. |
There was a problem hiding this 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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a type called nanoCPUs
that can be used here. https://github.com/docker/docker/blob/e93f84a48bb394ed181aa48d9400922b292eb15a/cli/command/service/opts.go#L43
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@mlaventure @stevvooe I was just discussing with @crosbymichael and realized this is the same thing as For consistency, I think it would make sense to rename the flag so that it matches that WDYT? |
@thaJeztah what about deprecating |
@tiborvass both could work, although we have |
I'm in favour of the descriptive variants. Because of their common prefix, they appear grouped in help output, documentation and completions. |
@albers @tiborvass I opened #28095 |
@thaJeztah Will do when we will have finished with |
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 <github@gone.nl>
Hi everyone, any chance we could add |
@vincentwoo I think that could make sense (as we support other resource options for |
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
- What I did
This fix tries to address the proposal raised in #27921 and add
--cpus
flag fordocker 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 toHostConfig
since swarmkit alreay have a concept of NanoCPUs for tasks. The--cpus
flag will translate the number into reusedNanoCPUs
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 fordocker run/create
, and addNanoCPUs
toHostConfig
.- A picture of a cute animal (not mandatory but encouraged)
This fix fixes #27921.
Signed-off-by: Yong Tang yong.tang.github@outlook.com