Allow npipe volume type on stack file #1195

olljanat · 2018-07-08T17:09:42Z

- What I did
Added volume type npipe to compose file handing.

- How to verify it
Can be tested together with docked from moby/moby#37400

Pre-requirement for moby/moby#37400 to be able to fix moby/moby#34795

silvin-lubecki · 2018-07-10T00:07:53Z

@simonferquel Could you take a look please?

simonferquel

Overall lgtm, just a single nitpicking on unknown mount types error message

simonferquel · 2018-09-12T13:36:55Z

cli/compose/convert/volume.go

@@ -135,6 +155,8 @@ func convertVolumeToMount(
 		return handleBindToMount(volume)
 	case "tmpfs":
 		return handleTmpfsToMount(volume)
+	case "npipe":
+		return handleNpipeToMount(volume)
 	}
 	return mount.Mount{}, errors.New("volume type must be volume, bind, or tmpfs")


Error message should now include npipe as a valid mount type.

Good point. I included it to commit now.

simonferquel · 2018-09-12T13:41:04Z

Did not have time to review the moby/moby and swarmkit prs though

olljanat · 2018-09-12T19:32:21Z

@simonferquel swarmkit PR was just merged so can we also merge this one so I will be able to get that moby/moby done?

olljanat · 2018-09-18T08:18:42Z

@vdemeester can you please look this one too?

After moby/moby#37400 is merged we will be able to use named pipes with docker service create but this one is needed to be able do same thing with stack.

simonferquel · 2018-09-19T09:19:52Z

I think we should merge the moby side first

olljanat · 2018-09-26T17:03:35Z

@simonferquel @vdemeester @thaJeztah moby/moby#37400 have been merged so can we merge this one too?

cpuguy83

LGTM

vdemeester

LGTM 🐯

thaJeztah

Thanks! Overall looking good, but I think some more changes are needed 😅 🤗. I'll make a summary

thaJeztah · 2018-09-26T21:25:46Z

Thanks @olljanat! Looks like some changes are needed to the compose-file schema. Given that this is a new option, and the 3.7 schema is now frozen, that would involve a new schema version (3.8), and changes to this section; https://github.com/docker/cli/blob/master/cli/compose/schema/data/config_schema_v3.7.json#L279-L317

Also ping @shin- for changes in docker compose 🤗

The compose-file reference in the documentation repository will also need a slight update; https://github.com/docker/docker.github.io/blob/master/compose/compose-file/index.md#long-syntax-3, and mention the new option (that's in the documentation repository, so will be a follow up, or separate PR)

Some changes to the reference documentation are needed; the reference docs for docker service create; this section mentions the available types for --mount, so needs some information about the new type; https://github.com/docker/cli/blob/master/docs/reference/commandline/service_create.md#add-bind-mounts-volumes-or-memory-filesystems

Finally, we'll probably need some changes to the "storage" documentation for the new type; https://github.com/docker/docker.github.io/blob/master/storage/index.md That's also in the documentation repository, so can be done as a follow-up.

Perhaps we need some assistance from the documentation team for that (we can make a tracking issue if needed)

shin- · 2018-09-26T21:39:54Z

@thaJeztah It's already supported in docker-compose since 1.18.0 👼 docker/compose#5181

Also AFAICT, this doesn't require any changes to the schema; since we don't do any validation on the mount type field, this will work retroactively with any schema that supports the mount syntax (so 3.2+).

olljanat · 2018-09-26T22:57:30Z

@thaJeztah I added one commit which contains service create documentation and created PR docker/docs/pull/7427 of these two other page updates.

Also please note that named pipes support have been originally added for non-swarm mode over year ago on moby/moby/pull/33852 so maybe you can also ask help from these Microsoft dudes if more detailed documentation of named pipes are needed.

thaJeztah · 2018-09-27T07:58:34Z

Also AFAICT, this doesn't require any changes to the schema; since we don't do any validation on the mount type field, this will work retroactively with any schema that supports the mount syntax (so 3.2+).

@shin- @olljanat doh! You're right; I got confused there; the section I was pointing to is for the tmpfs, volume, and bind options to be passed as part of a volume, i.e.;

volumes:
  - type: tmpfs
    tmpfs:
      size: 12345

my bad! 😊

thaJeztah

Thanks for updating! Found some small issues in the documentation changes; could you also squash both commits? It's ok to have docs and code changes in the same commit for this PR.

thaJeztah · 2018-09-27T08:00:26Z

docs/reference/commandline/service_create.md

      <ul>
        <li><tt>volume</tt>: mounts a <a href="https://docs.docker.com/engine/reference/commandline/volume_create/">managed volume</a>
        into the container.</li> <li><tt>bind</tt>:
        bind-mounts a directory or file from the host into the container.</li>
        <li><tt>tmpfs</tt>: mount a tmpfs in the container</li>
+		<li><tt>npipe</tt>: mounts named pide from the host into the container.</li>


😅 looks like you indented with a tab, instead of spaces here.

Also there's a typo; pide instead of pipe ☺️

Perhaps it's good to add (Windows containers only) to the description

thaJeztah · 2018-09-27T08:01:59Z

docs/reference/commandline/service_create.md

@@ -312,15 +314,16 @@ volumes in a service:
    <th>Description</th>
  </tr>
  <tr>
-    <td><b>types</b></td>
+    <td><b>type</b></td>


👍 nice catch

thaJeztah · 2018-09-27T08:05:41Z

docs/reference/commandline/service_create.md

      <ul>
        <li><tt>volume</tt>: mounts a <a href="https://docs.docker.com/engine/reference/commandline/volume_create/">managed volume</a>
        into the container.</li> <li><tt>bind</tt>:
        bind-mounts a directory or file from the host into the container.</li>
        <li><tt>tmpfs</tt>: mount a tmpfs in the container</li>
+		<li><tt>npipe</tt>: mounts named pide from the host into the container.</li>
      </ul></p>
    </td>
  </tr>


Could you also update the required column at line 332 below https://github.com/docker/cli/pull/1195/files#diff-d4779426535b6679e406cf721c9f9289R332 ?

It currently says;

<td>for <tt>type=bind</tt> only></td>

Which should now be;

<td>for <tt>type=bind</tt> and <tt>type=npipe</tt></td>

(Noticed there's a stray > there as well)

thaJeztah · 2018-09-27T08:08:05Z

docs/reference/commandline/service_create.md

@@ -291,6 +291,8 @@ You can back up or restore volumes using Docker commands.

 A **tmpfs** mounts a tmpfs inside a container for volatile data.

+A **npipe** mounts a named pide from the host into the container.


whoops typo: pide (should be pipe)

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat · 2018-09-27T16:09:20Z

@thaJeztah requested changes should be in-place now.

thaJeztah

LGTM, thanks!!

olljanat requested a review from vdemeester as a code owner July 8, 2018 17:09

GordonTheTurtle added the status/0-triage label Jul 8, 2018

olljanat mentioned this pull request Jul 8, 2018

Allow mount type npipe on service/stack moby/moby#37400

Merged

olljanat force-pushed the 34795-npipe-mount-type branch from 9ba0a18 to 554830d Compare July 8, 2018 17:15

vdemeester added status/1-design-review and removed status/0-triage labels Jul 9, 2018

vdemeester requested review from simonferquel and johnstep September 12, 2018 13:27

simonferquel suggested changes Sep 12, 2018

View reviewed changes

olljanat force-pushed the 34795-npipe-mount-type branch 2 times, most recently from de98345 to 8178403 Compare September 12, 2018 15:31

olljanat mentioned this pull request Sep 26, 2018

Windows: Support named pipe mounts in docker service create + stack yml moby/moby#34795

Closed

cpuguy83 added status/2-code-review and removed status/1-design-review labels Sep 26, 2018

cpuguy83 approved these changes Sep 26, 2018

View reviewed changes

vdemeester approved these changes Sep 26, 2018

View reviewed changes

thaJeztah requested changes Sep 26, 2018

View reviewed changes

thaJeztah added impact/changelog platform/windows impact/compose-file labels Sep 26, 2018

olljanat mentioned this pull request Sep 26, 2018

[19.03] Included named pipe mounts documentation docker/docs#7427

Merged

thaJeztah requested changes Sep 27, 2018

View reviewed changes

Allow npipe volume type on stack file

0704d9a

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat force-pushed the 34795-npipe-mount-type branch from f50136a to 0704d9a Compare September 27, 2018 15:47

thaJeztah approved these changes Sep 28, 2018

View reviewed changes

thaJeztah merged commit 3e4e232 into docker:master Sep 28, 2018

GordonTheTurtle added this to the 19.03.0 milestone Sep 28, 2018

Drewster727 mentioned this pull request Jul 24, 2019

Docker stack deploy do not undersand volumes in windows format #1403

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow npipe volume type on stack file #1195

Allow npipe volume type on stack file #1195

olljanat commented Jul 8, 2018

silvin-lubecki commented Jul 10, 2018

simonferquel left a comment

simonferquel Sep 12, 2018

olljanat Sep 12, 2018

simonferquel commented Sep 12, 2018

olljanat commented Sep 12, 2018

olljanat commented Sep 18, 2018

simonferquel commented Sep 19, 2018

olljanat commented Sep 26, 2018

cpuguy83 left a comment

vdemeester left a comment

thaJeztah left a comment

thaJeztah commented Sep 26, 2018

shin- commented Sep 26, 2018

olljanat commented Sep 26, 2018

thaJeztah commented Sep 27, 2018

thaJeztah left a comment

thaJeztah Sep 27, 2018

thaJeztah Sep 27, 2018

thaJeztah Sep 27, 2018

thaJeztah Sep 27, 2018

thaJeztah Sep 27, 2018

olljanat commented Sep 27, 2018

thaJeztah left a comment

		@@ -291,6 +291,8 @@ You can back up or restore volumes using Docker commands.

		A tmpfs mounts a tmpfs inside a container for volatile data.

		A npipe mounts a named pide from the host into the container.

Allow npipe volume type on stack file #1195

Allow npipe volume type on stack file #1195

Conversation

olljanat commented Jul 8, 2018

silvin-lubecki commented Jul 10, 2018

simonferquel left a comment

Choose a reason for hiding this comment

simonferquel Sep 12, 2018

Choose a reason for hiding this comment

olljanat Sep 12, 2018

Choose a reason for hiding this comment

simonferquel commented Sep 12, 2018

olljanat commented Sep 12, 2018

olljanat commented Sep 18, 2018

simonferquel commented Sep 19, 2018

olljanat commented Sep 26, 2018

cpuguy83 left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Sep 26, 2018

shin- commented Sep 26, 2018

olljanat commented Sep 26, 2018

thaJeztah commented Sep 27, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Sep 27, 2018

Choose a reason for hiding this comment

thaJeztah Sep 27, 2018

Choose a reason for hiding this comment

thaJeztah Sep 27, 2018

Choose a reason for hiding this comment

thaJeztah Sep 27, 2018

Choose a reason for hiding this comment

thaJeztah Sep 27, 2018

Choose a reason for hiding this comment

olljanat commented Sep 27, 2018

thaJeztah left a comment

Choose a reason for hiding this comment