Skip to content
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

daemon option (--storage-opt dm.basesize) for increasing the base device size on daemon restart #19123

Merged
merged 1 commit into from
Jan 13, 2016

Conversation

shishir-a412ed
Copy link
Contributor

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

Fixes #18314

@shishir-a412ed
Copy link
Contributor Author

With this patch the user can pass the container's filesystem size at creation time.
I have added CLI options for docker create and run.

User can grow the FS but cannot shrink it. e.g if the Basedevice FS size is 100G.
User can specify --storage-opt size:120G to grow the container's filesystem size to 120GB. However if the user tries to pass --storage-opt size:80G daemon will throw an error.

e.g $ docker create -it --storage-opt size:120G fedora /bin/bash
$ docker run -it --storage-opt size:120G fedora /bin/bash

Shishir

@thaJeztah
Copy link
Member

@shishir-a412ed @rhvgoyal I think this closes #14678 ?

@rhvgoyal
Copy link
Contributor

rhvgoyal commented Jan 6, 2016

@thaJeztah right.

@shishir-a412ed shishir-a412ed force-pushed the rootfs_size_configurable branch from cdacd27 to 80c2d55 Compare January 6, 2016 16:34
@calavera
Copy link
Contributor

calavera commented Jan 6, 2016

we should revert the default value to 10G or 20G if growing is okay but shrinking is not allowed.

@rhvgoyal
Copy link
Contributor

rhvgoyal commented Jan 6, 2016

@calavera makes sense. Shishir, can you update the patch to also change default base size value to say 10G.

@thaJeztah
Copy link
Member

@rhvgoyal does this only work for loop, or also for direct-lvm (I'm not too fluent in devicemapper)

@shishir-a412ed shishir-a412ed force-pushed the rootfs_size_configurable branch from 80c2d55 to 1ace6fe Compare January 6, 2016 17:46
@shishir-a412ed
Copy link
Contributor Author

@rhvgoyal
Changed the default base size to 10G.

@thaJeztah @calavera Added tests.

Shishir

@rhvgoyal
Copy link
Contributor

rhvgoyal commented Jan 6, 2016

@thaJeztah This works for devicemapper graph driver. One could be using loop devices or regular block devices and it does not matter. That's underlying detail. So yes, it will work for loop devices as well.

This size parameter will not work for other graph drivers like, overlay, btrfs, aufs and zfs.

@shishir-a412ed shishir-a412ed force-pushed the rootfs_size_configurable branch 2 times, most recently from c024225 to c95415a Compare January 7, 2016 15:14
logrus.Debugf("Failed to grow rootfs:%v:%s", err, string(out))
}
default:
err = fmt.Errorf("Unsupported filesystem type %s", devices.BaseDeviceFilesystem)
Copy link
Contributor

Choose a reason for hiding this comment

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

this error is never returned, you need to add return err at the end of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@calavera
Copy link
Contributor

calavera commented Jan 7, 2016

I think we should include this for 1.10, but we might be slightly constraint in time.

I'm going to add the StorageOpt in https://github.com/docker/engine-api so we can make this happen.

I don't think we should add more features here, like version logic detection, so we have less things to discuss.

@rhatdan
Copy link
Contributor

rhatdan commented Jan 7, 2016

Getting this into docker-1.10 would be great.

@calavera
Copy link
Contributor

calavera commented Jan 8, 2016

This PR brings the StorageOpt to the api. We should be good to go once it's merged:

#19173

@icecrime
Copy link
Contributor

icecrime commented Jan 8, 2016

Can someone please explain:

  • What are the implication of the initial size? In particular, I've heard about the tradeoff of a too important initial size, but what are the drawbacks of a smaller default (such as the 10G we use to have)?
  • Why would I want to pick a bigger one from the start?

I'm not contesting this PR, I'm just trying to understand the requirement here. Thanks!

@runcom
Copy link
Member

runcom commented Jan 8, 2016

@rhvgoyal

This size parameter will not work for other graph drivers like, overlay, btrfs, aufs and zfs.

Agree but I think we shouldn't error out if storageOpt is provided to other graph drivers. Silently ignore not supported storage options would be better if in the future we will add some to the other graph drivers (also, does the daemon ignore --storage-opt on other graph drivers as well?)

@rhvgoyal
Copy link
Contributor

rhvgoyal commented Jan 8, 2016

@runcom

I think if you pass --storage-opt to other graph drivers, then there is no error. It is ignored silently. BUT, if you pass a non-supported option to device mapper graph driver it errors out.

I personally feel that it probably is better to error out on unsupported option. For example if somebody is trying to shrink container size we should return error instead of ignoring it.

If we implement options for other graph drivers later, then we will stop erroring out on valid options.

IOW, what's the advantage of ignoring storage-opt which are not supported?

@shishir-a412ed shishir-a412ed force-pushed the rootfs_size_configurable branch 3 times, most recently from 37b537b to 17dd1ce Compare January 13, 2016 17:37
@tiborvass
Copy link
Contributor

Ping @vdemeester @MHBauer @thaJeztah


Example use: `docker daemon --storage-opt dm.basesize=50G`

This will increase the base device size to 50G. docker daemon will throw an
Copy link
Contributor

Choose a reason for hiding this comment

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

The Docker daemon

@jamtur01
Copy link
Contributor

Docs mostly LGTM - minor comments.

@shishir-a412ed shishir-a412ed force-pushed the rootfs_size_configurable branch from 17dd1ce to d70745e Compare January 13, 2016 17:46
@shishir-a412ed
Copy link
Contributor Author

@jamtur01 check now. I have kept my example in the same format as the example below (docker daemon --storage-opt dm.basesize=20G ) in that section.

@jamtur01
Copy link
Contributor

One comment still not addressed. Thanks!

@shishir-a412ed shishir-a412ed force-pushed the rootfs_size_configurable branch from d70745e to d7bd45b Compare January 13, 2016 17:59
@shishir-a412ed
Copy link
Contributor Author

@jamtur01 PTAL.

@jamtur01
Copy link
Contributor

Docs LGTM


Example use:

$ docker daemon --storage-opt dm.basesize=50G`
Copy link
Member

Choose a reason for hiding this comment

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

Can you change the indentation here to 8 spaces, otherwise it won't render as a code-block

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and I missed the trailing ` - you should remove that.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@thaJeztah
Copy link
Member

two formatting comments, but LGTM otherwise

…ice size on daemon restart

Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
@shishir-a412ed shishir-a412ed force-pushed the rootfs_size_configurable branch from d7bd45b to e47112d Compare January 13, 2016 18:57
@calavera
Copy link
Contributor

docs LGTM, thanks!

@thaJeztah
Copy link
Member

actually, looks like there's one comment not addressed; #19123 (comment)

can you take care of that @shishir-a412ed ?

@thaJeztah
Copy link
Member

thanks @shishir-a412ed, LGTM!

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

Successfully merging this pull request may close these issues.