New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Mar 31, 2017

Conversation

@jlhawn
Contributor

jlhawn commented Nov 22, 2016

This patch deprecates the -g or --graph flag on the dockerd or docker daemon command in favor of the more descriptive --data-root flag.

The name "graph" is a legacy term from long ago when there used to be a directory at the default location /var/lib/docker/graph. However, the flag would indicate the path of the parent directory of the "graph" directory which contains not only image data but also data for volumes, containers, and networks. In the most recent version of docker, this directory also contains swarm cluster state and node certificates.

Documentation for the --graph option has been updated to use the --data-root flag instead. A deprecation notice has also been added.

@shykes

This comment has been minimized.

Show comment
Hide comment
@shykes

shykes Nov 22, 2016

Collaborator

As the guy who first used the term "graph" in this context (because of the git-like graph relationship between layers), I agree that it's confusing and should be deprecated.

As long as this introduces absolutely no interface regression, I am in favor.

Collaborator

shykes commented Nov 22, 2016

As the guy who first used the term "graph" in this context (because of the git-like graph relationship between layers), I agree that it's confusing and should be deprecated.

As long as this introduces absolutely no interface regression, I am in favor.

Show outdated Hide outdated docs/deprecated.md Outdated
@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Nov 22, 2016

Member

According to the docs include in this PR, the milestone of this PR seems 1.14.

However, in 1.13, GraphDriver plugin will be moved to out of experimental.
So, we need to decide whether we rename GraphDriver as well or not.
#28623

I'm +1 for renaming GraphDriver, but I don't think we should call it "DataRootDriver"...
Perhaps --storage-driver and "StorageDriver"?

PTAL @cpuguy83

Member

AkihiroSuda commented Nov 22, 2016

According to the docs include in this PR, the milestone of this PR seems 1.14.

However, in 1.13, GraphDriver plugin will be moved to out of experimental.
So, we need to decide whether we rename GraphDriver as well or not.
#28623

I'm +1 for renaming GraphDriver, but I don't think we should call it "DataRootDriver"...
Perhaps --storage-driver and "StorageDriver"?

PTAL @cpuguy83

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn Nov 22, 2016

Contributor

So, we need to decide whether we rename GraphDriver as well or not.

Oh, I've been wanting to do that for about 2 years: #9758 but we never got around to doing it. There is also already an existing --storage-driver flag for dockerd:

  -s, --storage-driver string                 Storage driver to use

I'm +1 for renaming GraphDriver, but I don't think we should call it "DataRootDriver"...
Perhaps --storage-driver and "StorageDriver"?

I think these are two distinct things: --data-root just specifies where the directory is. Your choice of copy-on-write image/container filesystem storage driver is independent of that. The same directory will contain subdirectories for containers, image, network, swarm, tmp, volumes, and whatever the name of the storage driver is that you choose.

Contributor

jlhawn commented Nov 22, 2016

So, we need to decide whether we rename GraphDriver as well or not.

Oh, I've been wanting to do that for about 2 years: #9758 but we never got around to doing it. There is also already an existing --storage-driver flag for dockerd:

  -s, --storage-driver string                 Storage driver to use

I'm +1 for renaming GraphDriver, but I don't think we should call it "DataRootDriver"...
Perhaps --storage-driver and "StorageDriver"?

I think these are two distinct things: --data-root just specifies where the directory is. Your choice of copy-on-write image/container filesystem storage driver is independent of that. The same directory will contain subdirectories for containers, image, network, swarm, tmp, volumes, and whatever the name of the storage driver is that you choose.

@vdemeester vdemeester added this to the 1.14.0 milestone Nov 22, 2016

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 22, 2016

Member

@AkihiroSuda @jlhawn it was attempted twice; #22228 and #23237, but there were issues on Windows. Maybe they are resolved in the newer Windows versions.

Member

thaJeztah commented Nov 22, 2016

@AkihiroSuda @jlhawn it was attempted twice; #22228 and #23237, but there were issues on Windows. Maybe they are resolved in the newer Windows versions.

@@ -1759,7 +1759,7 @@ _docker_daemon() {
_filedir
return
;;
--exec-root|--graph|-g)
--exec-root|--data-root)

This comment has been minimized.

@albers

albers Nov 22, 2016

Member

Please add -d as a third choice here .

@albers

albers Nov 22, 2016

Member

Please add -d as a third choice here .

This comment has been minimized.

@thaJeztah

thaJeztah Nov 22, 2016

Member

Hm, would -d become confusing with -D? Otherwise I suggest to skip adding a shorthand flag

@thaJeztah

thaJeztah Nov 22, 2016

Member

Hm, would -d become confusing with -D? Otherwise I suggest to skip adding a shorthand flag

This comment has been minimized.

@jlhawn

jlhawn Nov 22, 2016

Contributor

That sounds reasonable. There's also not a shorthand for the --exec-root option. Let me know when the maintainers have a consensus on what to do here and I'll update if needed.

@jlhawn

jlhawn Nov 22, 2016

Contributor

That sounds reasonable. There's also not a shorthand for the --exec-root option. Let me know when the maintainers have a consensus on what to do here and I'll update if needed.

This comment has been minimized.

@thaJeztah

thaJeztah Nov 22, 2016

Member

Probably best to remove it initially, it's easier to add later than to deprecate / remove

@thaJeztah

thaJeztah Nov 22, 2016

Member

Probably best to remove it initially, it's easier to add later than to deprecate / remove

This comment has been minimized.

@jlhawn

jlhawn Nov 22, 2016

Contributor

Okay, removed for now. I'll add it back if needed.

@jlhawn

jlhawn Nov 22, 2016

Contributor

Okay, removed for now. I'll add it back if needed.

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Nov 28, 2016

Contributor

SGTM

Contributor

estesp commented Nov 28, 2016

SGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 21, 2016

Contributor

Ok for me, but I don't think we could ever actually remove --graph/-g

Contributor

cpuguy83 commented Dec 21, 2016

Ok for me, but I don't think we could ever actually remove --graph/-g

@tianon

tianon approved these changes Jan 19, 2017

❤️!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 19, 2017

Member

@jlhawn can you keep the old flags around as a hidden flag, in addition to the current one?

Also, the daemon.json should probably support the old name as well (but we can issue a warning, and deprecated that, so you need to add that to the deprecated.md)

Member

thaJeztah commented Jan 19, 2017

@jlhawn can you keep the old flags around as a hidden flag, in addition to the current one?

Also, the daemon.json should probably support the old name as well (but we can issue a warning, and deprecated that, so you need to add that to the deprecated.md)

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn Jan 19, 2017

Contributor

can you keep the old flags around as a hidden flag, in addition to the current one?
Also, the daemon.json should probably support the old name as well (but we can issue a warning, and deprecated that, so you need to add that to the deprecated.md)

Is this not already covered by this: https://github.com/docker/docker/pull/28696/files#diff-a348195a1b645d5b477946b1efae728bR173 ? Also the graph option in the config file corresponds to the command line option so you should get a warning for that as well right?

Contributor

jlhawn commented Jan 19, 2017

can you keep the old flags around as a hidden flag, in addition to the current one?
Also, the daemon.json should probably support the old name as well (but we can issue a warning, and deprecated that, so you need to add that to the deprecated.md)

Is this not already covered by this: https://github.com/docker/docker/pull/28696/files#diff-a348195a1b645d5b477946b1efae728bR173 ? Also the graph option in the config file corresponds to the command line option so you should get a warning for that as well right?

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

ping @thaJeztah

Contributor

LK4D4 commented Jan 30, 2017

ping @thaJeztah

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 31, 2017

Member

@jlhawn sorry, yes, you're right about the hidden flag. w.r.t. removing the flag, given that this flag has been around since docker < 1.0, we may need to decide to keep it around, and not actually remove it, but open to suggestions there.

There's a think that needs to be addressed in this PR; you forgot to update the name in the daemon.json file; https://github.com/docker/docker/blob/d6be0e98027611cfb14a3246ca797bee0936e649/daemon/config.go#L99

To allow both the old and the new name to work; possibly we need to have an additional property, that's mapped to the old (graph) name in the daemon.json. If a user uses that option, a deprecation warning should be printed.

In addition; the deprecation warning is currently printed on the commandline. Given that daemon options would usually be set in either a systemd unit file, or through daemon.json, I think we should actually log the error, otherwise people will never see the deprecation message

Member

thaJeztah commented Jan 31, 2017

@jlhawn sorry, yes, you're right about the hidden flag. w.r.t. removing the flag, given that this flag has been around since docker < 1.0, we may need to decide to keep it around, and not actually remove it, but open to suggestions there.

There's a think that needs to be addressed in this PR; you forgot to update the name in the daemon.json file; https://github.com/docker/docker/blob/d6be0e98027611cfb14a3246ca797bee0936e649/daemon/config.go#L99

To allow both the old and the new name to work; possibly we need to have an additional property, that's mapped to the old (graph) name in the daemon.json. If a user uses that option, a deprecation warning should be printed.

In addition; the deprecation warning is currently printed on the commandline. Given that daemon options would usually be set in either a systemd unit file, or through daemon.json, I think we should actually log the error, otherwise people will never see the deprecation message

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn Jan 31, 2017

Contributor

@thaJeztah I can't quite wrap my head around the code which handles the config file and command line arguments (theres a bunch of stuff which handles resolving conflicts and setting defaults when both are used). Is there an existing example of how to deprecate something from the config file? The only other daemon flag I see which is marked as deprecated is the --restart flag which doesn't have a corresponding config file JSON tag.

I think we should actually log the error, otherwise people will never see the deprecation message

The flag library being used will print the deprecation warning to stderr. What is different about manually logging it (besides the formatting)?

Contributor

jlhawn commented Jan 31, 2017

@thaJeztah I can't quite wrap my head around the code which handles the config file and command line arguments (theres a bunch of stuff which handles resolving conflicts and setting defaults when both are used). Is there an existing example of how to deprecate something from the config file? The only other daemon flag I see which is marked as deprecated is the --restart flag which doesn't have a corresponding config file JSON tag.

I think we should actually log the error, otherwise people will never see the deprecation message

The flag library being used will print the deprecation warning to stderr. What is different about manually logging it (besides the formatting)?

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn Jan 31, 2017

Contributor

The more I look at it, the more it looks like I might have to "invent" a way of marking and handling deprecated config file options.

Contributor

jlhawn commented Jan 31, 2017

The more I look at it, the more it looks like I might have to "invent" a way of marking and handling deprecated config file options.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 31, 2017

Member

Yes, I think this is a first for config-file deprecation. It will probably mean something like;

type CommonConfig struct {
	RootDeprecated     string              `json:"graph,omitempty"`
	Root               string              `json:"data-dir,omitempty"`
}
  • if the "old" (graph) is set; print deprecation warning
  • if both "old" and "new" are set; produce error and fail

w.r.t. both "flag" and "config file"; the current behavior is that if both a flag and the same option in the config file are set, that an error is shown.

Member

thaJeztah commented Jan 31, 2017

Yes, I think this is a first for config-file deprecation. It will probably mean something like;

type CommonConfig struct {
	RootDeprecated     string              `json:"graph,omitempty"`
	Root               string              `json:"data-dir,omitempty"`
}
  • if the "old" (graph) is set; print deprecation warning
  • if both "old" and "new" are set; produce error and fail

w.r.t. both "flag" and "config file"; the current behavior is that if both a flag and the same option in the config file are set, that an error is shown.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 14, 2017

Member

Ouch, I think this conflicted with #29673

@jlhawn do you still want to work on this, or would you prefer someone to carry?

Member

thaJeztah commented Feb 14, 2017

Ouch, I think this conflicted with #29673

@jlhawn do you still want to work on this, or would you prefer someone to carry?

@jlhawn

This comment has been minimized.

Show comment
Hide comment
@jlhawn

jlhawn Feb 23, 2017

Contributor

@thaJeztah hopefully I rebased correctly. I did a local build and it seems to be okay. I'm not sure if I did the config_solaris.go file correctly though.

Contributor

jlhawn commented Feb 23, 2017

@thaJeztah hopefully I rebased correctly. I did a local build and it seems to be okay. I'm not sure if I did the config_solaris.go file correctly though.

defaultPidFile = "/system/volatile/docker/docker.pid"
defaultGraph = "/var/lib/docker"
defaultExec = "zones"
)

This comment has been minimized.

@jlhawn

jlhawn Feb 23, 2017

Contributor

I removed these because 1) they don't seem to be used anywhere, and 2) files for other operating systems seem to have been moved to the cmd/dockerd package while these were left behind. It looks like this should be covered by https://github.com/docker/docker/pull/28696/files#diff-adcf97f8e8b7815a2ec4660c286531ea

@jlhawn

jlhawn Feb 23, 2017

Contributor

I removed these because 1) they don't seem to be used anywhere, and 2) files for other operating systems seem to have been moved to the cmd/dockerd package while these were left behind. It looks like this should be covered by https://github.com/docker/docker/pull/28696/files#diff-adcf97f8e8b7815a2ec4660c286531ea

@thaJeztah thaJeztah modified the milestones: 17.05.0, 17.04.0 Mar 15, 2017

@thaJeztah

found some minor issues, but otherwise LGTM; let me push the fixes here to get this going again.

After this is merged, we'll have to check if other documentation needs to be updated https://github.com/docker/docker.github.io/search?p=1&q=graph&type=&utf8=✓

Show outdated Hide outdated docs/deprecated.md Outdated
Show outdated Hide outdated docs/deprecated.md Outdated
@@ -351,8 +352,21 @@ func getConflictFreeConfiguration(configFile string, flags *pflag.FlagSet) (*Con
}
reader = bytes.NewReader(b)
err = json.NewDecoder(reader).Decode(&config)
return &config, err
if err := json.NewDecoder(reader).Decode(&config); err != nil {

This comment has been minimized.

@thaJeztah

thaJeztah Mar 28, 2017

Member

Looks like validation doesn't currently catch specifying both --graph and --data-dir as flags

@thaJeztah

thaJeztah Mar 28, 2017

Member

Looks like validation doesn't currently catch specifying both --graph and --data-dir as flags

This comment has been minimized.

@ehazlett

ehazlett Mar 28, 2017

Contributor

I think adding the flag check (in aeb461e) is fine without adding more config validation.

@ehazlett

ehazlett Mar 28, 2017

Contributor

I think adding the flag check (in aeb461e) is fine without adding more config validation.

This comment has been minimized.

@thaJeztah

thaJeztah Mar 28, 2017

Member

Yes, I added that commit based on this finding 😄 Was writing it up as an example, and decided to save @jlhawn the time and add rebase it, and add it to the PR

@thaJeztah

thaJeztah Mar 28, 2017

Member

Yes, I added that commit based on this finding 😄 Was writing it up as an example, and decided to save @jlhawn the time and add rebase it, and add it to the PR

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 30, 2017

Member

Also, looking at this; the "--graph is deprecated" message is now printed on the command line, not logged; given that daemon options are usually set in an init-script (e.g. the systemd unit file), I'll add it as a warning to the logs as well.

Member

thaJeztah commented Mar 30, 2017

Also, looking at this; the "--graph is deprecated" message is now printed on the command line, not logged; given that daemon options are usually set in an init-script (e.g. the systemd unit file), I'll add it as a warning to the logs as well.

Deprecate --graph flag; Replace with --data-root
Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
@vdemeester

LGTM with I'll add it as a warning to the logs as well. from @thaJeztah 👼

addressed

Add conflict check for flags, and update deprecation versions
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 30, 2017

Member

ok, updated after discussing with @vdemeester, and changed to hide the flag instead of printing a deprecation warning, and only log the warning. I think this is the least disruptive approach, and still discourages using the old flag, but happy to change back if we think otherwise

Member

thaJeztah commented Mar 30, 2017

ok, updated after discussing with @vdemeester, and changed to hide the flag instead of printing a deprecation warning, and only log the warning. I think this is the least disruptive approach, and still discourages using the old flag, but happy to change back if we think otherwise

@thaJeztah thaJeztah added this to backlog in maintainers-session Mar 30, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 30, 2017

Member

ok, discussed in the maintainers meeting, and there were no objections to this approach, so should be ready to go 😄

Member

thaJeztah commented Mar 30, 2017

ok, discussed in the maintainers meeting, and there were no objections to this approach, so should be ready to go 😄

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 31, 2017

Member

Ok then, let's be crazy and merge it 👼

Member

vdemeester commented Mar 31, 2017

Ok then, let's be crazy and merge it 👼

@vdemeester vdemeester merged commit 1ecaed0 into moby:master Mar 31, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32290 has succeeded
Details
janky Jenkins build Docker-PRs 40901 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1063 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12019 has succeeded
Details
z Jenkins build Docker-PRs-s390x 901 has succeeded
Details
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 31, 2017

Member

Thanks @jlhawn !

Member

thaJeztah commented Mar 31, 2017

Thanks @jlhawn !

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis May 6, 2017

Contributor

Although deprecated - is this a breaking change? There are lots of tutorials etc on the internet that advise on using -g to specify a different path/disk for the Docker data area.

Contributor

alexellis commented May 6, 2017

Although deprecated - is this a breaking change? There are lots of tutorials etc on the internet that advise on using -g to specify a different path/disk for the Docker data area.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester May 6, 2017

Member

@alexellis the flags are hidden not removed. It won't break anyone.

These flags were added before Docker 1.0, so will not be removed, only
hidden, to discourage their use.

Member

vdemeester commented May 6, 2017

@alexellis the flags are hidden not removed. It won't break anyone.

These flags were added before Docker 1.0, so will not be removed, only
hidden, to discourage their use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment