-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
daemon option (--storage-opt dm.basesize) for increasing the base device size on daemon restart #19123
Conversation
With this patch the user can pass the container's filesystem size at creation time. User can grow the FS but cannot shrink it. e.g if the Basedevice FS size is 100G. e.g $ docker create -it --storage-opt size:120G fedora /bin/bash Shishir |
@shishir-a412ed @rhvgoyal I think this closes #14678 ? |
@thaJeztah right. |
cdacd27
to
80c2d55
Compare
we should revert the default value to 10G or 20G if growing is okay but shrinking is not allowed. |
@calavera makes sense. Shishir, can you update the patch to also change default base size value to say 10G. |
@rhvgoyal does this only work for loop, or also for direct-lvm (I'm not too fluent in devicemapper) |
80c2d55
to
1ace6fe
Compare
@rhvgoyal @thaJeztah @calavera Added tests. Shishir |
@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. |
c024225
to
c95415a
Compare
logrus.Debugf("Failed to grow rootfs:%v:%s", err, string(out)) | ||
} | ||
default: | ||
err = fmt.Errorf("Unsupported filesystem type %s", devices.BaseDeviceFilesystem) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
I think we should include this for 1.10, but we might be slightly constraint in time. I'm going to add the I don't think we should add more features here, like version logic detection, so we have less things to discuss. |
Getting this into docker-1.10 would be great. |
This PR brings the StorageOpt to the api. We should be good to go once it's merged: |
Can someone please explain:
I'm not contesting this PR, I'm just trying to understand the requirement here. Thanks! |
Agree but I think we shouldn't error out if |
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? |
37b537b
to
17dd1ce
Compare
|
||
Example use: `docker daemon --storage-opt dm.basesize=50G` | ||
|
||
This will increase the base device size to 50G. docker daemon will throw an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Docker daemon
Docs mostly LGTM - minor comments. |
17dd1ce
to
d70745e
Compare
@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. |
One comment still not addressed. Thanks! |
d70745e
to
d7bd45b
Compare
@jamtur01 PTAL. |
Docs LGTM |
|
||
Example use: | ||
|
||
$ docker daemon --storage-opt dm.basesize=50G` |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
two formatting comments, but LGTM otherwise |
…ice size on daemon restart Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>
d7bd45b
to
e47112d
Compare
docs LGTM, thanks! |
actually, looks like there's one comment not addressed; #19123 (comment) can you take care of that @shishir-a412ed ? |
thanks @shishir-a412ed, LGTM! |
daemon option (--storage-opt dm.basesize) for increasing the base device size on daemon restart
Signed-off-by: Shishir Mahajan shishir.mahajan@redhat.com
Fixes #18314