Ensure graphdriver only loads with experimental flag by cpuguy83 · Pull Request #28623 · moby/moby

cpuguy83 · 2016-11-19T16:51:31Z

Also updates some of the structures being sent so plugins are getting
all the new options.

ping @thaJeztah

thaJeztah

Thanks! Left some comments

Also /cc @mstanleyjones because I think in the new docs, every page has to be added to the menu manually

thaJeztah · 2016-11-19T17:46:09Z

docs/extend/plugins_graphdriver.md

Forgot to remove "Experimental:" from the title

Also, there should be metadata added for the docs (easiest to take other documents from the docker.github.io repo as example)

What do you mean about metadata added? In the docs repo we will need to add a TOC reference for this file, but I don't understand what else you mean...

@mstanleyjones oh, the metadata was added (I couldn't comment on the line where the change had to be made 😄)

thaJeztah · 2016-11-19T17:47:38Z

docs/extend/plugins_graphdriver.md

Should be no trailing comma here

thaJeztah · 2016-11-19T17:48:21Z

docs/extend/plugins_graphdriver.md

Can you add a json language hint to the fence here, and for other JSON examples?

AkihiroSuda · 2016-11-21T06:34:02Z

docs/extend/plugins_graphdriver.md

nit: we need a , in the end of this line

AkihiroSuda · 2016-11-21T06:34:33Z

docs/extend/plugins_graphdriver.md

AkihiroSuda · 2016-11-21T06:35:11Z

docs/extend/plugins_graphdriver.md

thaJeztah

found some more missing commas

thaJeztah · 2016-11-21T10:38:13Z

docs/extend/plugins_graphdriver.md

comma here as well (thanks @AkihiroSuda )

thaJeztah · 2016-11-21T10:39:19Z

docs/extend/plugins_graphdriver.md

missing comma here as well

thaJeztah · 2016-11-21T10:39:41Z

docs/extend/plugins_graphdriver.md

missing comma

cpuguy83 · 2016-11-22T11:11:00Z

This is updated with comma changes.

mdlinville · 2016-11-22T17:05:39Z

docs/extend/plugins_graphdriver.md

What do you mean about metadata added? In the docs repo we will need to add a TOC reference for this file, but I don't understand what else you mean...

mdlinville · 2016-11-22T17:09:16Z

docs/extend/plugins_graphdriver.md

This is an awkward wording. I don't really understand what it is talking about; Is there a driver back-end that has to be running before Docker Engine can be started?

I agree, I had trouble phrasing it... basically this:

$ docker plugin install myGraphDriver $ docker plugin enable myGraphDriver $ pkill dockerd $ dockerd -s myGraphDriver

The daemon has to be started with the graphdriver that it is going to use. You can't use a graphdriver plugin unless the plugin is enabled before you restart the daemon.

How about "You need to install and enable the plugin and then restart the Docker daemon before using the plugin. See the following example for the correct ordering of steps." followed by your 4-line code block?

mdlinville · 2016-11-22T17:13:17Z

docs/extend/plugins_graphdriver.md

All of this passive voice makes this hard to understand. Maybe something like "The array of option is passed through to the plugin, but the plugin is not required to parse or honor them."

mdlinville · 2016-11-22T17:14:06Z

docs/extend/plugins_graphdriver.md

How about something like:

The response also includes a list of UID and GID mappings, structured as follows:

mdlinville · 2016-11-22T17:16:10Z

docs/extend/plugins_graphdriver.md

Maybe something like:

If Parent is an empty string, there is no parent layer.

StoreOpt is a map of strings which indicate storage options.

mdlinville · 2016-11-22T17:16:53Z

docs/extend/plugins_graphdriver.md

s/for example/such as

mdlinville · 2016-11-22T17:17:19Z

docs/extend/plugins_graphdriver.md

If Parent is an empty string, there is no parent.

cpuguy83 · 2016-11-23T17:14:10Z

Adding [WIP] because there's still a few kinks to work out.
Technically nothing is blocking someone from loading a plugin graphdriver on a non-experimental daemon, but moving docs over is another thing altogether.

thaJeztah · 2016-11-23T23:33:39Z

@cpuguy83 if it remains experimental, you can add advisory: experimental to the metadata of the docs, that way it will print a warning (but you can still have the documentation published on docs.docker.com)

cpuguy83 · 2016-11-29T14:20:10Z

@thaJeztah Think I've worked out all the issues in any case, just blocked by #26398 at the moment.

thaJeztah

docs LGTM

mdlinville · 2016-11-30T16:50:35Z

Any thoughts on my suggestions above?

cpuguy83 · 2016-11-30T18:45:46Z

@mstanleyjones 👍 to the suggestions. I'll get them in.

thaJeztah · 2016-12-09T21:22:30Z

@cpuguy83 #26398 was merged

cpuguy83 · 2016-12-20T18:48:44Z

Ok, this is updated.
Gates loading a graphdriver via plugin by --experimental daemon flag... this is due to a few issues to deal with in the graphdriver interface, and makes sure we aren't stuck in the future because we need to support existing plugins.... and I doubt any pluggable graphdriver is really production ready at this point anyway.

ping @anusha-ragunathan

cpuguy83 · 2016-12-21T15:31:28Z

ping @tiborvass @aaronlehmann

tiborvass · 2016-12-21T18:52:52Z

LGTM

anusha-ragunathan · 2016-12-22T20:07:28Z

daemon/graphdriver/plugin.go

I would add "graphdriver plugins are only..." just to be extra clear.

anusha-ragunathan · 2016-12-22T20:10:30Z

docs/extend/plugins_graphdriver.md

dockerd --experimental -s ...

anusha-ragunathan · 2016-12-22T20:13:04Z

integration-cli/docker_cli_daemon_plugins_test.go

Lets make the testRequires an ExperimentalDaemon instead. Easier to switch to stable, when its supported.

anusha-ragunathan · 2016-12-22T20:13:47Z

integration-cli/docker_cli_external_graphdriver_unix_test.go

testRequires(ExperimentalDaemon) instead?

Also, not sure why this test is affected?

Actually, setupTest already sets up an ExperimentalDaemon. So this should not be necessary.

I think I ran into this because in order for this test to work for me, I have to manually run the make script (whereas I normally have a helper bash function) and I didn't set DOCKER_EXPERIMENTAL=1 in doing so.
Looks good to remove this when setup properly ;)

Also updates some of the structures being sent so plugins are getting all the new options. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2016-12-22T20:43:03Z

Updated

anusha-ragunathan · 2016-12-22T21:03:14Z

LGTM

thaJeztah · 2016-12-23T23:29:30Z

Windows CI failed because a Jenkins issue, so i restarted. I'm moving this to "merge", because it has two LGTM's, and I see misty's suggestions were addressed

cpuguy83 added the process/cherry-pick label Nov 19, 2016

cpuguy83 added this to the 1.13.0 milestone Nov 19, 2016

GordonTheTurtle added the status/0-triage label Nov 19, 2016

thaJeztah requested changes Nov 19, 2016

View reviewed changes

thaJeztah added impact/changelog impact/documentation status/2-code-review and removed status/0-triage labels Nov 19, 2016

cpuguy83 force-pushed the update_graphdriver_docs branch from 7bc918f to d499304 Compare November 20, 2016 15:57

AkihiroSuda reviewed Nov 21, 2016

View reviewed changes

docs/extend/plugins_graphdriver.md Outdated

Copy link

Member

AkihiroSuda Nov 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we need a , in the end of this line

AkihiroSuda reviewed Nov 21, 2016

View reviewed changes

docs/extend/plugins_graphdriver.md Outdated

Copy link

Member

AkihiroSuda Nov 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

AkihiroSuda reviewed Nov 21, 2016

View reviewed changes

docs/extend/plugins_graphdriver.md Outdated

Copy link

Member

AkihiroSuda Nov 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

thaJeztah changed the title ~~Moves graphdriver plugn docs out of experimental~~ Moves graphdriver plugin docs out of experimental Nov 21, 2016

thaJeztah requested changes Nov 21, 2016

View reviewed changes

cpuguy83 force-pushed the update_graphdriver_docs branch from d499304 to 4a29a85 Compare November 22, 2016 03:16

AkihiroSuda mentioned this pull request Nov 22, 2016

Deprecate --graph flag; Replace with --data-root #28696

Merged

mdlinville suggested changes Nov 22, 2016

View reviewed changes

cpuguy83 force-pushed the update_graphdriver_docs branch from 4a29a85 to 96b0c31 Compare November 22, 2016 17:29

cpuguy83 changed the title ~~Moves graphdriver plugin docs out of experimental~~ [WIP]Moves graphdriver plugin docs out of experimental Nov 23, 2016

cpuguy83 mentioned this pull request Nov 28, 2016

RFC: File System for Image Distribution #28878

Open

thaJeztah approved these changes Nov 29, 2016

View reviewed changes

cpuguy83 changed the title ~~[WIP]Moves graphdriver plugin docs out of experimental~~ [WIP] Ensure graphdriver only loads with experimental flag Dec 20, 2016

cpuguy83 force-pushed the update_graphdriver_docs branch from 96b0c31 to 8a743a6 Compare December 20, 2016 18:45

cpuguy83 force-pushed the update_graphdriver_docs branch 3 times, most recently from 3107b4e to dbcf0e1 Compare December 20, 2016 21:38

anusha-ragunathan reviewed Dec 22, 2016

View reviewed changes

docs/extend/plugins_graphdriver.md Outdated

Copy link

Contributor

anusha-ragunathan Dec 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dockerd --experimental -s ...

anusha-ragunathan reviewed Dec 22, 2016

View reviewed changes

Moves graphdriver plugn docs out of experimental

677fa03

Also updates some of the structures being sent so plugins are getting all the new options. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the update_graphdriver_docs branch from dbcf0e1 to 677fa03 Compare December 22, 2016 20:31

thaJeztah added kind/experimental rebuild/windowsRS1 and removed impact/changelog labels Dec 23, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Dec 23, 2016

thaJeztah added status/4-merge and removed status/2-code-review labels Dec 23, 2016

vdemeester merged commit d3e3a97 into moby:master Dec 24, 2016

cpuguy83 deleted the update_graphdriver_docs branch December 27, 2016 14:11

vieux removed the process/cherry-pick label Jan 4, 2017

thaJeztah modified the milestones: 1.13.1, 1.13.0 Jan 18, 2017

cpuguy83 mentioned this pull request Feb 19, 2017

[cherry-pick] Moves graphdriver plugin docs out of experimental #29869

Merged

thaJeztah added the process/cherry-picked label Feb 19, 2017

thaJeztah added area/plugins area/storage Image Storage labels Jun 8, 2024

Conversation

cpuguy83 commented Nov 19, 2016

Uh oh!

thaJeztah left a comment

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!

thaJeztah left a comment

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!

cpuguy83 commented Nov 22, 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!

cpuguy83 commented Nov 23, 2016

Uh oh!

thaJeztah commented Nov 23, 2016

Uh oh!

cpuguy83 commented Nov 29, 2016

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

mdlinville commented Nov 30, 2016

Uh oh!

cpuguy83 commented Nov 30, 2016

Uh oh!

thaJeztah commented Dec 9, 2016

Uh oh!

cpuguy83 commented Dec 20, 2016

Uh oh!

cpuguy83 commented Dec 21, 2016

Uh oh!

tiborvass commented Dec 21, 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!