added support for tmpfs-mode in compose file #808

ethan-haynes · 2018-01-14T03:12:35Z

Provides a fix for #698

- What I did
Added the ability to use a tmpfs volume type in compose with the optional parameter of size.
- How I did it
Changed compose schemas 3.2-3.5 to allow for the type. Tweaked volumes parser to read it.
Removed it from unapproved params.
- How to verify it

Test it with compose:

version: "3.6"
services:
  tmpfs:
    image: nginx:latest
    volumes:
      - type: tmpfs
        target: /app
        tmpfs:
          size: 10000

$ docker stack deploy -c swarm-compose.yml nginx

codecov-io · 2018-01-14T05:01:15Z

Codecov Report

Merging #808 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
+ Coverage   50.92%   51.01%   +0.08%     
==========================================
  Files         237      237              
  Lines       15332    15362      +30     
==========================================
+ Hits         7808     7837      +29     
- Misses       7024     7025       +1     
  Partials      500      500

vdemeester

Design LGTM

thaJeztah · 2018-01-15T11:56:06Z

design looks good to me as well; moving to code review

thaJeztah

Thanks! Left some comments

thaJeztah · 2018-01-15T11:58:45Z

cli/compose/schema/data/config_schema_v3.2.json

@@ -261,6 +261,12 @@
                    "properties": {
                      "nocopy": {"type": "boolean"}
                    }
+                  },
+                  "tmpfs": {


The 3.2 ... 3.5 formats have been released, so those formats are now frozen; to add a new feature to the existing formats, a new version (3.6) needs to be created.

thaJeztah · 2018-01-15T11:59:14Z

cli/compose/loader/volume.go

@@ -15,7 +15,6 @@ const endOfSpec = rune(0)
 // ParseVolume parses a volume spec without any knowledge of the target platform
 func ParseVolume(spec string) (types.ServiceVolumeConfig, error) {
 	volume := types.ServiceVolumeConfig{}
-


can you revert this change?

thaJeztah · 2018-01-15T12:22:37Z

cli/compose/convert/volume.go

+	}
+
+	// Anonymous volumes
+	if volume.Source == "" && volume.Type != "tmpfs" {


Perhaps this was already in the existing code, but this looks like a bug, as this allows a type=bind without specifying a source;

version: "3.5" services: tmpfs: image: nginx:alpine volumes: - type: bind target: /app

I suggest moving this validation into the handleVolumeToMount(), handleBindToMount(), and handleTmpfsToMount() functions; it makes the logic easier to follow, and puts validation with each type.

Hm, interesting; it looks like there's actually a different issue as well; if nosource: is specified, docker stack deploy resolves this to the directory of the docker-compose.yml; I think that's not the correct behaviour; if no source is specified, it should produce an error (if you want to use the directory of the docker-compose file, an explicit source: "./" or source: "." should be required)

@dnephin @vdemeester ^^ wdyt?

thaJeztah · 2018-01-15T12:58:26Z

cli/compose/convert/volume.go

+	}
+	if volume.Tmpfs != nil {
+		result.TmpfsOptions = &mount.TmpfsOptions{
+			SizeBytes: volume.Tmpfs.Size,


Do we need validation of the Size property itself? (i.e., should likely be a positive integer)

Possibly this can be handled by the JSON-schema (see my other comment), but we should check if that is sufficient

thaJeztah · 2018-01-15T12:58:58Z

cli/compose/convert/volume.go

+	volume composetypes.ServiceVolumeConfig,
+	result mount.Mount,
+) (mount.Mount, error) {
+	// source must be empty for tmpfs


This comment is a bit redundant

thaJeztah · 2018-01-15T13:10:52Z

cli/compose/convert/volume.go

+	stackVolumes volumes,
+	namespace Namespace,
+) (mount.Mount, error) {
+	result := mount.Mount{


I'm not a big fan of moving this outside of the handleXX functions; I assume you kept it out to reduce code duplication, but now the conversion (and validation) is "split" between the handleXX functions and this function.

Given that not all mount-types use the same options in this type (e.g., type=bind uses Source, but type=tmpfs doesn't), I'd move this code to each handleXX function. If duplication of code is a concern (but I think it's minimal), you could have a helper function for the shared code

thaJeztah · 2018-01-15T13:11:23Z

cli/compose/convert/volume.go

+			Propagation: mount.Propagation(volume.Bind.Propagation),
+		}
+	}
+	// Binds volumes


This comment doesn't add much, so I'd remove it

thaJeztah · 2018-01-15T13:11:59Z

cli/compose/convert/volume.go

+			SizeBytes: volume.Tmpfs.Size,
+		}
+	}
+	// Tmpfs volumes


This comment doesn't add much, so I'd remove it; if the function needs documentation, add a GoDoc for the function itself

thaJeztah · 2018-01-15T13:13:11Z

cli/compose/schema/data/config_schema_v3.5.json

+                  "tmpfs": {
+                    "type": "object",
+                    "properties": {
+                      "size": {"type": "number"}


should this be "type": "integer" ?

If only positive integers are allowed, likely:

{ "type": "integer", "exclusiveMinimum": 0 }

(not sure if exclusiveMinimum or minimum; likely depends on how empty values are handled)

In the Docker documentation if tmpfs-size isn't provided it will be unlimited by default:

Option Description

tmpfs-size Size of the tmpfs mount in bytes. Unlimited by default.

So if the user wants to specify a ceiling of how many bytes to allocate, then they should provide a positive integer above zero. The docker cli, as it behaves now, will not allow negative numbers:

invalid argument "type=tmpfs,destination=/app,tmpfs-size=-1" for --mount: invalid value for tmpfs-size: -1

It will allow zero, but when you inspect the container it just defaults TmpfsOptions to an empty object, which (since tmpfs-size won't actually be set) will allocate "unlimited" resources:

"Mounts": [ { "Type": "tmpfs", "Target": "/app", "TmpfsOptions": {} } ]

So I think we should, as you said, use an exclusiveMinimum of zero. That way the user at least attaches the TmpfsOptions with a value above zero for tmpfs-size. I will go ahead and add that to the 3.6v schema, unless someone thinks it should be handled differently.

If the CLI uses zero as equivalent of "not set", then probably minimum should be used (zero is a valid value, but indicates "no limit").

That makes sense. I will Git started on those changes

thaJeztah · 2018-01-15T13:13:46Z

cli/compose/types/types.go

@@ -296,6 +296,11 @@ type ServiceVolumeVolume struct {
 	NoCopy bool `mapstructure:"nocopy"`
 }

+// ServiceVolumeTmpfs are options for a service volume of type tmpfs
+type ServiceVolumeTmpfs struct {
+	Size int64


This should probably be an uint64? Or is a signed integer used elsewhere?

Chose to use int64 over unit64 because the mount struct looked to expect TmpfsOptions to contain it. Source:
https://github.com/moby/moby/blob/6415f1dcf5c79da853c604733c56b43ae3d18ced/api/types/mount/mount.go#L108

Thanks! Wonder if that was for a reason ("special -1 value, e.g.) or just overlooked during review of the initial implementation. 🤔

ethan-haynes · 2018-01-15T13:39:56Z

Thanks for the helpful feedback. I will make the necessary changes and update the PR.

ethan-haynes · 2018-01-15T21:07:08Z

I believe that I addressed most of the concerns that @thaJeztah brought up in my latest PR, with the exception being the issue regarding the directory of the compose file being set when a source is not passed to a volume of type bind (using the long compose syntax). I added explicit checks for an empty source in the convert package, but by the time it is received there it is already set. I am happy to alter/fix the behavior in this PR if it is decided that the current functionality is not desired. Or we/I can make a separate issue for it and address it in a different PR. Thanks! 😄

dnephin · 2018-01-15T21:54:16Z

Not sure why jenkins didn't run the integration suite.

I forced a build from https://jenkins.dockerproject.org/job/docker/job/cli/view/change-requests/

dnephin

Thanks, this is looking good. Just a couple minor comments.

dnephin · 2018-01-15T21:59:40Z

cli/compose/convert/volume.go

+) (mount.Mount, error) {
+
+	switch volume.Type {
+	case "volume":


I think the old default (when type is not set) would be volume, so I guess this case should also have ""

dnephin · 2018-01-15T22:00:45Z

cli/compose/convert/volume.go

-	if volume.Type == "bind" && volume.Volume != nil {
-		return result, errors.New("volume options are incompatible with type bind")
+	if volume.Type != "tmpfs" {
+		result.Source = volume.Source


This isn't really necessary because handleTmpfsToMount() will error out in this case. I would remove this so we don't have to check it in two places.

thaJeztah

Thanks @ethan-haynes! Changes in this PR look good to me 🎉

I have one nit left;

Can you squash your commits (we tend to keep changes made following "review comments" out of our git history), but keep separate commits for the compose-file spec changes, so:

1 commit that copies the compose 3.5 spec to 3.6 (and updates the cli/compose/schema/bindata.go accordingly)
1 commit that adds the new "tmpfs" field to the compose file spec (and updates the cli/compose/schema/bindata.go accordingly)
1 commit with your code changes

(You can combine 2. and 3. in a single commit if you prefer)

Doing so makes it easier to see the changes that were made in the new 3.6 spec, and makes it easier to find which commit added the new field in git history.

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

dnephin

LGTM with squashed commits

thaJeztah

perfect! LGTM, thanks so much 👍

thaJeztah · 2018-01-16T19:59:53Z

ping @shin- for Docker Compose

thaJeztah · 2018-01-16T20:01:54Z

Oh! before I forget; @ethan-haynes could you also take care of updating the compose-file documentation? Possibly needs to go in the vnext-compose branch; https://github.com/docker/docker.github.io/tree/vnext-compose/compose but @mistyhacks may be able to give guidance there 😊

mdlinville · 2018-01-16T20:06:58Z

Yes, that's correct if it applies to a not-yet-released feature of Compose.

ethan-haynes · 2018-01-16T23:17:06Z

Sounds good. I'll go ahead and update the documentation with new examples for the 3.6v compose release. 📝

ethan-haynes requested review from dnephin and vdemeester as code owners January 14, 2018 03:12

GordonTheTurtle added the status/0-triage label Jan 14, 2018

vdemeester added status/2-code-review status/1-design-review and removed status/0-triage status/2-code-review labels Jan 15, 2018

vdemeester requested review from shin- and thaJeztah January 15, 2018 10:22

vdemeester reviewed Jan 15, 2018

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Jan 15, 2018

thaJeztah requested changes Jan 15, 2018

View reviewed changes

dnephin reviewed Jan 15, 2018

View reviewed changes

thaJeztah reviewed Jan 16, 2018

View reviewed changes

Ethan Haynes added 3 commits January 16, 2018 10:44

adding config_schema_v3.6.json and updating binary

2b8cc52

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

adding tmpfs field to the compose 3.6v file spec and updating binary

b15362c

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

added support for tmpfs-mode in compose file

cd69d08

Signed-off-by: Ethan Haynes <ethanhaynes@alumni.harvard.edu>

dnephin approved these changes Jan 16, 2018

View reviewed changes

ethan-haynes force-pushed the 698-tmpfs-mode-compose branch from 747dd8a to cd69d08 Compare January 16, 2018 18:00

thaJeztah approved these changes Jan 16, 2018

View reviewed changes

thaJeztah merged commit 14c62f6 into docker:master Jan 16, 2018

GordonTheTurtle added this to the 18.02.0 milestone Jan 16, 2018

thaJeztah added impact/changelog impact/compose-file labels Jan 16, 2018

ethan-haynes mentioned this pull request Jan 19, 2018

added doc changes for upcoming tmpfs-mode 3.6 compose docker/docs#5777

Merged

ethan-haynes deleted the 698-tmpfs-mode-compose branch January 20, 2018 12:43

thaJeztah mentioned this pull request Jan 24, 2018

[18.02] Create 18.02 Changelog docker-archive/docker-ce#394

Merged

shin- mentioned this pull request Feb 2, 2018

Add support for 3.6 schema and tmpfs mount size docker/compose#5637

Merged

vdemeester mentioned this pull request Apr 24, 2018

Support tmpfs-size and tmpfs-mode in compose-file #698

Closed

t-anjan mentioned this pull request Aug 13, 2018

tmpfs-mode not implemented in Compose file syntax #1285

Closed

thaJeztah mentioned this pull request Jan 29, 2020

Allow setting of shm-size for docker service create moby/moby#26714

Open

thaJeztah mentioned this pull request May 1, 2020

Permissions are not specific in tmpfs subdirectory of volume directory docker/for-mac#3214

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for tmpfs-mode in compose file #808

added support for tmpfs-mode in compose file #808

ethan-haynes commented Jan 14, 2018 •

edited

Loading

codecov-io commented Jan 14, 2018 •

edited

Loading

vdemeester left a comment

thaJeztah commented Jan 15, 2018

thaJeztah left a comment

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

thaJeztah Jan 15, 2018

ethan-haynes Jan 15, 2018 •

edited

Loading

thaJeztah Jan 15, 2018

ethan-haynes Jan 15, 2018

thaJeztah Jan 15, 2018

ethan-haynes Jan 15, 2018

thaJeztah Jan 15, 2018

ethan-haynes commented Jan 15, 2018

ethan-haynes commented Jan 15, 2018

dnephin commented Jan 15, 2018 •

edited

Loading

dnephin left a comment

dnephin Jan 15, 2018

dnephin Jan 15, 2018

thaJeztah left a comment

dnephin left a comment

thaJeztah left a comment

thaJeztah commented Jan 16, 2018

thaJeztah commented Jan 16, 2018

mdlinville commented Jan 16, 2018

ethan-haynes commented Jan 16, 2018

added support for tmpfs-mode in compose file #808

added support for tmpfs-mode in compose file #808

Conversation

ethan-haynes commented Jan 14, 2018 • edited Loading

codecov-io commented Jan 14, 2018 • edited Loading

Codecov Report

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 15, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethan-haynes Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethan-haynes commented Jan 15, 2018

ethan-haynes commented Jan 15, 2018

dnephin commented Jan 15, 2018 • edited Loading

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 16, 2018

thaJeztah commented Jan 16, 2018

mdlinville commented Jan 16, 2018

ethan-haynes commented Jan 16, 2018

ethan-haynes commented Jan 14, 2018 •

edited

Loading

codecov-io commented Jan 14, 2018 •

edited

Loading

ethan-haynes Jan 15, 2018 •

edited

Loading

dnephin commented Jan 15, 2018 •

edited

Loading