Warning message for lvm devmapper running on top of loopback devices #13266

Merged
merged 1 commit into from Aug 31, 2015

Projects

None yet
@shishir-a412ed
Contributor

Signed-off-by: Shishir Mahajan shishir.mahajan@redhat.com

@shishir-a412ed
Contributor

There are 2 changes in this PR:

  1. We want to warn users if they are running devmapper on loopback devices. This is not production ready and should be discouraged.
  2. Users should have the ability to suppress this warning, and not constantly pestered by it.

Docs are updated for more elaborate description.

@thaJeztah
Member

ping @vbatts

@vbatts
Contributor
vbatts commented May 17, 2015

it doesn't say expressly say "it is not supported", but hopefully will once we decide to never fall back to devicemapper on loop-lvm, because upstream lvm absolutely does not support this loop-lvm setup.

LGTM

@vbatts
Contributor
vbatts commented May 17, 2015
@rhvgoyal
Contributor

This patch takes warning all the way to client. Assumption here is that not many people might bother to read syslogs and notice warning.

LGTM.

@shishir-a412ed
Contributor

Following code and doc changes are taken from PR #12404

  1. daemon/graphdriver/devmapper/README.md: doc change.
  2. lvm devmapper on loopback devices check: daemon/graphdriver/devmapper/deviceset.go
@runcom runcom commented on an outdated diff May 18, 2015
api/client/run.go
@@ -188,10 +188,15 @@ func (cli *DockerCli) CmdRun(args ...string) error {
}()
//start the container
- if _, _, err = readBody(cli.call("POST", "/containers/"+createResponse.ID+"/start", nil, nil)); err != nil {
+ var obj []byte
+ if obj, _, err = readBody(cli.call("POST", "/containers/"+createResponse.ID+"/start", nil, nil)); err != nil {
@runcom
runcom May 18, 2015 Member

nit but can we avoid the var declaration and split in obj, _, err := ... and if err != nil {} otherwise obj?

@crosbymichael
Member

Why should this be returned to the client? It's a daemon option and the client should not know about this. The warning should be to the admin that configured and started docker and we should not have a specific flag to stop this warning.

@vbatts
Contributor
vbatts commented May 18, 2015

@crosbymichael while I understand what you're saying, the fact that devicemapper loop-lvm is what so many distros allow users to default to, the fact is that this not an admin specifically configuring to use it. They install docker, and get an awful experience with devicemapper on loop-lvm, and have no idea why. We could hope for users that intentionally configure the storage driver for their optimal use case ...

@cgwalters
Contributor

Can we land #13323 first, then rebase this on top?

@cgwalters
Contributor

One issue with this is that for any usage of the client binary in scripts, all of a sudden we're injecting new output to stderr.

# docker run -ti centos echo hello |& wc -l
2

I hope not too many people are parsing stderr, but it's something to be aware of.

One compromise we might make is to only emit the warning when the client is connected to a tty.

@icecrime
Member

Agree with Michael: error message shouldn't be shown on the client side (and especially not mixed with regular output).

@icecrime icecrime and 1 other commented on an outdated diff Jun 26, 2015
api/server/server.go
@@ -1047,6 +1048,11 @@ func (s *Server) postContainersStart(version version.Version, w http.ResponseWri
}
return err
}
+
+ if devmapper.WarnOnLoopback && devmapper.LoopbackInUse {
+ fmt.Fprintln(w, "Usage of loopback devices is strongly discouraged for production use. Either use `--storage-opt dm.thinpooldev` or use `--storage-opt dm.no_warn_on_loop_devices=true` to suppress this warning.")
@icecrime
icecrime Jun 26, 2015 Member

This should be a log.

@rhatdan
rhatdan Jun 27, 2015 Contributor

I am not a big fan of this either, the problem is our file system team is wants it badly to make sure customers do not accidentally put this into production, and they feel that the log message will be ignored. I believe there is another patch that writes this message to the syslog system.

@shishir-a412ed
Contributor

seems like we are going in an impasse :)
We understand that warning message on client side may not be the best place, but do we really want docker customers to be unhappy with the experience they get running docker ?

Scenario: I am a docker user, not a storage guy. I just started docker (which is running loopback devices by default). I didn't set any options. Now my docker is running slow. It didn't blew up, it's just running slow. I have no clue why but I am not happy with the experience !

Honestly, This is not the first time we are having a warning message on client side. Try updating an image with docker load command. you ll get a warning message that the old image is getting renamed to none:none. The load will still go through as it's a warning message. I think this one is much more critical.

@mrjana
Contributor
mrjana commented Jul 22, 2015

@shishir-a412ed Based on the discussion I and @tiborvass had the warning definitely needs to be a part of daemon log and it can also be be part of docker info so users can see that straight away when they either see the syslog or docker info. Showing it on the client side is tricky because the user cannot distinguish if the message is coming from the daemon or from the application running in the container unless the container was started in detached mode.

@rhatdan
Contributor
rhatdan commented Jul 23, 2015

I agree this should be put into two different pull requests. One to reveal the information in the docker logs, and docker info. And one to output the data to the client. If docker only accepted the first patch, I would be fine with it. We are also working on a tool to help users to migrate off of one back end to another, this would allow someone who went into production using loopback a way to migrate off without loosing their containers. It should also allow people to move from one back end to another. Say try out Overlayfs.

@rhvgoyal
Contributor

docker info has the information present that we are using loop devices.

Putting a warning in there too, feels little odd to me. I feel warning should be part of syslogs and if possible carry it all the way to the client.

@rhatdan
Contributor
rhatdan commented Jul 23, 2015

I look at that data and I have no idea what it means, Adding (Developer Mode) or (No Production Mode) would be useful.

@rhvgoyal
Contributor

I am afraid then this mode field will start getting overloaded soon and one will have to go through logs anyway to figure out why their docker is in developer mode and not in production mode. And if one has to go through logs anyway, why to introduce more fields in docker info.

@rhvgoyal
Contributor

And then their will be arguments about when something is considered developer mode and when does it go into production mode. For example.

  • Using overlay is developer mode or production mode.
  • Say there is some configuration of lvm thin pool which is not recommended and user has setup. Then should we transition docker into developer mode?

IOW, what I am trying to say is that trying to classify docker/storage configuration into "Developer/Production" mode is not going to be sufficient. There can be many reasons for docker to be in certain mode and user will have to walk through the logs anyway. It is not a replacement for that.

@icecrime
Member

@shishir-a412ed Some concerns have been raised some time ago by @crosbymichael and @mrjana (with @tiborvass), especially regarding the fact that this error is produced to the client, as part of a docker run. Can you please address them? Thanks.

@shishir-a412ed
Contributor

@rhvgoyal @rhatdan
What do you guys wanna do ? If you guys are okay with putting the message on the daemon log, I can quickly make the changes.

If not, I can just close this PR. I see a lot of back and forth happening on this one, with no consensus.

@rhatdan
Contributor
rhatdan commented Aug 11, 2015

I think we should just log to the syslog upstream. Our file system teams insist on us logging to the client, so we need to carry the patch until we can change the default.

@rhvgoyal
Contributor

Agreed. One step at a time. Let us first log it in syslog and then see what else can be done to make it more explicit.

@shishir-a412ed
Contributor

@icecrime I have made the changes.

@tiborvass tiborvass and 1 other commented on an outdated diff Aug 11, 2015
daemon/graphdriver/devmapper/deviceset.go
@@ -1397,6 +1397,12 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error {
}
}
+ if devices.thinPoolDevice == "" {
+ if devices.metadataLoopFile != "" || devices.dataLoopFile != "" {
+ logrus.Warnf("Usage of loopback devices is strongly discouraged for production use. Please use `--storage-opt dm.thinpooldev`.")
@tiborvass
tiborvass Aug 11, 2015 Contributor

would be great to output a URL to the docs where users can read more about how to properly configure devmapper

@shishir-a412ed
shishir-a412ed Aug 12, 2015 Contributor

@tiborvass I have made some changes. Take a look now and see if this is good.

@cpuguy83
Contributor

LGTM

@calavera
Contributor

LGTM. Moving to docs review

@bfirsh
Contributor
bfirsh commented Aug 28, 2015
@SvenDowideit
Collaborator
@jamtur01 jamtur01 commented on an outdated diff Aug 29, 2015
daemon/graphdriver/devmapper/README.md
@@ -3,12 +3,21 @@
### Theory of operation
The device mapper graphdriver uses the device mapper thin provisioning
-module (dm-thinp) to implement CoW snapshots. For each devicemapper
-graph location (typically `/var/lib/docker/devicemapper`, $graph below)
-a thin pool is created based on two block devices, one for data and
-one for metadata. By default these block devices are created
-automatically by using loopback mounts of automatically created sparse
-files.
+module (dm-thinp) to implement CoW snapshots. The preferred model is
+to have a thin pool reserved outside of Docker, and passed to the
+daemon via the `--storage-opt dm.thinpooldev` option.
+
+As a fallback if no thin pool is provided, loopback files will be
+created. Loopback is very slow, but can be used without any
+pre-configuration of storage. It is *strongly recommended* to not use
@jamtur01
jamtur01 Aug 29, 2015 Contributor

This phraseology is very awkward - perhaps: "It is strongly recommended that you do not use loopback in production." Ditto below.

@jamtur01 jamtur01 commented on an outdated diff Aug 29, 2015
daemon/graphdriver/devmapper/README.md
-a thin pool is created based on two block devices, one for data and
-one for metadata. By default these block devices are created
-automatically by using loopback mounts of automatically created sparse
-files.
+module (dm-thinp) to implement CoW snapshots. The preferred model is
+to have a thin pool reserved outside of Docker, and passed to the
+daemon via the `--storage-opt dm.thinpooldev` option.
+
+As a fallback if no thin pool is provided, loopback files will be
+created. Loopback is very slow, but can be used without any
+pre-configuration of storage. It is *strongly recommended* to not use
+loopback in production. Ensure your docker daemon has a
+`--storage-opt dm.thinpooldev` argument provided.
+
+In loopback, each devicemapper graph location (typically
+`/var/lib/docker/devicemapper`, $graph below) a thin pool is created
@jamtur01
jamtur01 Aug 29, 2015 Contributor

The $graph reference isn't clear - I'd rewrite this sentence.

@jamtur01 jamtur01 commented on an outdated diff Aug 29, 2015
docs/reference/commandline/daemon.md
@@ -192,6 +192,12 @@ options for `zfs` start with `zfs`.
resize support, dynamically changing thin-pool features, automatic thinp
metadata checking when lvm activates the thin-pool, etc.
+ As a fallback if no thin pool is provided, loopback files will be
+ created. Loopback is very slow, but can be used without any
+ pre-configuration of storage. It is *strongly recommended* to not use
+ loopback in production. Ensure your docker daemon has a
@jamtur01
jamtur01 Aug 29, 2015 Contributor

Docker

@jamtur01 jamtur01 commented on an outdated diff Aug 29, 2015
daemon/graphdriver/devmapper/README.md
@@ -3,12 +3,21 @@
### Theory of operation
The device mapper graphdriver uses the device mapper thin provisioning
-module (dm-thinp) to implement CoW snapshots. For each devicemapper
-graph location (typically `/var/lib/docker/devicemapper`, $graph below)
-a thin pool is created based on two block devices, one for data and
-one for metadata. By default these block devices are created
-automatically by using loopback mounts of automatically created sparse
-files.
+module (dm-thinp) to implement CoW snapshots. The preferred model is
+to have a thin pool reserved outside of Docker, and passed to the
+daemon via the `--storage-opt dm.thinpooldev` option.
+
+As a fallback if no thin pool is provided, loopback files will be
+created. Loopback is very slow, but can be used without any
+pre-configuration of storage. It is *strongly recommended* to not use
+loopback in production. Ensure your docker daemon has a
@jamtur01
jamtur01 Aug 29, 2015 Contributor

Docker

@shishir-a412ed
Contributor

@jamtur01 Please check now.

@jamtur01 jamtur01 and 1 other commented on an outdated diff Aug 31, 2015
daemon/graphdriver/devmapper/README.md
@@ -3,22 +3,33 @@
### Theory of operation
The device mapper graphdriver uses the device mapper thin provisioning
-module (dm-thinp) to implement CoW snapshots. For each devicemapper
-graph location (typically `/var/lib/docker/devicemapper`, $graph below)
-a thin pool is created based on two block devices, one for data and
-one for metadata. By default these block devices are created
-automatically by using loopback mounts of automatically created sparse
+module (dm-thinp) to implement CoW snapshots. The preferred model is
+to have a thin pool reserved outside of Docker, and passed to the
@jamtur01
jamtur01 Aug 31, 2015 Contributor

You don't need a comma here after Docker but no big deal.

@jamtur01
Contributor

Docs broadly LGTM

@shishir-a412ed shishir-a412ed Warning message for lvm devmapper running on top of loopback devices
Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
4870fb3
@thaJeztah
Member

docs LGTM, thanks @shishir-a412ed!

@thaJeztah thaJeztah merged commit 81d6f35 into docker:master Aug 31, 2015
@shishir-a412ed shishir-a412ed deleted the shishir-a412ed:docker_lvm_devmapper_loopbackdevices branch Oct 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment