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

Add option to auto-configure blkdev for devmapper #31104

Merged
merged 1 commit into from May 5, 2017

Conversation

@cpuguy83
Contributor

cpuguy83 commented Feb 16, 2017

Instead of forcing users to manually configure a block device to use
with devmapper, this gives the user the option to let the devmapper
driver configure a device for them.

Adds several new options to the devmapper storage-opts:

  • dm.directlvm_device="" - path to the block device to configure for
    direct-lvm
  • dm.thinp_percent=95 - sets the percentage of space to use for
    storage from the passed in block device
  • dm.thinp_metapercent=1 - sets the percentage of space to for metadata
    storage from the passed in block device
  • dm.thinp_autoextend_threshold=80 - sets the threshold for when lvm
    should automatically extend the thin pool as a percentage of the total
    storage space
  • dm.thinp_autoextend_percent=20 - sets the percentage to increase the
    thin pool by when an autoextend is triggered.

Defaults are taken from here

The only option that is required is dm.directlvm_device for docker to
set everything up.

Changes to these settings are not currently supported and will error
out.
Future work could support allowing changes to these values.

ping @rhvgoyal

@cpuguy83 cpuguy83 changed the title from Signed-off-by: Brian Goff <cpuguy83@gmail.com> to https://docs.docker.com/engine/userguide/storagedriver/device-mapper-driver/#/configure-direct-lvm-mode-for-production Feb 16, 2017

@cpuguy83 cpuguy83 changed the title from https://docs.docker.com/engine/userguide/storagedriver/device-mapper-driver/#/configure-direct-lvm-mode-for-production to Add option to auto-configure blkdev for devmapper Feb 16, 2017

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Feb 17, 2017

Contributor

Nice. It was time that docker had all this functionality. We had been doing this using docker-storage-setup service.

Does this access multiple block devices also?

So docker will setup the storage and cleaning up or freeing up that device will be manually done by user?

Contributor

rhvgoyal commented Feb 17, 2017

Nice. It was time that docker had all this functionality. We had been doing this using docker-storage-setup service.

Does this access multiple block devices also?

So docker will setup the storage and cleaning up or freeing up that device will be manually done by user?

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Feb 17, 2017

Contributor

So on a reboot we rely on lvm to auto activate VG and thin pool lv which are previously setup.

At some point of time, we probably will require a notion of waiting for device. Over a reboot, sometimes device might not be available immediately or is slow to come up, hence thin pool will not be available immediately and docker should ideally wait till user configurable timeout. But that can probably be an extension for later.

Contributor

rhvgoyal commented Feb 17, 2017

So on a reboot we rely on lvm to auto activate VG and thin pool lv which are previously setup.

At some point of time, we probably will require a notion of waiting for device. Over a reboot, sometimes device might not be available immediately or is slow to come up, hence thin pool will not be available immediately and docker should ideally wait till user configurable timeout. But that can probably be an extension for later.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 17, 2017

Contributor

Does this access multiple block devices also?

I suppose multiple device support is just a ux concern (ie, how do we support this in the options). I think that would be discussed on a separate PR just so we can get something in more easily.

So docker will setup the storage and cleaning up or freeing up that device will be manually done by user?

This is how it is currently setup. I would be a bit leary of doing automatic cleanup without some explicit user action... could be yet another dm option... ie if "dm.clean_all_the_things=true" then look at the previous config and wipe it out.... but still seems dangerous to automatically clean up.
How does docker-storage-setup handle this?

At some point of time, we probably will require a notion of waiting for device

Interesting. I think this is likely a problem today.

Contributor

cpuguy83 commented Feb 17, 2017

Does this access multiple block devices also?

I suppose multiple device support is just a ux concern (ie, how do we support this in the options). I think that would be discussed on a separate PR just so we can get something in more easily.

So docker will setup the storage and cleaning up or freeing up that device will be manually done by user?

This is how it is currently setup. I would be a bit leary of doing automatic cleanup without some explicit user action... could be yet another dm option... ie if "dm.clean_all_the_things=true" then look at the previous config and wipe it out.... but still seems dangerous to automatically clean up.
How does docker-storage-setup handle this?

At some point of time, we probably will require a notion of waiting for device

Interesting. I think this is likely a problem today.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 9, 2017

Member

ping @rhvgoyal would you be interested in reviewing this PR?

Member

thaJeztah commented Mar 9, 2017

ping @rhvgoyal would you be interested in reviewing this PR?

@thaJeztah thaJeztah requested a review from estesp Mar 9, 2017

@thaJeztah thaJeztah removed this from backlog in maintainers-session Mar 9, 2017

@estesp

Some basic typo-like changes requested, but overall it looks straightforward. Wondering about whether any validation is necessary on the devicename before passing it to pvcreate.

Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go Outdated
Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go Outdated
Show outdated Hide outdated daemon/graphdriver/devmapper/deviceset.go Outdated
@@ -2698,11 +2726,55 @@ func NewDeviceSet(root string, doInit bool, options []string, uidMaps, gidMaps [
return nil, err
}
devices.xfsNospaceRetries = val
case "dm.directlvm_device":
lvmSetupConfig.Device = val

This comment has been minimized.

@estesp

estesp Mar 10, 2017

Contributor

is it worth doing any validation that this is an actual device on the system? Obviously pvcreate will error out in some way if this is invalid, but maybe safer to do some simple validation. What does pvcreate do if I point it at my already mounted disk in some other filesystem format? I assume the error cases are "safe" but worth getting @rhvgoyal input on this..

@estesp

estesp Mar 10, 2017

Contributor

is it worth doing any validation that this is an actual device on the system? Obviously pvcreate will error out in some way if this is invalid, but maybe safer to do some simple validation. What does pvcreate do if I point it at my already mounted disk in some other filesystem format? I assume the error cases are "safe" but worth getting @rhvgoyal input on this..

This comment has been minimized.

@cpuguy83

cpuguy83 Mar 23, 2017

Contributor

If there is a filesystem on the device, pvcreate asks if it should be wiped. Since there's no stdin for the process it causes docker to error out, with the output from pvcreate in the logs.
This is a pretty bad experience, though at least it won't overwrite someone's existing stuff automatically.

We can check 2 things:

  1. Does this device exists
  2. Is there a filesystem on the device

For check 2, we can we can add an option to allow forcing the overwrite but I do not think we should just overwrite by default.

@cpuguy83

cpuguy83 Mar 23, 2017

Contributor

If there is a filesystem on the device, pvcreate asks if it should be wiped. Since there's no stdin for the process it causes docker to error out, with the output from pvcreate in the logs.
This is a pretty bad experience, though at least it won't overwrite someone's existing stuff automatically.

We can check 2 things:

  1. Does this device exists
  2. Is there a filesystem on the device

For check 2, we can we can add an option to allow forcing the overwrite but I do not think we should just overwrite by default.

This comment has been minimized.

@cpuguy83

cpuguy83 Mar 23, 2017

Contributor

Updated this to do some verification on the device.

@cpuguy83

cpuguy83 Mar 23, 2017

Contributor

Updated this to do some verification on the device.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 22, 2017

Contributor

Hmm, I'm not sure about validating dev name.
We could see if we can stat it, but likely it's just extra work, and probably will have to take symlinks into account, etc.

Contributor

cpuguy83 commented Mar 22, 2017

Hmm, I'm not sure about validating dev name.
We could see if we can stat it, but likely it's just extra work, and probably will have to take symlinks into account, etc.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 22, 2017

Contributor

Updated to fix typos.

Contributor

cpuguy83 commented Mar 22, 2017

Updated to fix typos.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester
Member

vdemeester commented Apr 3, 2017

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 12, 2017

Contributor

ping @estesp

Contributor

cpuguy83 commented Apr 12, 2017

ping @estesp

@estesp

estesp approved these changes Apr 13, 2017

LGTM

@thaJeztah thaJeztah added this to the 17.06 milestone Apr 26, 2017

@thaJeztah thaJeztah added this to backlog in maintainers-session Apr 27, 2017

@mbentley

This comment has been minimized.

Show comment
Hide comment
@mbentley

mbentley Apr 28, 2017

Contributor

Looked at the implementation; it's fairly straight forwarding for a fresh install. This will require some additional documentation on the automatic vs. manual usage which would work well in https://github.com/docker/docker.github.io/blob/master/engine/userguide/storagedriver/device-mapper-driver.md.

The only thing I found that was really unexpected is that the dm.directlvm_device_force=true option will not remove any existing LVs, VGs or PVs which is a bit difficult to detect and clear cleanly.

Contributor

mbentley commented Apr 28, 2017

Looked at the implementation; it's fairly straight forwarding for a fresh install. This will require some additional documentation on the automatic vs. manual usage which would work well in https://github.com/docker/docker.github.io/blob/master/engine/userguide/storagedriver/device-mapper-driver.md.

The only thing I found that was really unexpected is that the dm.directlvm_device_force=true option will not remove any existing LVs, VGs or PVs which is a bit difficult to detect and clear cleanly.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 28, 2017

Contributor

@mbentley

The only thing I found that was really unexpected is that the dm.directlvm_device_force=true option will not remove any existing LVs, VGs or PVs which is a bit difficult to detect and clear cleanly.

Can you explain this? You are saying you are trying to use a device that is already part of some previous DM setup?

Contributor

cpuguy83 commented Apr 28, 2017

@mbentley

The only thing I found that was really unexpected is that the dm.directlvm_device_force=true option will not remove any existing LVs, VGs or PVs which is a bit difficult to detect and clear cleanly.

Can you explain this? You are saying you are trying to use a device that is already part of some previous DM setup?

@mbentley

This comment has been minimized.

Show comment
Hide comment
@mbentley

mbentley Apr 28, 2017

Contributor

Right. If the device previously had any previous devicemapper configuration, it fails to force a wipe. Not sure how it would even be possible to detect whether or not it is the correct device that is already setup; maybe just some clarification about what dm.directlvm_device_force=true is intended to be used for would be fine.

Quick example would be to allow this code to run to set up DM, remove everything in /var/lib/docker and try to run the setup again with the force option and it fails to clear the existing device because it has an existing logical volume, volume group, and physical volume setup in DM already for the device.

Contributor

mbentley commented Apr 28, 2017

Right. If the device previously had any previous devicemapper configuration, it fails to force a wipe. Not sure how it would even be possible to detect whether or not it is the correct device that is already setup; maybe just some clarification about what dm.directlvm_device_force=true is intended to be used for would be fine.

Quick example would be to allow this code to run to set up DM, remove everything in /var/lib/docker and try to run the setup again with the force option and it fails to clear the existing device because it has an existing logical volume, volume group, and physical volume setup in DM already for the device.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 3, 2017

Contributor

@mbentley You are right this is pretty tricky.
One thing we can do is at least check if the device is part of a volume group and then tell them to remove it first, I don't think it's particularly safe to try and automate this.

Contributor

cpuguy83 commented May 3, 2017

@mbentley You are right this is pretty tricky.
One thing we can do is at least check if the device is part of a volume group and then tell them to remove it first, I don't think it's particularly safe to try and automate this.

@mbentley

This comment has been minimized.

Show comment
Hide comment
@mbentley

mbentley May 3, 2017

Contributor

@cpuguy83 I would agree - that sounds like a safe approach.

Contributor

mbentley commented May 3, 2017

@cpuguy83 I would agree - that sounds like a safe approach.

Add option to auto-configure blkdev for devmapper
Instead of forcing users to manually configure a block device to use
with devmapper, this gives the user the option to let the devmapper
driver configure a device for them.

Adds several new options to the devmapper storage-opts:

- dm.directlvm_device="" - path to the block device to configure for
  direct-lvm
- dm.thinp_percent=95 - sets the percentage of space to use for
  storage from the passed in block device
- dm.thinp_metapercent=1 - sets the percentage of space to for metadata
  storage from the passed in block device
- dm.thinp_autoextend_threshold=80 - sets the threshold for when `lvm`
  should automatically extend the thin pool as a percentage of the total
  storage space
- dm.thinp_autoextend_percent=20 - sets the percentage to increase the
  thin pool by when an autoextend is triggered.

Defaults are taken from
[here](https://docs.docker.com/engine/userguide/storagedriver/device-mapper-driver/#/configure-direct-lvm-mode-for-production)

The only option that is required is `dm.directlvm_device` for docker to
set everything up.

Changes to these settings are not currently supported and will error
out.
Future work could support allowing changes to these values.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 3, 2017

Contributor

Updated this to check for volume group as discussed above.

Contributor

cpuguy83 commented May 3, 2017

Updated this to check for volume group as discussed above.

@thaJeztah thaJeztah removed this from backlog in maintainers-session May 4, 2017

@tonistiigi

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 4, 2017

Member

This probably needs an update to the storage driver section in the docs repository; https://github.com/docker/docker.github.io/blob/vnext-engine/engine/userguide/storagedriver/device-mapper-driver.md#configure-direct-lvm-mode-for-production, but otherwise looks like it's ready to go

/cc @albers @sdurrheimer for completion scripts

Member

thaJeztah commented May 4, 2017

This probably needs an update to the storage driver section in the docs repository; https://github.com/docker/docker.github.io/blob/vnext-engine/engine/userguide/storagedriver/device-mapper-driver.md#configure-direct-lvm-mode-for-production, but otherwise looks like it's ready to go

/cc @albers @sdurrheimer for completion scripts

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 5, 2017

Contributor

powerpc issue is known.
Merging.

Contributor

cpuguy83 commented May 5, 2017

powerpc issue is known.
Merging.

@cpuguy83 cpuguy83 merged commit 05ad14f into moby:master May 5, 2017

5 of 6 checks passed

powerpc Jenkins build Docker-PRs-powerpc 2603 has encountered an error
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 33603 has succeeded
Details
janky Jenkins build Docker-PRs 42209 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13412 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2290 has succeeded
Details

@cpuguy83 cpuguy83 deleted the cpuguy83:dm_lvmsetup branch May 5, 2017

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