Add disk quota support for btrfs #19651

Merged
merged 1 commit into from May 5, 2016

Projects

None yet
@zhuguihua
Contributor
zhuguihua commented Jan 25, 2016 edited

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
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

@zhuguihua
Contributor

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

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jan 25, 2016
@thaJeztah
Member

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

@icecrime icecrime and 3 others commented on an outdated diff Feb 1, 2016
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)")
@icecrime
icecrime Feb 1, 2016 Member

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

@thaJeztah
thaJeztah Feb 1, 2016 Member

Was thinking the same, see #19651 (comment)

@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?

@cpuguy83
cpuguy83 Feb 9, 2016 Contributor

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

@jhowardmsft
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)
@thaJeztah
Member

ping @zhuguihua were you working on this?

@zhuguihua
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.

@thaJeztah
Member

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

@zhuguihua
Contributor

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

@calavera
Contributor

LGTM.

The new option needs to be properly documented.

@zhuguihua
Contributor

@calavera Thanks your review. Will add docs later.

@zhuguihua
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?

@cpuguy83
Contributor

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

@zhuguihua
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?

@icecrime
Member

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
Contributor

@icecrime Fixed, also passed CI. Thanks.

@cpuguy83 cpuguy83 commented on an outdated diff Feb 29, 2016
daemon/graphdriver/btrfs/btrfs.go
"github.com/opencontainers/runc/libcontainer/label"
)
+var (
+ defaultDiskQuota uint64 = 10 * 1024 * 1024 * 1024
@cpuguy83
cpuguy83 Feb 29, 2016 Contributor

nit, this is minDiskQuota not defaultDiskQuota

@cpuguy83
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.

@thaJeztah
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

@thaJeztah
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?

@thaJeztah
Member

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

@cpuguy83
Contributor

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

@thaJeztah
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?

@zhuguihua
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

@calavera
Contributor

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

SGTM

@thaJeztah
Member

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

@thaJeztah
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.
@zhuguihua
Contributor

@thaJeztah Updated. But the ci failure maybe not related to this PR. Thanks.

@thaJeztah
Member

Should this option be called min_free_space, to match #20786 ?

@zhuguihua
Contributor

@thaJeztah Changed. Thanks for your suggestion.

@calavera calavera commented on the diff Mar 31, 2016
daemon/graphdriver/btrfs/btrfs.go
@@ -285,6 +428,51 @@ func (d *Driver) Create(id, parent, mountLabel string, storageOpt map[string]str
return label.Relabel(path.Join(subvolumes, id), mountLabel, false)
}
+// Parse btrfs storage options
+func (d *Driver) parseStorageOpt(storageOpt map[string]string, driver *Driver) error {
+ // Read size to change the subvolume disk quota per container
+ for key, val := range storageOpt {
+ key := strings.ToLower(key)
+ switch key {
+ case "size":
@calavera
calavera Mar 31, 2016 Contributor

is this case correct?

@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

@thaJeztah
thaJeztah Apr 1, 2016 Member

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

@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
Member

ping @calavera @cpuguy83 PTAL

@tonistiigi tonistiigi and 1 other commented on an outdated diff Apr 14, 2016
daemon/graphdriver/btrfs/btrfs.go
"github.com/opencontainers/runc/libcontainer/label"
)
func init() {
graphdriver.Register("btrfs", Init)
}
+var (
+ defaultMinFreeSpace uint64 = 10 * 1024 * 1024 * 1024
@tonistiigi
tonistiigi Apr 14, 2016 Contributor

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

@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.

@tonistiigi
tonistiigi Apr 15, 2016 Contributor

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

@cpuguy83 cpuguy83 and 1 other commented on an outdated diff Apr 15, 2016
daemon/graphdriver/btrfs/btrfs.go
}
return graphdriver.NewNaiveDiffDriver(driver, uidMaps, gidMaps), nil
}
+func parseOptions(opt []string) (btrfsOptions, error) {
+ var options btrfsOptions
+ options.quotaEnabled = false
@cpuguy83
cpuguy83 Apr 15, 2016 Contributor

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

@zhuguihua
zhuguihua Apr 19, 2016 Contributor

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

@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

@zhuguihua
zhuguihua Apr 19, 2016 Contributor

Got it, thanks.

@cpuguy83 cpuguy83 commented on an outdated diff Apr 15, 2016
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 {
@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
Contributor

Updated, and passed all checks. Welcome more comments.

@cpuguy83
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.

@tonistiigi
Contributor

LGTM

@thaJeztah thaJeztah commented on an outdated diff Apr 26, 2016
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`.
@thaJeztah
thaJeztah Apr 26, 2016 Member

perhaps;

This option is only available for the `devicemapper` and `btrfs` graphrivers.
@thaJeztah thaJeztah commented on an outdated diff Apr 26, 2016
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`.
@thaJeztah
thaJeztah Apr 26, 2016 Member

Same here

@thaJeztah thaJeztah and 1 other commented on an outdated diff Apr 26, 2016
man/docker-daemon.8.md
@@ -513,6 +513,16 @@ By default docker will pick up the zfs filesystem where docker graph
Example use: `docker daemon -s zfs --storage-opt zfs.fsname=zroot/docker`
+## Btrfs options
+
+#### btrfs.min_free_space
+
+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.
@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?

@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
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.
@vdemeester vdemeester commented on an outdated diff May 4, 2016
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.
@vdemeester
vdemeester May 4, 2016 Member

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

@vdemeester vdemeester commented on an outdated diff May 4, 2016
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.
@vdemeester
vdemeester May 4, 2016 Member

Same here ?

@vdemeester
Member

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

@SvenDowideit
Collaborator

docs LGTM, pending @vdemeester 's nits

Zhu Guihua Add disk quota support for btrfs
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
401c8d1
@vdemeester vdemeester merged commit 6dd4c35 into docker: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
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
Contributor

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

@drusmik
drusmik commented Jul 1, 2016 edited

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
dotNetDR commented Aug 1, 2016 edited

@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
dotNetDR commented Aug 12, 2016 edited

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment