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 disk quota support for btrfs #19651

Merged
merged 1 commit into from May 5, 2016

Conversation

Projects
None yet
@zhuguihua
Contributor

zhuguihua commented Jan 25, 2016

This patch is adding disk quota support for btrfs.
We can use this patch like:
docker daemon --storage-opt btrfs.min_space=xx_
docker run --storage-opt size=xx

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 25, 2016

Member

Can you sign-off your commit? Otherwise we won't be able to review this.

w.r.t the flag; I wonder if it should be, for example --storage-opt btrfs.disk-quota=xx, however, there's been some discussion around storage driver options on a per-container base (see #19367, #19367) so this may need some more discussion.

/cc @tiborvass

Member

thaJeztah commented Jan 25, 2016

Can you sign-off your commit? Otherwise we won't be able to review this.

w.r.t the flag; I wonder if it should be, for example --storage-opt btrfs.disk-quota=xx, however, there's been some discussion around storage driver options on a per-container base (see #19367, #19367) so this may need some more discussion.

/cc @tiborvass

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Jan 25, 2016

Contributor

@thaJeztah I will add sign-off later. Thanks for your comments, and welcome more discussion about this.

Contributor

zhuguihua commented Jan 25, 2016

@thaJeztah I will add sign-off later. Thanks for your comments, and welcome more discussion about this.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 25, 2016

Member

Thanks @zhuguihua, I like the feature; But we should get agreement on the UX.

Member

thaJeztah commented Jan 25, 2016

Thanks @zhuguihua, I like the feature; But we should get agreement on the UX.

Show outdated Hide outdated runconfig/opts/parse.go
@@ -78,6 +78,7 @@ func Parse(cmd *flag.FlagSet, args []string) (*container.Config, *container.Host
flCPUQuota = cmd.Int64([]string{"-cpu-quota"}, 0, "Limit CPU CFS (Completely Fair Scheduler) quota")
flCpusetCpus = cmd.String([]string{"-cpuset-cpus"}, "", "CPUs in which to allow execution (0-3, 0,1)")
flCpusetMems = cmd.String([]string{"-cpuset-mems"}, "", "MEMs in which to allow execution (0-3, 0,1)")
flDiskQuota = cmd.String([]string{"-disk-quota"}, "", "Limit disk quota (Now only support btrfs)")

This comment has been minimized.

@icecrime

icecrime Feb 1, 2016

Contributor

I don't think we need a new top-level flag here: we should reuse --storage-opt.

@icecrime

icecrime Feb 1, 2016

Contributor

I don't think we need a new top-level flag here: we should reuse --storage-opt.

This comment has been minimized.

@thaJeztah

thaJeztah Feb 1, 2016

Member

Was thinking the same, see #19651 (comment)

@thaJeztah

thaJeztah Feb 1, 2016

Member

Was thinking the same, see #19651 (comment)

This comment has been minimized.

@zhuguihua

zhuguihua Feb 2, 2016

Contributor

Previously, --disk-quota is designed for a per-container base, and for all storage backends. So now we should only add --storage-opt btrfs.disk-quota for btrfs, for all images and containers. Is it right?

@zhuguihua

zhuguihua Feb 2, 2016

Contributor

Previously, --disk-quota is designed for a per-container base, and for all storage backends. So now we should only add --storage-opt btrfs.disk-quota for btrfs, for all images and containers. Is it right?

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 9, 2016

Contributor

yes, there's another PR to introduce this for other reasons, I think it's fitting.

@cpuguy83

cpuguy83 Feb 9, 2016

Contributor

yes, there's another PR to introduce this for other reasons, I think it's fitting.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Feb 5, 2016

Contributor

FYI - Compile fails on Windows:

00:02:42.400 ---> Making bundle: binary (in bundles/1.11.0-dev/binary)
00:02:42.985 Building: bundles/1.11.0-dev/binary/docker-1.11.0-dev.exe
00:02:52.407 # github.com/docker/docker/runconfig/opts
00:02:52.407 C:\Go\src\github.com\docker\docker\runconfig\opts\parse.go:355: unknown container.Resources field 'DiskQuota' in struct literal
00:02:54.721 # github.com/docker/docker/daemon/graphdriver/windows
00:02:54.721 C:\Go\src\github.com\docker\docker\daemon\graphdriver\windows\windows.go:61: cannot use d (type *Driver) as type graphdriver.Driver in return argument:
00:02:54.721    *Driver does not implement graphdriver.Driver (missing SetDiskQuota method)
00:02:54.722 C:\Go\src\github.com\docker\docker\daemon\graphdriver\windows\windows.go:74: cannot use d (type *Driver) as type graphdriver.Driver in return argument:
00:02:54.722    *Driver does not implement graphdriver.Driver (missing SetDiskQuota method)
Contributor

jhowardmsft commented Feb 5, 2016

FYI - Compile fails on Windows:

00:02:42.400 ---> Making bundle: binary (in bundles/1.11.0-dev/binary)
00:02:42.985 Building: bundles/1.11.0-dev/binary/docker-1.11.0-dev.exe
00:02:52.407 # github.com/docker/docker/runconfig/opts
00:02:52.407 C:\Go\src\github.com\docker\docker\runconfig\opts\parse.go:355: unknown container.Resources field 'DiskQuota' in struct literal
00:02:54.721 # github.com/docker/docker/daemon/graphdriver/windows
00:02:54.721 C:\Go\src\github.com\docker\docker\daemon\graphdriver\windows\windows.go:61: cannot use d (type *Driver) as type graphdriver.Driver in return argument:
00:02:54.721    *Driver does not implement graphdriver.Driver (missing SetDiskQuota method)
00:02:54.722 C:\Go\src\github.com\docker\docker\daemon\graphdriver\windows\windows.go:74: cannot use d (type *Driver) as type graphdriver.Driver in return argument:
00:02:54.722    *Driver does not implement graphdriver.Driver (missing SetDiskQuota method)
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 15, 2016

Member

ping @zhuguihua were you working on this?

Member

thaJeztah commented Feb 15, 2016

ping @zhuguihua were you working on this?

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Feb 16, 2016

Contributor

@thaJeztah Oh, sorry. I have been on vacation for Chinese new year before. And I am still working on this, I will update this PR in a day or two. Thanks.

Contributor

zhuguihua commented Feb 16, 2016

@thaJeztah Oh, sorry. I have been on vacation for Chinese new year before. And I am still working on this, I will update this PR in a day or two. Thanks.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 16, 2016

Member

@zhuguihua no problem; looks like you pushed new changes? Is this ready for review?

Member

thaJeztah commented Feb 16, 2016

@zhuguihua no problem; looks like you pushed new changes? Is this ready for review?

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Feb 16, 2016

Contributor

@thaJeztah Yes, I updated this, and this is ready for review.

Contributor

zhuguihua commented Feb 16, 2016

@thaJeztah Yes, I updated this, and this is ready for review.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Feb 18, 2016

Contributor

LGTM.

The new option needs to be properly documented.

Contributor

calavera commented Feb 18, 2016

LGTM.

The new option needs to be properly documented.

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Feb 19, 2016

Contributor

@calavera Thanks your review. Will add docs later.

Contributor

zhuguihua commented Feb 19, 2016

@calavera Thanks your review. Will add docs later.

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Feb 19, 2016

Contributor

I found a bug about this PR. If I load a image, and its size is larger than btrfs.diskquota, it will cause a error. So is it necessary to set a default disk quota? Maybe its value 100G or others?

Contributor

zhuguihua commented Feb 19, 2016

I found a bug about this PR. If I load a image, and its size is larger than btrfs.diskquota, it will cause a error. So is it necessary to set a default disk quota? Maybe its value 100G or others?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 22, 2016

Contributor

@zhuguihua Isn't this the expected behavior? Or should this only really apply to r/w layers?

Contributor

cpuguy83 commented Feb 22, 2016

@zhuguihua Isn't this the expected behavior? Or should this only really apply to r/w layers?

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Feb 23, 2016

Contributor

@cpuguy83 I am sorry that I did not express clearly. I'd like to set a default disk quota to prevent users from setting the quota value too low, for example 10M. Is it necessary?

Contributor

zhuguihua commented Feb 23, 2016

@cpuguy83 I am sorry that I did not express clearly. I'd like to set a default disk quota to prevent users from setting the quota value too low, for example 10M. Is it necessary?

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Feb 29, 2016

Contributor

Some go fmt issue:

04:00:25 ---> Making bundle: validate-gofmt (in bundles/1.11.0-dev/validate-gofmt)
04:00:25 These files are not properly gofmt'd:
04:00:25  - daemon/graphdriver/btrfs/btrfs.go
04:00:25 
04:00:25 Please reformat the above files using "gofmt -s -w" and commit the result.

Thanks!

Contributor

icecrime commented Feb 29, 2016

Some go fmt issue:

04:00:25 ---> Making bundle: validate-gofmt (in bundles/1.11.0-dev/validate-gofmt)
04:00:25 These files are not properly gofmt'd:
04:00:25  - daemon/graphdriver/btrfs/btrfs.go
04:00:25 
04:00:25 Please reformat the above files using "gofmt -s -w" and commit the result.

Thanks!

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Feb 29, 2016

Contributor

@icecrime Fixed, also passed CI. Thanks.

Contributor

zhuguihua commented Feb 29, 2016

@icecrime Fixed, also passed CI. Thanks.

Show outdated Hide outdated daemon/graphdriver/btrfs/btrfs.go
"github.com/opencontainers/runc/libcontainer/label"
)
var (
defaultDiskQuota uint64 = 10 * 1024 * 1024 * 1024

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 29, 2016

Contributor

nit, this is minDiskQuota not defaultDiskQuota

@cpuguy83

cpuguy83 Feb 29, 2016

Contributor

nit, this is minDiskQuota not defaultDiskQuota

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 29, 2016

Contributor

What's weird about this is it's not really so much a quota as a disk size for layers.
Really I think there's likely 2 options to have for quota... a max image size and a max container FS size.

Contributor

cpuguy83 commented Feb 29, 2016

What's weird about this is it's not really so much a quota as a disk size for layers.
Really I think there's likely 2 options to have for quota... a max image size and a max container FS size.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 29, 2016

Member

Yes, I can imaging people want to limit a container from taking up too much disk space, but still want to be able to pull that large image that's needed to run the container

Member

thaJeztah commented Feb 29, 2016

Yes, I can imaging people want to limit a container from taking up too much disk space, but still want to be able to pull that large image that's needed to run the container

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 29, 2016

Member

I think we can add this option (--storage-opt) to docker run, so that it can be set for individual containers, similar to #19367. Do we want it on both the daemon and docker run, or only one of them?

Member

thaJeztah commented Feb 29, 2016

I think we can add this option (--storage-opt) to docker run, so that it can be set for individual containers, similar to #19367. Do we want it on both the daemon and docker run, or only one of them?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 15, 2016

Member

ping @cpuguy83 wdyt; should we move this to docker run? Both?

Member

thaJeztah commented Mar 15, 2016

ping @cpuguy83 wdyt; should we move this to docker run? Both?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 16, 2016

Contributor

@thaJeztah Yes for on run. Though I'm not 100% on limiting image layer size. Seems a little weird.

Contributor

cpuguy83 commented Mar 16, 2016

@thaJeztah Yes for on run. Though I'm not 100% on limiting image layer size. Seems a little weird.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 16, 2016

Member

Agreed, I don't think we should limit image layer size; when adding this flag to docker run, I'm thinking it should just limit the writable layer of the container, not affect pulling large images.

If we want to limit total consumption, I'm thinking of having something like #20786

@zhuguihua can you find yourself in that? do you think you can work with that?

Member

thaJeztah commented Mar 16, 2016

Agreed, I don't think we should limit image layer size; when adding this flag to docker run, I'm thinking it should just limit the writable layer of the container, not affect pulling large images.

If we want to limit total consumption, I'm thinking of having something like #20786

@zhuguihua can you find yourself in that? do you think you can work with that?

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Mar 17, 2016

Contributor

@thaJeztah Yes, I can. I will set a min_disk_quota for the minimum quota. Then only limit the writable layer of the container, it is rebased on #19367

Contributor

zhuguihua commented Mar 17, 2016

@thaJeztah Yes, I can. I will set a min_disk_quota for the minimum quota. Then only limit the writable layer of the container, it is rebased on #19367

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 17, 2016

Contributor

Yes, I can. I will set a min_disk_quota for the minimum quota

SGTM

Contributor

calavera commented Mar 17, 2016

Yes, I can. I will set a min_disk_quota for the minimum quota

SGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 23, 2016

Member

@zhuguihua looks like something went wrong during rebasing, there's now a non-related commit in the PR

Member

thaJeztah commented Mar 23, 2016

@zhuguihua looks like something went wrong during rebasing, there's now a non-related commit in the PR

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 23, 2016

Member

Also looks like this needs gofmt;

12:03:01 ---> Making bundle: validate-gofmt (in bundles/1.11.0-dev/validate-gofmt)
12:03:02 These files are not properly gofmt'd:
12:03:02  - daemon/graphdriver/btrfs/btrfs.go
12:03:02 
12:03:02 Please reformat the above files using "gofmt -s -w" and commit the result.
Member

thaJeztah commented Mar 23, 2016

Also looks like this needs gofmt;

12:03:01 ---> Making bundle: validate-gofmt (in bundles/1.11.0-dev/validate-gofmt)
12:03:02 These files are not properly gofmt'd:
12:03:02  - daemon/graphdriver/btrfs/btrfs.go
12:03:02 
12:03:02 Please reformat the above files using "gofmt -s -w" and commit the result.
for key, val := range storageOpt {
key := strings.ToLower(key)
switch key {
case "size":

This comment has been minimized.

@calavera

calavera Mar 31, 2016

Contributor

is this case correct?

@calavera

calavera Mar 31, 2016

Contributor

is this case correct?

This comment has been minimized.

@zhuguihua

zhuguihua Apr 1, 2016

Contributor

@calavera Yes. This option is set per container. It can be used like this:
docker create --storage-opt size=10G busybox /bin/sh

@zhuguihua

zhuguihua Apr 1, 2016

Contributor

@calavera Yes. This option is set per container. It can be used like this:
docker create --storage-opt size=10G busybox /bin/sh

This comment has been minimized.

@thaJeztah

thaJeztah Apr 1, 2016

Member

@calavera yes, size is consistent with #19367 (for devicemapper)

@thaJeztah

thaJeztah Apr 1, 2016

Member

@calavera yes, size is consistent with #19367 (for devicemapper)

This comment has been minimized.

@thaJeztah

thaJeztah Apr 8, 2016

Member

But I think it needs to be updated in the documentation (I noticed the current docs don't mention for which graphdrivers it's supported, which we may want to add)

@thaJeztah

thaJeztah Apr 8, 2016

Member

But I think it needs to be updated in the documentation (I noticed the current docs don't mention for which graphdrivers it's supported, which we may want to add)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Apr 8, 2016

ping @calavera @cpuguy83 PTAL

Show outdated Hide outdated daemon/graphdriver/btrfs/btrfs.go
"github.com/opencontainers/runc/libcontainer/label"
)
func init() {
graphdriver.Register("btrfs", Init)
}
var (
defaultMinFreeSpace uint64 = 10 * 1024 * 1024 * 1024

This comment has been minimized.

@tonistiigi

tonistiigi Apr 14, 2016

Member

Where is this number coming from? From the code it looks more like a limit than a default.

@tonistiigi

tonistiigi Apr 14, 2016

Member

Where is this number coming from? From the code it looks more like a limit than a default.

This comment has been minimized.

@zhuguihua

zhuguihua Apr 15, 2016

Contributor

This number is only refer to devmapper's value defaultBaseFsSize.
Oh, you are right, it is a limit for minimum value of free space.

@zhuguihua

zhuguihua Apr 15, 2016

Contributor

This number is only refer to devmapper's value defaultBaseFsSize.
Oh, you are right, it is a limit for minimum value of free space.

This comment has been minimized.

@tonistiigi

tonistiigi Apr 15, 2016

Member

I see no reason to restrict a 10GB minimum for btrfs.

@tonistiigi

tonistiigi Apr 15, 2016

Member

I see no reason to restrict a 10GB minimum for btrfs.

Show outdated Hide outdated daemon/graphdriver/btrfs/btrfs.go
}
return graphdriver.NewNaiveDiffDriver(driver, uidMaps, gidMaps), nil
}
func parseOptions(opt []string) (btrfsOptions, error) {
var options btrfsOptions
options.quotaEnabled = false

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 15, 2016

Contributor

this should be the default value, no need to set it

@cpuguy83

cpuguy83 Apr 15, 2016

Contributor

this should be the default value, no need to set it

This comment has been minimized.

@zhuguihua

zhuguihua Apr 19, 2016

Contributor

@cpuguy83 I need a value to determine whether to invoke func subvolEnableQuota or not. Thanks.

@zhuguihua

zhuguihua Apr 19, 2016

Contributor

@cpuguy83 I need a value to determine whether to invoke func subvolEnableQuota or not. Thanks.

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 19, 2016

Contributor

Yes, but it's going to be false by default.
For example var foo bool, foo will be false

@cpuguy83

cpuguy83 Apr 19, 2016

Contributor

Yes, but it's going to be false by default.
For example var foo bool, foo will be false

This comment has been minimized.

@zhuguihua

zhuguihua Apr 19, 2016

Contributor

Got it, thanks.

@zhuguihua

zhuguihua Apr 19, 2016

Contributor

Got it, thanks.

Show outdated Hide outdated daemon/graphdriver/btrfs/btrfs.go
@@ -274,6 +407,16 @@ func (d *Driver) Create(id, parent, mountLabel string, storageOpt map[string]str
}
}
if len(storageOpt) != 0 {

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 15, 2016

Contributor

It might be better to be a little more explicit here.
AFAICT if another storage opt is supported later on that has nothing to do with the storage size we'll still set one anyway (or at least try to) and potentially get an error.

@cpuguy83

cpuguy83 Apr 15, 2016

Contributor

It might be better to be a little more explicit here.
AFAICT if another storage opt is supported later on that has nothing to do with the storage size we'll still set one anyway (or at least try to) and potentially get an error.

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Apr 22, 2016

Contributor

Updated, and passed all checks. Welcome more comments.

Contributor

zhuguihua commented Apr 22, 2016

Updated, and passed all checks. Welcome more comments.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 22, 2016

Contributor

Other than it seems like btrfs quotas are really terrible, LGTM!

We'll probably want to add some extra docs for quota quirks... e.g., quota is reached and you can't rm files, but you can echo "" > /file then rm... but only sometimes, you might have to wait a period of time to be able to actually do such an operation.

Contributor

cpuguy83 commented Apr 22, 2016

Other than it seems like btrfs quotas are really terrible, LGTM!

We'll probably want to add some extra docs for quota quirks... e.g., quota is reached and you can't rm files, but you can echo "" > /file then rm... but only sometimes, you might have to wait a period of time to be able to actually do such an operation.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 22, 2016

Member

LGTM

Member

tonistiigi commented Apr 22, 2016

LGTM

Show outdated Hide outdated man/docker-create.1.md
@@ -333,6 +333,7 @@ unit, `b` is used. Set LIMIT to `-1` to enable unlimited swap.
$ docker create -it --storage-opt size=120G fedora /bin/bash
This (size) will allow to set the container rootfs size to 120G at creation time. User cannot pass a size less than the Default BaseFS Size.
Now, the supported graphdrivers for this (size) option are `devicemapper` and `btrfs`.

This comment has been minimized.

@thaJeztah

thaJeztah Apr 26, 2016

Member

perhaps;

This option is only available for the `devicemapper` and `btrfs` graphrivers.
@thaJeztah

thaJeztah Apr 26, 2016

Member

perhaps;

This option is only available for the `devicemapper` and `btrfs` graphrivers.
Show outdated Hide outdated man/docker-run.1.md
@@ -484,7 +484,8 @@ its root filesystem mounted as read only prohibiting any writes.
$ docker run -it --storage-opt size=120G fedora /bin/bash
This (size) will allow to set the container rootfs size to 120G at creation time. User cannot pass a size less than the Default BaseFS Size.
Now, the supported graphdrivers for this (size) option are `devicemapper` and `btrfs`.

This comment has been minimized.

@thaJeztah

thaJeztah Apr 26, 2016

Member

Same here

@thaJeztah

thaJeztah Apr 26, 2016

Member

Same here

Show outdated Hide outdated man/docker-daemon.8.md
Specifies the size to use when creating the subvolume which is used for
containers. If user uses disk quota for btrfs, docker will check the
minimum free space when a new container is created.

This comment has been minimized.

@thaJeztah

thaJeztah Apr 26, 2016

Member

I'm still a bit confused by this description; does this set the size of the subvolume (per container? for all containers?) or set the minimum amount of space that should be kept available on disk?

@thaJeztah

thaJeztah Apr 26, 2016

Member

I'm still a bit confused by this description; does this set the size of the subvolume (per container? for all containers?) or set the minimum amount of space that should be kept available on disk?

This comment has been minimized.

@zhuguihua

zhuguihua Apr 28, 2016

Contributor

@thaJeztah oh, now btrfs.min_free_space is only to set the minimum size of the subvolume (per container). Then if you specify the size option when creating a container, the value of size must not be smaller than min_free_space. Maybe the variable name is not correct.

@zhuguihua

zhuguihua Apr 28, 2016

Contributor

@thaJeztah oh, now btrfs.min_free_space is only to set the minimum size of the subvolume (per container). Then if you specify the size option when creating a container, the value of size must not be smaller than min_free_space. Maybe the variable name is not correct.

@thaJeztah thaJeztah added this to the 1.12.0 milestone Apr 28, 2016

@zhuguihua

This comment has been minimized.

Show comment
Hide comment
@zhuguihua

zhuguihua Apr 29, 2016

Contributor

@cpuguy83 Your example about quota quirks has already been fixed.
Now there are two things we should pay special attention to:

  1. If quota is reached, we cannot make too many snapshots. If we do that, the balance operation will be too slow.
  2. Snapshots cannot be made into the sub directory, this operation will lead to the wrong quota value.
    AFAIK, the btrfs experts are trying to fix the above two.
Contributor

zhuguihua commented Apr 29, 2016

@cpuguy83 Your example about quota quirks has already been fixed.
Now there are two things we should pay special attention to:

  1. If quota is reached, we cannot make too many snapshots. If we do that, the balance operation will be too slow.
  2. Snapshots cannot be made into the sub directory, this operation will lead to the wrong quota value.
    AFAIK, the btrfs experts are trying to fix the above two.
Show outdated Hide outdated man/docker-create.1.md
@@ -333,6 +333,7 @@ unit, `b` is used. Set LIMIT to `-1` to enable unlimited swap.
$ docker create -it --storage-opt size=120G fedora /bin/bash
This (size) will allow to set the container rootfs size to 120G at creation time. User cannot pass a size less than the Default BaseFS Size.
This option is only available for the `devicemapper` and `btrfs` graphrivers.

This comment has been minimized.

@vdemeester

vdemeester May 4, 2016

Member

Doesn't zfs support this option too (see the docker-daemon man below) ?

@vdemeester

vdemeester May 4, 2016

Member

Doesn't zfs support this option too (see the docker-daemon man below) ?

Show outdated Hide outdated man/docker-run.1.md
@@ -484,7 +484,8 @@ its root filesystem mounted as read only prohibiting any writes.
$ docker run -it --storage-opt size=120G fedora /bin/bash
This (size) will allow to set the container rootfs size to 120G at creation time. User cannot pass a size less than the Default BaseFS Size.
This option is only available for the `devicemapper` and `btrfs` graphrivers.

This comment has been minimized.

@vdemeester

vdemeester May 4, 2016

Member

Same here ?

@vdemeester

vdemeester May 4, 2016

Member

Same here ?

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester May 4, 2016

Member

Appart from 2 nits, docs LGTM for me
/cc @thaJeztah

Member

vdemeester commented May 4, 2016

Appart from 2 nits, docs LGTM for me
/cc @thaJeztah

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit May 5, 2016

Contributor

docs LGTM, pending @vdemeester 's nits

Contributor

SvenDowideit commented May 5, 2016

docs LGTM, pending @vdemeester 's nits

Zhu Guihua
Add disk quota support for btrfs
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

@vdemeester vdemeester merged commit 6dd4c35 into moby:master May 5, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 18297 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 5129 has succeeded
Details
janky Jenkins build Docker-PRs 27126 has succeeded
Details
userns Jenkins build Docker-PRs-userns 9314 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 25604 has succeeded
Details
windowsTP5 Jenkins build Docker-PRs-WoW-TP5 1393 has succeeded
Details
@drusmik

This comment has been minimized.

Show comment
Hide comment
@drusmik

drusmik Jun 30, 2016

Help me please. I can not make it work. I'm using Ubuntu 16.04 on host.

root@drusdocker1604:~# df -hT
------------------------------------------------------
/dev/sdb       btrfs       50G         254M   48G            1% /var/lib/docker

I'm starting dockerd with:
root@drusdocker1604:~# dockerd -s btrfs --storage-opt btrfs.min_space=10G -D

Then I run:

root@drusdocker1604:~# docker run -it --rm --storage-opt size=10G ubuntu:xenial
root@11c351105341:/# df -h
Filesystem      Size  Used Avail Use% Mounted on
/dev/sdb         50G  254M   48G   1% /
tmpfs          1001M     0 1001M   0% /dev
tmpfs          1001M     0 1001M   0% /sys/fs/cgroup
/dev/sdb         50G  254M   48G   1% /etc/hosts
shm              64M     0   64M   0% /dev/shm
root@11c351105341:/# exit

And there is no 10G limit inside my container. What am I doing wrong?
Thank you

drusmik commented Jun 30, 2016

Help me please. I can not make it work. I'm using Ubuntu 16.04 on host.

root@drusdocker1604:~# df -hT
------------------------------------------------------
/dev/sdb       btrfs       50G         254M   48G            1% /var/lib/docker

I'm starting dockerd with:
root@drusdocker1604:~# dockerd -s btrfs --storage-opt btrfs.min_space=10G -D

Then I run:

root@drusdocker1604:~# docker run -it --rm --storage-opt size=10G ubuntu:xenial
root@11c351105341:/# df -h
Filesystem      Size  Used Avail Use% Mounted on
/dev/sdb         50G  254M   48G   1% /
tmpfs          1001M     0 1001M   0% /dev
tmpfs          1001M     0 1001M   0% /sys/fs/cgroup
/dev/sdb         50G  254M   48G   1% /etc/hosts
shm              64M     0   64M   0% /dev/shm
root@11c351105341:/# exit

And there is no 10G limit inside my container. What am I doing wrong?
Thank you

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 30, 2016

Contributor

btrfs.min_space is not the setting you want.
You need --storage-opt size

Contributor

cpuguy83 commented Jun 30, 2016

btrfs.min_space is not the setting you want.
You need --storage-opt size

@drusmik

This comment has been minimized.

Show comment
Hide comment
@drusmik

drusmik Jul 1, 2016

Thank you for your reply. I start container with:
root@drusdocker1604:~# docker run -it --rm --storage-opt size=10G ubuntu:xenial
And btrfs.min_space I pass to docker deamon. But this didn't work for me

drusmik commented Jul 1, 2016

Thank you for your reply. I start container with:
root@drusdocker1604:~# docker run -it --rm --storage-opt size=10G ubuntu:xenial
And btrfs.min_space I pass to docker deamon. But this didn't work for me

@dotNetDR

This comment has been minimized.

Show comment
Hide comment
@dotNetDR

dotNetDR Aug 1, 2016

@drusmik I have the same problem. container df -h output qual Host...

and i test btrfs quota, it working for me.

# i set --storage-opt size=2G, and i run `dd` create files
dd if=/dev/zero of=hello{n}.txt bs={y}M count=1
...

files detail:

[root@77d89d25d0e7 home]# du -sh *
500M    hello.txt
500M    hello2.txt
500M    hello3.txt
100M    hello4.txt
100M    hello5.txt
100M    hello6.txt
73M     hello7.txt
100M    hello8.txt

when i create 2.1G file, system show me dd: failed to open 'hello9.txt': Disk quota exceeded

[root@77d89d25d0e7 home]# dd if=/dev/zero of=hello9.txt bs=100M count=1
dd: failed to open 'hello9.txt': Disk quota exceeded

so, btrfs quota is working, just df -h tell me Avail column not btrfs quota value.


Q: how can i query btrfs quota value in docker container.

dotNetDR commented Aug 1, 2016

@drusmik I have the same problem. container df -h output qual Host...

and i test btrfs quota, it working for me.

# i set --storage-opt size=2G, and i run `dd` create files
dd if=/dev/zero of=hello{n}.txt bs={y}M count=1
...

files detail:

[root@77d89d25d0e7 home]# du -sh *
500M    hello.txt
500M    hello2.txt
500M    hello3.txt
100M    hello4.txt
100M    hello5.txt
100M    hello6.txt
73M     hello7.txt
100M    hello8.txt

when i create 2.1G file, system show me dd: failed to open 'hello9.txt': Disk quota exceeded

[root@77d89d25d0e7 home]# dd if=/dev/zero of=hello9.txt bs=100M count=1
dd: failed to open 'hello9.txt': Disk quota exceeded

so, btrfs quota is working, just df -h tell me Avail column not btrfs quota value.


Q: how can i query btrfs quota value in docker container.

@dotNetDR

This comment has been minimized.

Show comment
Hide comment
@dotNetDR

dotNetDR Aug 12, 2016

i test zfs, quota is working.

# docker run -it --name c01 --storage-opt size=3G centos:7 bash

[root@d1fb6824152b home]# du -sh *
501M    hello1.txt
501M    hello2.txt
501M    hello3.txt
501M    hello4.txt
501M    hello5.txt
501M    hello6.txt
51M     hello7.txt
22M     hello8.txt

[root@d1fb6824152b home]# df -h
Filesystem                                                                            Size  Used Avail Use% Mounted on
zpool-docker/docker/c8e6d7752e3ecd66f857a87cbceb0cf42e70faaf9a59da7fac29f89383c57c95  3.2G  3.2G     0 100% /
tmpfs                                                                                 489M     0  489M   0% /dev
tmpfs                                                                                 489M     0  489M   0% /sys/fs/cgroup
zpool-docker/docker                                                                    36G  896K   36G   1% /etc/hosts
shm                                                                                    64M     0   64M   0% /dev/shm
tmpfs                                                                                 489M     0  489M   0% /proc/kcore
tmpfs                                                                                 489M     0  489M   0% /proc/timer_stats
tmpfs                                                                                 489M     0  489M   0% /proc/sched_debug

[root@d1fb6824152b home]# dd if=/dev/zero of=hello9.txt bs=30M count=1
dd: failed to open 'hello9.txt': Disk quota exceeded

dotNetDR commented Aug 12, 2016

i test zfs, quota is working.

# docker run -it --name c01 --storage-opt size=3G centos:7 bash

[root@d1fb6824152b home]# du -sh *
501M    hello1.txt
501M    hello2.txt
501M    hello3.txt
501M    hello4.txt
501M    hello5.txt
501M    hello6.txt
51M     hello7.txt
22M     hello8.txt

[root@d1fb6824152b home]# df -h
Filesystem                                                                            Size  Used Avail Use% Mounted on
zpool-docker/docker/c8e6d7752e3ecd66f857a87cbceb0cf42e70faaf9a59da7fac29f89383c57c95  3.2G  3.2G     0 100% /
tmpfs                                                                                 489M     0  489M   0% /dev
tmpfs                                                                                 489M     0  489M   0% /sys/fs/cgroup
zpool-docker/docker                                                                    36G  896K   36G   1% /etc/hosts
shm                                                                                    64M     0   64M   0% /dev/shm
tmpfs                                                                                 489M     0  489M   0% /proc/kcore
tmpfs                                                                                 489M     0  489M   0% /proc/timer_stats
tmpfs                                                                                 489M     0  489M   0% /proc/sched_debug

[root@d1fb6824152b home]# dd if=/dev/zero of=hello9.txt bs=30M count=1
dd: failed to open 'hello9.txt': Disk quota exceeded
@bklau

This comment has been minimized.

Show comment
Hide comment
@bklau

bklau Jun 15, 2017

@thaJeztah @cpuguy83 Example : 'docker run --storage-option size=30G ...'
Q: Is the size=30G referring to the CoW writable size allowed per container or the combined total of base image + CoW ?

bklau commented Jun 15, 2017

@thaJeztah @cpuguy83 Example : 'docker run --storage-option size=30G ...'
Q: Is the size=30G referring to the CoW writable size allowed per container or the combined total of base image + CoW ?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 15, 2017

Contributor

@bklau RW layer.

Contributor

cpuguy83 commented Jun 15, 2017

@bklau RW layer.

@bklau

This comment has been minimized.

Show comment
Hide comment
@bklau

bklau Jun 15, 2017

@thaJeztah @cpuguy83 #24771 reads --

"This pull request adds support for --storage-opt size to overlay2.
The semantics of the size parameter are slightly different then those of devmapper/zfs/btrfs.
For the latter, it means total size limit of composite base image + diff.
For overlay, it means size limit for the diff directory."

Isn't above (#24771) seems to hint that --storage-opt to meant different thing for different storage drivers?

bklau commented Jun 15, 2017

@thaJeztah @cpuguy83 #24771 reads --

"This pull request adds support for --storage-opt size to overlay2.
The semantics of the size parameter are slightly different then those of devmapper/zfs/btrfs.
For the latter, it means total size limit of composite base image + diff.
For overlay, it means size limit for the diff directory."

Isn't above (#24771) seems to hint that --storage-opt to meant different thing for different storage drivers?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 15, 2017

Contributor

@bklau Try it?

Contributor

cpuguy83 commented Jun 15, 2017

@bklau Try it?

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