Add expanded mount format to stack deploy by dnephin · Pull Request #30597 · moby/moby

dnephin · 2017-01-31T17:28:01Z

Fixes #29193
Fixes #31224
Also (should) fix some windows support for volumes

I've added this to Compose format v3.1 for now, but it might be more appropriate for a v3.2, depending on when it gets merged and where it gets cherry-picked.

@cpuguy83 I'd appreciate your review of 3c74323d94491571e8667ba0a4b2cca9911b4dc0. It adds a client-side parser for the old volume spec. As far as I know this is the first time we're attempting to fully parse the spec on the client (without being aware of the engine platform). It assumes no knowledge of the platform, so windows and linux container paths are both supported on all clients.

@shin- we will probably need to implement this on the python side as well. It should be a smaller change. I think we'll only need to copy the schema and implement something like 3c74323d94491571e8667ba0a4b2cca9911b4dc0

cc @vdemeester

vdemeester

SGTM 👼
I wonder how using volume fits with ongoing discussion on #28527 (volumes, mounts ?).

/cc @icecrime @stevvooe

stevvooe · 2017-02-07T18:58:37Z

cli/compose/loader/loader_test.go

Shouldn't these be called mounts? It is very confusing to have a volume declaration here then a volume declaration at the top level. This contributes to confusion. Not that this section will also handle binds, so calling it volumes is inaccurate.

Yes, but it's also a breaking change, so not something we can change in the v3.1 format. We missed our chance with v3, so we can queue it up for v4 I guess (depending on the outcome of the docker run flag change).

stevvooe · 2017-02-07T19:00:26Z

The format looks good, in general. I would really prefer that we call it mounts, or at least have that as an alias. It is very weird to have volumes, then type: volume. It is also problematic to mix binds, volume, tmpfs, (and soon image mounts!) into volume. We should focus the term volume around the orchestratable object.

dnephin · 2017-02-07T19:04:00Z

We could do a mount alias

dnephin · 2017-02-16T17:22:09Z

Rebased

AkihiroSuda · 2017-02-28T03:33:08Z

cli/compose/schema/schema.go

Can we make these strings constant values?

justincormack · 2017-03-01T02:20:11Z

cc @cpuguy83 @jhowardmsft

cpuguy83 · 2017-03-03T13:30:54Z

Spoke with Daniel since I was having some issues trying to get this going. From a functionality POV, just have a couple of issues:

When "driver" is not defined on the volume, "driver_opts" are not passed through
The --mount cli supports templating everything about the mount spec, including Source which allows for some really powerful use-cases... this is kind of lost here afaict.

dnephin · 2017-03-03T21:28:32Z

The --mount cli supports templating everything about the mount spec, including Source which allows for some really powerful use-cases... this is kind of lost here afaict.

I believe it's still possible for all fields (it even works with the old short syntax, see #30770) . For source it requires that you use an external volume, because the source name in the service has to be a reference to a volume definition. The volume definition can use a template:

data:
  external:
    name: '{{ .Template }}'

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Add a test for loading expanded mount format. Signed-off-by: Daniel Nephin <dnephin@docker.com>

Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin · 2017-03-08T16:38:19Z

ping

cpuguy83

LGTM

vdemeester

Should we add a mount alias though ? 👼

dnephin · 2017-03-10T16:06:20Z

When it's supported by the CLI sure, but I think that's not required to merge this. We can add it later.

vdemeester

Sounds fair.
LGTM 🦁

thaJeztah · 2017-03-13T12:19:39Z

@dnephin can you open a pull request in the https://github.com/docker/docker.github.io repository for the syntax changes in the docker-compose file?

dnephin · 2017-03-13T18:35:18Z

Opened docker/docs#2283

cpuguy83 · 2017-03-13T22:25:27Z

@dnephin what cli? Service is --mount as is.

dnephin · 2017-03-14T14:26:43Z

True. The compose file fields are named after the docker run cli. Either way we can't drop volumes as the key in a minor version, so we can either decide to add an alias of mounts during code freeze, or we can decide to worry about it for the next version.

justincormack · 2017-03-14T16:33:06Z

@dnephin is there any reason we can't do a major bump of compose format for 17.04?

dnephin · 2017-03-14T16:37:21Z

You mean make it v4? There are no backwards incompatible changes, or any significantly large changes, so v4 would not be appropriate.

justincormack · 2017-03-14T17:00:04Z

@dnephin I meant so we can add mounts which you said was not suitable for a minor bump.

justincormack · 2017-03-14T17:53:25Z

Ok, lets leave any mounts change for another PR that bumps the API.

dnephin · 2017-03-14T17:59:29Z

I see what you mean. I think we'll probably want to batch up a few other breaking changes all at once before making a v4.

…to-stack-deploy Add expanded mount format to stack deploy

GordonTheTurtle added the status/0-triage label Jan 31, 2017

vdemeester added status/2-code-review impact/changelog impact/documentation and removed status/0-triage labels Jan 31, 2017

vdemeester requested review from cpuguy83 and vdemeester February 2, 2017 15:21

vdemeester reviewed Feb 7, 2017

View reviewed changes

dnephin force-pushed the add-expanded-mount-format-to-stack-deploy branch from 91ea532 to e66d61a Compare February 7, 2017 18:09

stevvooe reviewed Feb 7, 2017

View reviewed changes

cpuguy83 mentioned this pull request Feb 10, 2017

Docker 1.13 - Create / Mount volume with stack and docker-volume-drivers #30895

Closed

shin- mentioned this pull request Feb 10, 2017

Can not specify named volume flags (f.e. propagation) docker/compose#4467

Closed

GordonTheTurtle assigned justincormack Feb 15, 2017

dnephin force-pushed the add-expanded-mount-format-to-stack-deploy branch from e66d61a to 0721f41 Compare February 16, 2017 17:22

AkihiroSuda reviewed Feb 28, 2017

View reviewed changes

This was referenced Feb 28, 2017

Support expanded ports and mount format in the the compose v3 format #29193

Closed

Compose file parsing can't parse Windows paths #31224

Closed

dnephin force-pushed the add-expanded-mount-format-to-stack-deploy branch from 0721f41 to 4111153 Compare February 28, 2017 18:11

dnephin force-pushed the add-expanded-mount-format-to-stack-deploy branch 2 times, most recently from 617e237 to a1bbb2f Compare March 6, 2017 16:45

dnephin added 4 commits March 6, 2017 11:45

Add expanded mount syntax to Compose schema and types.

65c899b

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Parse a volume spec on the client, with support for windows drives

32c955b

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Support expanded mounts in Compose loader

190dc5a

Add a test for loading expanded mount format. Signed-off-by: Daniel Nephin <dnephin@docker.com>

Convert new compose volume type to swarm mount type

a1bbb2f

Signed-off-by: Daniel Nephin <dnephin@docker.com>

cpuguy83 approved these changes Mar 10, 2017

View reviewed changes

vdemeester reviewed Mar 10, 2017

View reviewed changes

AkihiroSuda mentioned this pull request Mar 11, 2017

[UX] Guidelines for short/long syntax flags #28527

Closed

3 tasks

vdemeester approved these changes Mar 12, 2017

View reviewed changes

dnephin mentioned this pull request Mar 13, 2017

Add ports and volumes long form syntax to compose-file reference docker/docs#2283

Merged

dnephin mentioned this pull request Mar 13, 2017

Compose file v3.2 #31795

Merged

justincormack merged commit 49376cd into moby:master Mar 14, 2017

GordonTheTurtle added this to the 17.04.0 milestone Mar 14, 2017

dnephin deleted the add-expanded-mount-format-to-stack-deploy branch March 14, 2017 17:58

This was referenced Apr 6, 2017

docker stack deploy does not recognize rw mount mode #32398

Closed

Support rw as a volume option in compose file #32407

Merged

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30597 from dnephin/add-expanded-mount-format-…

b8c49df

…to-stack-deploy Add expanded mount format to stack deploy

dnephin mentioned this pull request Jun 30, 2017

Define target folder for volume of service in compose file results in error: "invalid mount target, must be an absolute path: C" #32912

Closed

Conversation

dnephin commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

stevvooe Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Feb 7, 2017

Uh oh!

dnephin commented Feb 7, 2017

Uh oh!

dnephin commented Feb 16, 2017

Uh oh!

AkihiroSuda Feb 28, 2017 • 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!

dnephin Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

justincormack commented Mar 1, 2017

Uh oh!

cpuguy83 commented Mar 3, 2017

Uh oh!

dnephin commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Mar 8, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin commented Mar 10, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 13, 2017

Uh oh!

dnephin commented Mar 13, 2017

Uh oh!

cpuguy83 commented Mar 13, 2017

Uh oh!

dnephin commented Mar 14, 2017

Uh oh!

justincormack commented Mar 14, 2017

Uh oh!

dnephin commented Mar 14, 2017

Uh oh!

justincormack commented Mar 14, 2017

Uh oh!

justincormack commented Mar 14, 2017

Uh oh!

dnephin commented Mar 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dnephin commented Jan 31, 2017 •

edited

Loading

AkihiroSuda Feb 28, 2017 •

edited

Loading

dnephin commented Mar 3, 2017 •

edited

Loading