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

Fix honoring tmpfs-size for user /dev/shm mount #35316

Merged
merged 2 commits into from Nov 14, 2017

Conversation

Projects
None yet
8 participants
@kolyshkin
Contributor

kolyshkin commented Oct 27, 2017

- What I did
Commit 7120976 ("Implement none, private, and shareable ipc modes") introduces a bug: if a user-specified mount for /dev/shm is provided, its size is overridden by value of ShmSize. This was reported in #35271.

This commit is an attempt to fix this, as well as optimize things a but and make the code easier to read.

- How I did it
Appending the 'size' argument to /dev/shm mount options moved to an earlier place in the function, where we deal with the mounts that came from spec, therefore a user mount is left intact.

- How to verify it

There is a test case included in this PR. In addition, it can easily be checked manually:

 docker run --rm \
	--mount type=tmpfs,dst=/dev/shm,tmpfs-size=100K \
	alpine df /dev/shm

Expected output (i.e. if there is no bug):

Filesystem           1K-blocks      Used Available Use% Mounted on
tmpfs                      100         0       100   0% /dev/shm

(i.e. 100 should be shown under the "1K-blocks" header)

- Description for the changelog
Fix honoring tmpfs-size for user /dev/shm mount

- A picture of a cute animal (not mandatory but encouraged)
shy-beaver
Shy beaver | Minnesota zoo (source)

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Oct 27, 2017

Member

could you add a test?

Member

AkihiroSuda commented Oct 27, 2017

could you add a test?

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 8, 2017

Contributor

could you add a test?

What stops me is lack of client.ContainerExec functionality in integration/ tests, something similar to cli.DockerCmd("exec", ...) in integration-cli/. Guess I need to write it first.

Contributor

kolyshkin commented Nov 8, 2017

could you add a test?

What stops me is lack of client.ContainerExec functionality in integration/ tests, something similar to cli.DockerCmd("exec", ...) in integration-cli/. Guess I need to write it first.

@kolyshkin kolyshkin requested review from dnephin and vdemeester as code owners Nov 8, 2017

@dnephin

Would it be possible to test this with a unit test instead of an integration that? The fix seems to be in a single function, so having to make multiple API calls seems like it's maybe not the easiest way to reproduce and test the issue.

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 8, 2017

Contributor

@AkihiroSuda @thaJeztah I have added the test case, and made sure it is failing without the fix (and passing with the fix applied).

Contributor

kolyshkin commented Nov 8, 2017

@AkihiroSuda @thaJeztah I have added the test case, and made sure it is failing without the fix (and passing with the fix applied).

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 8, 2017

Contributor

Would it be possible to test this with a unit test instead of an integration that?

Problem is, there's a lot of code dealing with mounts, and the setMounts() is not very isolated function. If I am to write a unit test, I have to repeat some of that code in the test case which is prone to error later if this code changes.

Anyway, let me give it a try and see what I end up with.

Contributor

kolyshkin commented Nov 8, 2017

Would it be possible to test this with a unit test instead of an integration that?

Problem is, there's a lot of code dealing with mounts, and the setMounts() is not very isolated function. If I am to write a unit test, I have to repeat some of that code in the test case which is prone to error later if this code changes.

Anyway, let me give it a try and see what I end up with.

Fix user mount /dev/shm size
Commit 7120976 ("Implement none, private, and shareable ipc
modes") introduces a bug: if a user-specified mount for /dev/shm
is provided, its size is overriden by value of ShmSize.

A reproducer is simple:

 docker run --rm
	--mount type=tmpfs,dst=/dev/shm,tmpfs-size=100K \
	alpine df /dev/shm

This commit is an attempt to fix the bug, as well as optimize things
a but and make the code easier to read.

#35271

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 13, 2017

Contributor

@dnephin OK, I managed to write a unit test. PTAL once time permits.

Contributor

kolyshkin commented Nov 13, 2017

@dnephin OK, I managed to write a unit test. PTAL once time permits.

@dnephin

Thanks, test LGTM, just one small nit

Show outdated Hide outdated daemon/daemon_linux_test.go
@vdemeester

LGTM 🐯

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 13, 2017

Member

Looks good, but haven't given it a spin yet; let me check if the --tmpfs /dev/shm:rw,nosuid,nodev,size=8 option (more options other than just size) works as expected.

Member

thaJeztah commented Nov 13, 2017

Looks good, but haven't given it a spin yet; let me check if the --tmpfs /dev/shm:rw,nosuid,nodev,size=8 option (more options other than just size) works as expected.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Nov 13, 2017

Collaborator

@thaJeztah let us know :D

Collaborator

vieux commented Nov 13, 2017

@thaJeztah let us know :D

@thaJeztah

Doh! I see I mixed up your PR's, had the other PR (#35467) in mind when I commented.

this LGTM, thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 14, 2017

Member

Oh, looks like there's one linting failure:

22:04:28 daemon/daemon_linux_test.go:156:5:warning: should omit comparison to bool constant, can be simplified to !found (S1002) (gosimple)
22:04:28 Build step 'Execute shell' marked build as failure
Member

thaJeztah commented Nov 14, 2017

Oh, looks like there's one linting failure:

22:04:28 daemon/daemon_linux_test.go:156:5:warning: should omit comparison to bool constant, can be simplified to !found (S1002) (gosimple)
22:04:28 Build step 'Execute shell' marked build as failure
integration: test case for #35271
This test case is checking that the built-in default size for /dev/shm
(which is used for `--ipcmode` being `private` or `shareable`)
is not overriding the size of user-defined tmpfs mount for /dev/shm.

In other words, this is a regression test case for issue #35271,
#35271

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

@vdemeester vdemeester merged commit f70c715 into moby:master Nov 14, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37832 has succeeded
Details
janky Jenkins build Docker-PRs 46543 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6957 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18101 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6761 has succeeded
Details

@kolyshkin kolyshkin deleted the kolyshkin:facepalm branch Nov 18, 2017

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