api: add TypeTmpfs to api/types/mount by AkihiroSuda · Pull Request #26837 · moby/moby

AkihiroSuda · 2016-09-22T22:03:20Z

- What I did
added TypeTmpfs to api/types/mount.

TODO: migrate Swarmkit and CLI to use the new mount API

- How I did it

- How to verify it
TESTFLAGS='-check.f TestContainersApiCreateMounts' make test-integration-cli

- Description for the changelog

api: add TypeTmpfs to api/types/mount

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

Signed-off-by: Akihiro Suda [email protected]

AkihiroSuda · 2016-09-22T22:03:27Z

CC @cpuguy83

justincormack · 2016-09-24T08:42:31Z

cc @stevvooe

cpuguy83 · 2016-09-24T11:36:46Z

container/container_unix.go

We should have these added to container.Mountpoints now that we have a proper "Type" field.... it would also be helpful to have HostConfig.Tmpfs converted into this format like we are doing (daemon side) for stuff in Binds and Volumes.

cpuguy83 · 2016-09-24T11:39:47Z

api/types/mount/mount.go

:) We should just group these into a single comment above the const block describing what they are for rather than having these redundant doc strings.

cpuguy83 · 2016-09-24T11:45:50Z

api/types/mount/mount.go

I think swarmkit is using Mode here.

type Mount_TmpfsOptions struct { // Size sets the size of the tmpfs, in bytes. // // This will be converted to an operating system specific value // depending on the host. For example, on linux, it will be convered to // use a 'k', 'm' or 'g' syntax. BSD, though not widely supported with // docker, uses a straight byte value. // // Percentages are not supported. SizeBytes int64 `protobuf:"varint,1,opt,name=size_bytes,json=sizeBytes,proto3" json:"size_bytes,omitempty"` // Mode of the tmpfs upon creation Mode os.FileMode `protobuf:"varint,2,opt,name=mode,proto3,customtype=os.FileMode" json:"mode"` }

Yes, please no more "whatever goes in here, good luck" fields.

ok, I'm going to change the struct definition to like this:

// TmpfsOptions defines options specific to mounts of type "tmpfs". type TmpfsOptions struct { // Size sets the size of the tmpfs, in bytes. // // This will be converted to an operating system specific value // depending on the host. For example, on linux, it will be convered to // use a 'k', 'm' or 'g' syntax. BSD, though not widely supported with // docker, uses a straight byte value. // // Percentages are not supported. SizeBytes int64 // Mode of the tmpfs upon creation Mode os.FileMode // RawOptions is the raw string passed to mount(2). // e.g. "rw,noexec,nosuid,size=65536k" // // RawOptions is exclusive to other TmpfsOptions fields. // If RawOptions has a non-empty value, other fields must have empty values. // // RawOptions can contain "ro" or "rw" but needs to be consistent with // Mount.ReadOnly RawOptions string `json:",omitempty"` }

I think we need to keep RawOptions for compatibility, WDYT?

now TmpfsOptions is identical to the corresponding one of Swarmkit

stevvooe · 2016-09-26T19:24:54Z

integration-cli/docker_api_containers_test.go

Could we format these to make them readable?

stevvooe · 2016-09-26T19:25:24Z

integration-cli/docker_api_containers_test.go

Is tmpfs linux only? Windows does have ramdisk.

AFAIK yes, linux only in the current implementation?
https://github.com/docker/docker/blob/b8265e55508db99aea632033b2f5008f921b11e2/container/container_windows.go#L89-L93

IMO implementation for Windows should be another PR, but I understand we need to design the API with consideration for Windows in this PR.

CC @jhowardmsft

stevvooe · 2016-09-26T19:25:49Z

integration-cli/docker_api_containers_test.go

TestContainersAPICreateMountsTmpfs.

Please follow Go's naming conventions.

It seems there is some project-local naming convention that encourages Test.*Api.*?
If not, perhaps we need to fix that in separate PR?

$ grep "func (.*Suite) Test.*Api" integration-cli/*.go | wc -l 158 $ grep "func (.*Suite) Test.*API" integration-cli/*.go | wc -l 2

Agreed; better done in a separate PR if we want to change that

@thaJeztah This is a new test. It should follow our coding standards.

Demonstrations of others not following the standards does not make it okay to not follow the standards.

ok, and opened PR for existing tests: #26967

AkihiroSuda · 2016-09-27T08:46:12Z

updated PR, please review again if CI green?
@cpuguy83 @stevvooe

AkihiroSuda · 2016-09-27T08:52:50Z

daemon/volumes.go

forgot to fix this, now fixing..

AkihiroSuda · 2016-09-27T10:50:26Z

Failure in windowsRS1 is trivial, I'll make sure it is skipped on Windows

00:48:59.244 ----------------------------------------------------------------------
00:48:59.245 FAIL: docker_api_containers_test.go:1525: DockerSuite.TestContainersApiCreateMountsValidation
00:48:59.245 
00:48:59.245 case 0
00:48:59.245 case 1
00:48:59.245 case 2
00:48:59.245 case 3
00:48:59.245 case 4
00:48:59.245 case 5
00:48:59.245 case 6
00:48:59.245 case 7
00:48:59.245 docker_api_containers_test.go:1711:
00:48:59.245     c.Assert(status, checker.Equals, x.status, check.Commentf("%s\n%v", string(b), cases[i].config))
00:48:59.245 ... obtained int = 400
00:48:59.245 ... expected int = 201
00:48:59.246 ... {"message":"invalid mount config for type \"tmpfs\": Windows does not support tmpfs"}
00:48:59.246 
00:48:59.246 {busybox {[{tmpfs  c:\foo false <nil> <nil> <nil>}]}}
00:48:59.246 
00:48:59.395 
00:48:59.395 ----------------------------------------------------------------------

stevvooe · 2016-09-26T19:26:11Z

container/container_unix.go

Why would there be a source here?

It is from the original source https://github.com/docker/docker/pull/26837/files#diff-5dfdabc4cd5100a2aee949c12d2afe6cL413

stevvooe · 2016-09-26T19:27:07Z

api/types/mount/mount.go

Yes, please no more "whatever goes in here, good luck" fields.

AkihiroSuda · 2016-09-28T03:12:21Z

updated PR: removed RawOptions from api/types/mount.TmpfsOptions

stevvooe · 2016-10-03T15:17:28Z

api/types/mount/mount.go

-	// TypeBind BIND
-	TypeBind Type = "bind"
-	// TypeVolume VOLUME
+	TypeBind   Type = "bind"


Rather than remove documentation, let's actually add it in. You can copy from swarmkit.

stevvooe · 2016-10-03T15:17:54Z

api/types/mount/mount.go

 type Mount struct {
-	Type     Type   `json:",omitempty"`
+	Type Type `json:",omitempty"`
+	// Source is not supported in TMPFS (must be an empty value)


There is more to Source than just that. This is incomplete.

stevvooe · 2016-10-03T15:19:07Z

api/types/mount/mount.go

+	// Mode of the tmpfs upon creation
+	Mode os.FileMode
+
+	UID          uint32


How can any of this possibly be supported if we don't have support in swarmkit? https://github.com/docker/swarmkit/blob/master/api/types.proto#L191

they are for non-swarm containers, and imo they are still needed for compatibility; old tmpfs allowed these options

WDYT?

These were not supported with tmpfs previously. Do not add them in this PR. You can have a similar comment as I have in swarmkit, but please don't add things that aren't actually supportable.

It seems supported it in Docker 1.12.2, WDYT?

$ docker run -it --rm --tmpfs /foo:uid=4242,nr_inodes=3 busybox / # ls -ld /foo drwxrwxrwt 2 4242 root 40 Oct 14 05:10 /foo / # touch /foo/1 / # touch /foo/2 / # touch /foo/3 touch: /foo/3: No space left on device

They are not currently supported for swarmkit. I am not sure what to say about docker run. These options are in no way portable.

OK, I removed them in the latest commit

stevvooe · 2016-10-03T15:19:52Z

volume/volume_unix.go

+	}
+	if opt.INodes != 0 {
+		switch os {
+		case "linux":


Use build tags to do this.

removed the code itself

stevvooe · 2016-10-03T15:20:09Z

volume/volume_unix_test.go

+	mounttypes "github.com/docker/docker/api/types/mount"
+)
+
+func TestRawTmpfsOptions(t *testing.T) {


There is no more raw...

AkihiroSuda · 2016-10-04T09:17:37Z

volume/volume_unix.go

+		rawOpts = append(rawOpts, fmt.Sprintf("size=%d%s", size, suffix))
+	}
+	if opt.UID != 0 {
+		rawOpts = append(rawOpts, fmt.Sprintf("uid=%d", opt.UID))


TODO
Check nil instead of zero

removed the code itself

stevvooe · 2016-10-14T20:49:45Z

api/types/mount/mount.go

+	GID          uint32
+	Blocks       int64
+	INodes       int64
+	MemoryPolicy string


What is the contents of MemoryPolicy?

I removed it in the latest commit, but it meant mpol for Linux tmpfs

https://linux.die.net/man/8/mount

mpol=[default|prefer:Node|bind:NodeList|interleave|interleave:NodeList]

AkihiroSuda · 2016-10-25T06:37:28Z

updated PR #26837 (comment)
Please look into this again (if CI green)? @stevvooe @cpuguy83

cpuguy83

Pretty happy with this, just one minor change and I think we'll be ready to roll.

cpuguy83 · 2016-10-25T13:31:42Z

volume/validate.go

Please use errExtraField instead of fmt.Errorf

stevvooe · 2016-10-27T21:52:04Z

The fact that Mount.Data exists is disappointing but I don't see how we can remove it.

Thanks for the solid PR!

thaJeztah · 2016-10-27T23:12:06Z

ping @AkihiroSuda can you address @cpuguy83's nit (#26837 (comment)), then it looks like we're ready to go

AkihiroSuda · 2016-10-28T02:00:37Z

@thaJeztah done

AkihiroSuda · 2016-10-28T02:22:25Z

updated docs/reference/api as well

Signed-off-by: Akihiro Suda <[email protected]>

cpuguy83

LGTM

Moving to docs review

thaJeztah · 2016-10-28T17:03:17Z

docs/reference/api/docker_remote_api_v1.25.md

              - **Options** - key/value map of driver specific options.
+        - **TmpfsOptions** – Optional configuration for the `tmpfs` type.
+            - **SizeBytes** – The size for the tmpfs mount in bytes.
+            - **Mode** – The permission mode for the tmpfs mount in an integer.


Not for this PR, but I think we should update the JSON example with some mounts

(because we currently don't have an example in there it seems)

thaJeztah

docs LGTM, thanks so much @AkihiroSuda!

vdemeester

LGTM 🐸

GordonTheTurtle added the status/0-triage label Sep 22, 2016

AkihiroSuda mentioned this pull request Sep 22, 2016

Add new HostConfig field, Mounts. #22373

Merged

AkihiroSuda force-pushed the newtmpfs branch 2 times, most recently from f413b44 to 3321dc7 Compare September 22, 2016 22:38

thaJeztah added status/2-code-review and removed status/0-triage labels Sep 22, 2016

cpuguy83 requested changes Sep 24, 2016

View reviewed changes

stevvooe reviewed Sep 26, 2016

View reviewed changes

AkihiroSuda force-pushed the newtmpfs branch from 3321dc7 to cfd8138 Compare September 27, 2016 08:43

AkihiroSuda commented Sep 27, 2016

View reviewed changes

AkihiroSuda force-pushed the newtmpfs branch 3 times, most recently from a7afeda to 3b8527a Compare September 27, 2016 09:12

AkihiroSuda force-pushed the newtmpfs branch from 3b8527a to 65f23a5 Compare September 27, 2016 14:44

stevvooe requested changes Sep 27, 2016

View reviewed changes

AkihiroSuda mentioned this pull request Sep 28, 2016

[nit] integration-cli: obey Go's naming convention #26967

Merged

AkihiroSuda force-pushed the newtmpfs branch from 65f23a5 to d7179c8 Compare September 28, 2016 03:11

AkihiroSuda force-pushed the newtmpfs branch from d7179c8 to 942c9cb Compare September 28, 2016 03:18

AkihiroSuda mentioned this pull request Sep 29, 2016

[EXPERIMENTAL] new mount type: introspection #26331

Closed

7 tasks

stevvooe requested changes Oct 3, 2016

View reviewed changes

AkihiroSuda commented Oct 4, 2016

View reviewed changes

stevvooe reviewed Oct 14, 2016

View reviewed changes

AkihiroSuda force-pushed the newtmpfs branch from 942c9cb to ad9b444 Compare October 17, 2016 06:09

AkihiroSuda force-pushed the newtmpfs branch 2 times, most recently from e1c9452 to 1af22c3 Compare October 25, 2016 06:33

AkihiroSuda force-pushed the newtmpfs branch from 1af22c3 to 2b4cbcb Compare October 25, 2016 06:38

cpuguy83 requested changes Oct 25, 2016

View reviewed changes

AkihiroSuda mentioned this pull request Oct 27, 2016

Secret Management #27794

Merged

4 tasks

stevvooe approved these changes Oct 27, 2016

View reviewed changes

AkihiroSuda force-pushed the newtmpfs branch from 2b4cbcb to c72b00f Compare October 28, 2016 02:00

AkihiroSuda force-pushed the newtmpfs branch from c72b00f to e316627 Compare October 28, 2016 02:22

AkihiroSuda force-pushed the newtmpfs branch from e316627 to fa4e21e Compare October 28, 2016 04:22

api: add TypeTmpfs to api/types/mount

18768fd

Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the newtmpfs branch from fa4e21e to 18768fd Compare October 28, 2016 08:38

cpuguy83 approved these changes Oct 28, 2016

View reviewed changes

cpuguy83 added status/3-docs-review and removed status/2-code-review labels Oct 28, 2016

thaJeztah added this to the 1.13.0 milestone Oct 28, 2016

thaJeztah reviewed Oct 28, 2016

View reviewed changes

thaJeztah approved these changes Oct 28, 2016

View reviewed changes

thaJeztah added the impact/api label Oct 28, 2016

vdemeester approved these changes Oct 28, 2016

View reviewed changes

vdemeester merged commit 87ddc8b into moby:master Oct 28, 2016

AkihiroSuda mentioned this pull request Oct 31, 2016

Use container Mounts API for Swarm containers. #26833

Merged

AkihiroSuda mentioned this pull request Nov 8, 2016

opts/mount: add tmpfs-specific options #28150

Merged

jefflill mentioned this pull request Nov 12, 2016

Fatal Task Error mounting tmpfs volume to service #28335

Closed

thaJeztah mentioned this pull request Dec 2, 2016

[1.13] Tmpfs options not working for services #29070

Closed

thaJeztah mentioned this pull request Nov 22, 2017

Docker CLI should use mount API for --tmpfs docker/cli#710

Open

Conversation

AkihiroSuda commented Sep 22, 2016

Uh oh!

AkihiroSuda commented Sep 22, 2016

Uh oh!

justincormack commented Sep 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Sep 28, 2016 •

edited

Loading

AkihiroSuda commented Sep 27, 2016 •

edited

Loading