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` support for `docker update` #31148

Merged
merged 1 commit into from Apr 10, 2017

Conversation

@yongtang
Member

yongtang commented Feb 18, 2017

- What I did

This fix tries to address the issue raised in #31032 where it was not possible to specify --cpus for docker update.

- How I did it

This fix adds --cpus support for docker update. In case both --cpus and --cpu-period/--cpu-quota have been specified, an error will be returned.

Related docs has been updated.

- How to verify it

Integration tests have been added.

- Description for the changelog

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

05-kitten-cuteness-1

This fix fixes #31032.
This fix is related to #27921, #27958.

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

/cc @crosbymichael

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Feb 21, 2017

Contributor

design looks ok, consistent with other stuff

Contributor

LK4D4 commented Feb 21, 2017

design looks ok, consistent with other stuff

@darstahl

This comment has been minimized.

Show comment
Hide comment
@darstahl

darstahl Feb 21, 2017

Contributor

Can we add resources.NanoCPUs to the check in container_windows.go to make it error out (since Windows does not support updating container resources)?

Contributor

darstahl commented Feb 21, 2017

Can we add resources.NanoCPUs to the check in container_windows.go to make it error out (since Windows does not support updating container resources)?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 22, 2017

Member

Thanks @darrenstahlmsft. The PR has been updated with NanoCPUs check in place for Windows. Please take a look.

Member

yongtang commented Feb 22, 2017

Thanks @darrenstahlmsft. The PR has been updated with NanoCPUs check in place for Windows. Please take a look.

@darstahl

This comment has been minimized.

Show comment
Hide comment
@darstahl

darstahl Feb 22, 2017

Contributor

Thanks!

Contributor

darstahl commented Feb 22, 2017

Thanks!

@allencloud

This comment has been minimized.

Show comment
Hide comment
@allencloud

allencloud Mar 7, 2017

Contributor

need a rebase @yongtang ^^

Contributor

allencloud commented Mar 7, 2017

need a rebase @yongtang ^^

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 7, 2017

Member

Thanks @allencloud. Rebased.

Member

yongtang commented Mar 7, 2017

Thanks @allencloud. Rebased.

@thaJeztah

Thanks! I left some suggestions / thoughts. Design LGTM though, so moving to code review

Show outdated Hide outdated container/container_unix.go
Show outdated Hide outdated container/container_unix.go
@@ -21,12 +21,13 @@ Usage: docker update [OPTIONS] CONTAINER [CONTAINER...]
Update configuration of one or more containers
Options:
--blkio-weight value Block IO (relative weight), between 10 and 1000
--blkio-weight uint16 Block IO (relative weight), between 10 and 1000, or 0 to disable (default 0)

This comment has been minimized.

@thaJeztah

thaJeztah Mar 16, 2017

Member

Oh, can we make it so that this just outputs int? We already describe "between 10 and 1000", so using a rather technical uint16 doesn't add much 😄

@thaJeztah

thaJeztah Mar 16, 2017

Member

Oh, can we make it so that this just outputs int? We already describe "between 10 and 1000", so using a rather technical uint16 doesn't add much 😄

This comment has been minimized.

@yongtang

yongtang Apr 6, 2017

Member

Thanks @thaJeztah. I take a look. To fix this issue may require changes in pflag part which may involve other changes. I will create another PR to address this issue.

@yongtang

yongtang Apr 6, 2017

Member

Thanks @thaJeztah. I take a look. To fix this issue may require changes in pflag part which may involve other changes. I will create another PR to address this issue.

Show outdated Hide outdated cli/command/container/update.go
--cpu-rt-period int Limit the CPU real-time period in microseconds
--cpu-rt-runtime int Limit the CPU real-time runtime in microseconds
-c, --cpu-shares int CPU shares (relative weight)
--cpus decimal Number of CPUs (default 0.000)

This comment has been minimized.

@thaJeztah

thaJeztah Mar 16, 2017

Member

can we hide the (default 0.0000) here? It's probably confusing

@thaJeztah

thaJeztah Mar 16, 2017

Member

can we hide the (default 0.0000) here? It's probably confusing

This comment has been minimized.

@thaJeztah

thaJeztah Mar 16, 2017

Member

oh, I see it's the same on docker create, so not necessarily for this PR

@thaJeztah

thaJeztah Mar 16, 2017

Member

oh, I see it's the same on docker create, so not necessarily for this PR

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 31, 2017

Member

ping @yongtang, needs a rebase and can you take care of @thaJeztah comments ? 👼 🙏

Member

vdemeester commented Mar 31, 2017

ping @yongtang, needs a rebase and can you take care of @thaJeztah comments ? 👼 🙏

Add `--cpus` support for `docker update`
This fix tries to address the issue raised in 31032 where it was
not possible to specify `--cpus` for `docker update`.

This fix adds `--cpus` support for `docker update`. In case both
`--cpus` and `--cpu-period/--cpu-quota` have been specified,
an error will be returned.

Related docs has been updated.

Integration tests have been added.

This fix fixes 31032.

This fix is related to 27921, 27958.

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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Apr 6, 2017

Member

Thanks @darrenstahlmsft. I didn't saw the other check when performing a rebase. The PR has been updated. Please take a look.

Member

yongtang commented Apr 6, 2017

Thanks @darrenstahlmsft. I didn't saw the other check when performing a rebase. The PR has been updated. Please take a look.

@mlaventure

LGTM

@johnstep

LGTM

@thaJeztah

LGTM

@thaJeztah thaJeztah merged commit 5b1cae2 into moby:master Apr 10, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32643 has succeeded
Details
janky Jenkins build Docker-PRs 41253 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1489 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12370 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1267 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 10, 2017

Member

/cc @albers @sdurrheimer for completion scripts

Member

thaJeztah commented Apr 10, 2017

/cc @albers @sdurrheimer for completion scripts

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Apr 10, 2017

Member

Thanks @thaJeztah for the review, I will take care of the --blkio-weight uint16 in the review comment at a later time.

Member

yongtang commented Apr 10, 2017

Thanks @thaJeztah for the review, I will take care of the --blkio-weight uint16 in the review comment at a later time.

@yongtang yongtang deleted the yongtang:31032-NanoCPU-update branch Apr 10, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Apr 10, 2017

Thanks @yongtang

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Apr 10, 2017

Member

@thaJeztah reopened #31877 for bash completion.

Member

albers commented Apr 10, 2017

@thaJeztah reopened #31877 for bash completion.

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#31148 from yongtang/31032-NanoCPU-update
Add `--cpus` support for `docker update`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment