Fix honoring tmpfs-size for user /dev/shm mount by kolyshkin · Pull Request #35316 · moby/moby

kolyshkin · 2017-10-27T08:43:46Z

- 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 | Minnesota zoo (source)

AkihiroSuda · 2017-10-27T22:19:06Z

could you add a test?

kolyshkin · 2017-11-08T00:39:41Z

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.

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 · 2017-11-08T19:46:20Z

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

kolyshkin · 2017-11-08T23:23:07Z

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.

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. moby#35271 Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2017-11-13T05:44:23Z

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

AkihiroSuda · 2017-11-13T06:01:30Z

you can use assert.Truef
https://godoc.org/github.com/stretchr/testify/assert#Truef

I think this is one of the few times when a plain t.Fatal() is actually the best option:

t.Fatal("size option was missing from mount")

updated to use f.Fatal()

dnephin

Thanks, test LGTM, just one small nit

dnephin · 2017-11-13T17:10:47Z

I think this is one of the few times when a plain t.Fatal() is actually the best option:

t.Fatal("size option was missing from mount")

vdemeester

LGTM 🐯

thaJeztah · 2017-11-13T18:47:00Z

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 · 2017-11-13T23:12:48Z

@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 · 2017-11-14T01:00:20Z

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

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 moby#35271, moby#35271 Signed-off-by: Kir Kolyshkin <[email protected]>

GordonTheTurtle added the status/0-triage label Oct 27, 2017

thaJeztah added status/2-code-review and removed status/0-triage labels Oct 27, 2017

kolyshkin force-pushed the facepalm branch from ad38e24 to 45624a7 Compare November 8, 2017 19:20

kolyshkin requested review from dnephin and vdemeester as code owners November 8, 2017 19:20

dnephin reviewed Nov 8, 2017

View reviewed changes

kolyshkin force-pushed the facepalm branch from 45624a7 to 12619e0 Compare November 8, 2017 19:44

thaJeztah mentioned this pull request Nov 8, 2017

Enable 'exec' on /dev/shm, without --privileged #6758

Closed

GordonTheTurtle assigned johnstep Nov 11, 2017

kolyshkin force-pushed the facepalm branch from 12619e0 to 9f3ccbd Compare November 13, 2017 05:43

AkihiroSuda reviewed Nov 13, 2017

View reviewed changes

dnephin approved these changes Nov 13, 2017

View reviewed changes

vdemeester approved these changes Nov 13, 2017

View reviewed changes

kolyshkin force-pushed the facepalm branch from 9f3ccbd to 9531575 Compare November 13, 2017 21:49

thaJeztah approved these changes Nov 14, 2017

View reviewed changes

kolyshkin force-pushed the facepalm branch from 9531575 to 2e0a98b Compare November 14, 2017 06:08

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

kolyshkin deleted the facepalm branch November 18, 2017 01:24

kolyshkin mentioned this pull request Nov 18, 2017

[17.10] Fix honoring tmpfs-size for user /dev/shm mount docker-archive/docker-ce#324

Closed

This was referenced Nov 18, 2017

[17.11] Fix honoring tmpfs-size for user /dev/shm mount docker-archive/docker-ce#325

Closed

[17.09] Fix honoring tmpfs-size for user /dev/shm mount docker-archive/docker-ce#326

Merged

Fix "duplicate mount point" when --tmpfs /dev/shm is used #35467

Merged

thaJeztah mentioned this pull request Feb 23, 2018

tmpfs-size not working for service mount #35271

Closed

Conversation

kolyshkin commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Oct 27, 2017

Uh oh!

kolyshkin commented Nov 8, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Nov 8, 2017

Uh oh!

kolyshkin commented Nov 8, 2017

Uh oh!

kolyshkin commented Nov 13, 2017

Uh oh!

AkihiroSuda Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

kolyshkin Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 13, 2017

Uh oh!

vieux commented Nov 13, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kolyshkin commented Oct 27, 2017 •

edited

Loading