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 daemon option `--default-shm-size` #29692

Merged
merged 2 commits into from Feb 1, 2017

Conversation

@yongtang
Member

yongtang commented Dec 25, 2016

- What I did

This fix fixes issue raised in #29492 where it was not possible to specify a default --default-shm-size in daemon configuration for each `docker run``.

- How I did it

The flag --default-shm-size which is reloadable, has been added to the daemon configuation.
Related docs has been updated.

- How to verify it

- Description for the changelog

Add daemon option --default-shm-size for default shm size of created containers.

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

u 968902900 274083092 fm 15 gp 0

This fix fixes #29492.

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

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang
Member

yongtang commented Dec 25, 2016

@flx42

This comment has been minimized.

Show comment
Hide comment
@flx42

flx42 Dec 25, 2016

Contributor

Thanks @yongtang, I barely had time to look at how to implement it, I'm glad you did a PR.

Contributor

flx42 commented Dec 25, 2016

Thanks @yongtang, I barely had time to look at how to implement it, I'm glad you did a PR.

Show outdated Hide outdated daemon/config_unix.go Outdated
Show outdated Hide outdated docs/reference/commandline/dockerd.md Outdated
Show outdated Hide outdated man/dockerd.8.md Outdated

@yongtang yongtang changed the title from Add daemon option `--shm-size` to Add daemon option `--default-shm-size` Dec 25, 2016

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 26, 2016

Member

CI failed

00:07:17.861 --- FAIL: TestAdjustCPUShares (0.00s)
00:07:17.861 panic: runtime error: invalid memory address or nil pointer dereference [recovered]
00:07:17.861 	panic: runtime error: invalid memory address or nil pointer dereference
00:07:17.861 [signal SIGSEGV: segmentation violation code=0x1 addr=0x3b8 pc=0x4bf1b0]
00:07:17.862 
00:07:17.862 goroutine 51 [running]:
00:07:17.862 panic(0x13e5e20, 0xc4200100a0)
00:07:17.862 	/usr/local/go/src/runtime/panic.go:500 +0x1a1
00:07:17.862 testing.tRunner.func1(0xc42033ef00)
00:07:17.862 	/usr/local/go/src/testing/testing.go:579 +0x25d
00:07:17.862 panic(0x13e5e20, 0xc4200100a0)
00:07:17.862 	/usr/local/go/src/runtime/panic.go:458 +0x243
00:07:17.862 github.com/docker/docker/daemon.(*Daemon).adaptContainerSettings(0xc420213998, 0xc420213b80, 0xc42032f201, 0x26, 0xc42032f200)
00:07:17.863 	github.com/docker/docker/daemon/_test/_obj_test/daemon_unix.go:319 +0x130
00:07:17.863 github.com/docker/docker/daemon.TestAdjustCPUShares(0xc42033ef00)
00:07:17.863 	/go/src/github.com/docker/docker/daemon/daemon_unix_test.go:34 +0x177
00:07:17.863 testing.tRunner(0xc42033ef00, 0x1696ee0)
00:07:17.864 	/usr/local/go/src/testing/testing.go:610 +0x81
00:07:17.864 created by testing.(*T).Run
00:07:17.864 	/usr/local/go/src/testing/testing.go:646 +0x2ec
00:07:17.864 FAIL	github.com/docker/docker/daemon	2.044s
Member

AkihiroSuda commented Dec 26, 2016

CI failed

00:07:17.861 --- FAIL: TestAdjustCPUShares (0.00s)
00:07:17.861 panic: runtime error: invalid memory address or nil pointer dereference [recovered]
00:07:17.861 	panic: runtime error: invalid memory address or nil pointer dereference
00:07:17.861 [signal SIGSEGV: segmentation violation code=0x1 addr=0x3b8 pc=0x4bf1b0]
00:07:17.862 
00:07:17.862 goroutine 51 [running]:
00:07:17.862 panic(0x13e5e20, 0xc4200100a0)
00:07:17.862 	/usr/local/go/src/runtime/panic.go:500 +0x1a1
00:07:17.862 testing.tRunner.func1(0xc42033ef00)
00:07:17.862 	/usr/local/go/src/testing/testing.go:579 +0x25d
00:07:17.862 panic(0x13e5e20, 0xc4200100a0)
00:07:17.862 	/usr/local/go/src/runtime/panic.go:458 +0x243
00:07:17.862 github.com/docker/docker/daemon.(*Daemon).adaptContainerSettings(0xc420213998, 0xc420213b80, 0xc42032f201, 0x26, 0xc42032f200)
00:07:17.863 	github.com/docker/docker/daemon/_test/_obj_test/daemon_unix.go:319 +0x130
00:07:17.863 github.com/docker/docker/daemon.TestAdjustCPUShares(0xc42033ef00)
00:07:17.863 	/go/src/github.com/docker/docker/daemon/daemon_unix_test.go:34 +0x177
00:07:17.863 testing.tRunner(0xc42033ef00, 0x1696ee0)
00:07:17.864 	/usr/local/go/src/testing/testing.go:610 +0x81
00:07:17.864 created by testing.(*T).Run
00:07:17.864 	/usr/local/go/src/testing/testing.go:646 +0x2ec
00:07:17.864 FAIL	github.com/docker/docker/daemon	2.044s
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 26, 2016

Member

Thanks @AkihiroSuda. The PR has been updated and the CI failure has been fixed.

@flx42 I realized that we probably could have a customized JSON unmarshaler for human readable memory size parsing (e.g., 1g, 128M, etc). With a customized JSON unmarshaler there is no need to make dramatic changes in the current implementation of daemon.configStore.

The PR has been updated with the customized JSON unmarshaler and should take care of the human readable memory size issue. Please take a look and let me know if there are any issues.

Member

yongtang commented Dec 26, 2016

Thanks @AkihiroSuda. The PR has been updated and the CI failure has been fixed.

@flx42 I realized that we probably could have a customized JSON unmarshaler for human readable memory size parsing (e.g., 1g, 128M, etc). With a customized JSON unmarshaler there is no need to make dramatic changes in the current implementation of daemon.configStore.

The PR has been updated with the customized JSON unmarshaler and should take care of the human readable memory size issue. Please take a look and let me know if there are any issues.

Show outdated Hide outdated daemon/config_unix.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 28, 2016

Member

We should try to get dockerd and docker run consistent;

  • docker run uses "string", not "bytes"
  • docker run shows 64 MB, not MiB

Not sure which one is "best" though 😄

On docker run;

--shm-size string               Size of /dev/shm, default value is 64MB

On dockerd;

--default-shm-size bytes        Set the default shm size for containers (default 64 MiB)
Member

thaJeztah commented Dec 28, 2016

We should try to get dockerd and docker run consistent;

  • docker run uses "string", not "bytes"
  • docker run shows 64 MB, not MiB

Not sure which one is "best" though 😄

On docker run;

--shm-size string               Size of /dev/shm, default value is 64MB

On dockerd;

--default-shm-size bytes        Set the default shm size for containers (default 64 MiB)
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 28, 2016

Member

Thanks @thaJeztah for the review. I made some updates to bring consistency between docker run and dockerd:

  1. The docker run --shm-size (and docker create/build --shm-size as well) has been changed to use opts.MemBytes, thus the output type would be bytes (instead of string).
    The reason to change docker run (instead of dockerd) is that applying the logic of parsing string is going to be quite challenging in dockerd when combined with config.json. I also think bytes might be a better term as the input is much clear.
    Open to suggestions if the other way is preferred.
  2. The default value of --shm-size in docker run/create/build is hidden now. The default value of dockerd is shown as 64M in the daemon.
    I realized that since we set a default value in the daemon, the client side default value actually depends on the setting of the daemon (a 0 is passed to API anyway).
    Therefore, the old default value in client side (64M) is actually irrelevant because client will only pass 0 and let daemon decide in old and new behavior.
    Because of the above, we may want to hide the default value as it is not possible to know what the default value is at the client side.

Some related docs has been updated. As the above is quite a change that impact several places, I created a separate commit in this PR. If we prefer not to move forward with the above, I could strip the second commit from the PR.

Please take a look and let me know if there are any issues.

Member

yongtang commented Dec 28, 2016

Thanks @thaJeztah for the review. I made some updates to bring consistency between docker run and dockerd:

  1. The docker run --shm-size (and docker create/build --shm-size as well) has been changed to use opts.MemBytes, thus the output type would be bytes (instead of string).
    The reason to change docker run (instead of dockerd) is that applying the logic of parsing string is going to be quite challenging in dockerd when combined with config.json. I also think bytes might be a better term as the input is much clear.
    Open to suggestions if the other way is preferred.
  2. The default value of --shm-size in docker run/create/build is hidden now. The default value of dockerd is shown as 64M in the daemon.
    I realized that since we set a default value in the daemon, the client side default value actually depends on the setting of the daemon (a 0 is passed to API anyway).
    Therefore, the old default value in client side (64M) is actually irrelevant because client will only pass 0 and let daemon decide in old and new behavior.
    Because of the above, we may want to hide the default value as it is not possible to know what the default value is at the client side.

Some related docs has been updated. As the above is quite a change that impact several places, I created a separate commit in this PR. If we prefer not to move forward with the above, I could strip the second commit from the PR.

Please take a look and let me know if there are any issues.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 3, 2017

Member

Rebased to fix merging conflict.

Member

yongtang commented Jan 3, 2017

Rebased to fix merging conflict.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 26, 2017

Contributor

@thaJeztah design lgty? flag sounds ok for me

Contributor

LK4D4 commented Jan 26, 2017

@thaJeztah design lgty? flag sounds ok for me

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 26, 2017

Member

Rebased to fix conflict.

Member

yongtang commented Jan 26, 2017

Rebased to fix conflict.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 27, 2017

Contributor

ping @docker/core-maintainers

Contributor

LK4D4 commented Jan 27, 2017

ping @docker/core-maintainers

@vdemeester

Design SGTM 🐯

yongtang added some commits Dec 25, 2016

Add daemon option --default-shm-size
This fix fixes issue raised in 29492 where it was not
possible to specify a default `--default-shm-size` in daemon
configuration for each `docker run``.

The flag `--default-shm-size` which is reloadable, has been
added to the daemon configuation.
Related docs has been updated.

This fix fixes 29492.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Update opts.MemBytes to disable default, and move `docker run/create/…
…build` to use opts.MemBytes

This fix made several updates:
1. Update opts.MemBytes so that default value will not show up.
   The reason is that in case a default value is decided by daemon,
   instead of client, we actually want to not show default value.
2. Move `docker run/create/build` to use opts.MemBytes for `--shm-size`
   This is to bring consistency between daemon and docker run
3. docs updates.

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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 27, 2017

Member

@vdemeester The PR has been rebased and now all tests passed on Jenkins.

Member

yongtang commented Jan 27, 2017

@vdemeester The PR has been rebased and now all tests passed on Jenkins.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

LGTM
ping @docker/core-maintainers

Contributor

LK4D4 commented Jan 30, 2017

LGTM
ping @docker/core-maintainers

@vdemeester

LGTM 🐸
/cc @thaJeztah for docs-review

@vdemeester

docs LGTM 🐸
/cc @thaJeztah for docs revisit

@vdemeester vdemeester merged commit 354bd4a into moby:master Feb 1, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30053 has succeeded
Details
janky Jenkins build Docker-PRs 38666 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9692 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 1, 2017

@yongtang yongtang deleted the yongtang:29492-daemon-shm-size branch Feb 1, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 2, 2017

Member

/cc @albers @sdurrheimer for completion scripts

Member

thaJeztah commented Feb 2, 2017

/cc @albers @sdurrheimer for completion scripts

@stowns

This comment has been minimized.

Show comment
Hide comment
@stowns

stowns Dec 1, 2017

this is listed as a valid arg for the daemon.json https://docs.docker.com/engine/reference/commandline/dockerd/.
And it's listed as being included in the 17.04.0ce release notes https://docs.docker.com/release-notes/docker-ce/#17040-ce-2017-04-05. However, attempting to add it via Docker for Mac 17.09.0-ce results in

screen shot 2017-12-01 at 2 36 40 pm

screen shot 2017-12-01 at 2 39 46 pm

stowns commented Dec 1, 2017

this is listed as a valid arg for the daemon.json https://docs.docker.com/engine/reference/commandline/dockerd/.
And it's listed as being included in the 17.04.0ce release notes https://docs.docker.com/release-notes/docker-ce/#17040-ce-2017-04-05. However, attempting to add it via Docker for Mac 17.09.0-ce results in

screen shot 2017-12-01 at 2 36 40 pm

screen shot 2017-12-01 at 2 39 46 pm

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 1, 2017

Member

@stowns can you open an issue for that in the Docker for Mac issue tracker? The Docker for Mac UI has its own validation of the daemon.json file, and it looks like it wasn't updated to recognise the new option. The issue tracker for that is here: https://github.com/docker/for-mac/issues

To work around that problem, you can edit the daemon.json directly: on Docker for Mac, you can find the file in ~/.docker/daemon.json

Member

thaJeztah commented Dec 1, 2017

@stowns can you open an issue for that in the Docker for Mac issue tracker? The Docker for Mac UI has its own validation of the daemon.json file, and it looks like it wasn't updated to recognise the new option. The issue tracker for that is here: https://github.com/docker/for-mac/issues

To work around that problem, you can edit the daemon.json directly: on Docker for Mac, you can find the file in ~/.docker/daemon.json

@stowns

This comment has been minimized.

Show comment
Hide comment
@stowns

stowns commented Dec 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment