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 support for setting storage size on ZFS containers #21946

Merged
merged 3 commits into from Jun 8, 2016

Conversation

Projects
None yet
7 participants
@kenXengineering
Contributor

kenXengineering commented Apr 11, 2016

- What I did
Added support for for the --storage-opt CLI flag to ZFS. Users can specify a container's block device size by passing size (--storage-opt size=2G). Built off of pull request #19367

- How I did it
When creating or cloning a ZFS volume, check storage options for the key size. If it is provided, set the ZFS property quota to the given value on the new ZFS filesystem. If size is not specified, do not set quota.

- How to verify it
Have docker engine setup with ZFS as the storage driver. Pull an image and run it with the --storage-opts size=<size> flag. Run df -h to view attached volumes sizes. The Avail space on the root device should be the size specified.

root@dockerzfs:~# docker run -it --rm --storage-opt size=2G ubuntu bash
root@217c5a71c117:/# df -h
Filesystem                                                                            Size  Used Avail Use% Mounted on
zpool-docker/docker/a9897bab72a60873898191704eb0dfff96aa4efdb0a2676d9e8a74ad20b265a7  2.2G  198M  2.0G   9% /
tmpfs                                                                                1001M     0 1001M   0% /dev
tmpfs                                                                                1001M     0 1001M   0% /sys/fs/cgroup
zpool-docker/docker                                                                    48G  3.2M   48G   1% /etc/hosts
shm                                                                                    64M     0   64M   0% /dev/shm

Note: Due to the way docker utilizes ZFS, the newly created ZFS volume will be from a read-only snapshot of the docker image. Therefore setting the quota on the new volume will make the available space the size of the quota as it is applied to the new volume and not parent volumes.

- A picture of a cute animal
Westies at the Cincinnati Reds Parade

This is my first pull request to Docker, and I welcome all comments and suggestions!

Signed-off-by: Ken Herner kherner@progress.com

@kenXengineering kenXengineering changed the title from Add support for setting storage size on zfs containers to Add support for setting storage size on ZFS containers Apr 11, 2016

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Apr 11, 2016

Contributor

Nice! This was in my list of improvements for ZFS.

Contributor

calavera commented Apr 11, 2016

Nice! This was in my list of improvements for ZFS.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 14, 2016

Member

experimental is green; https://jenkins.dockerproject.org/job/Docker-PRs-experimental/17500/console

22:17:40 Archiving artifacts
22:17:41 Notifying endpoint 'HTTP:https://leeroy.dockerproject.org/notification/jenkins'
22:17:41 Finished: SUCCESS
Member

thaJeztah commented Apr 14, 2016

experimental is green; https://jenkins.dockerproject.org/job/Docker-PRs-experimental/17500/console

22:17:40 Archiving artifacts
22:17:41 Notifying endpoint 'HTTP:https://leeroy.dockerproject.org/notification/jenkins'
22:17:41 Finished: SUCCESS
@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Apr 14, 2016

Contributor

This would be easier to implement after merging #21139.

Contributor

calavera commented Apr 14, 2016

This would be easier to implement after merging #21139.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Apr 14, 2016

Contributor

we should have global quota(at the daemon level) and local quota(what this PR implements).

Contributor

calavera commented Apr 14, 2016

we should have global quota(at the daemon level) and local quota(what this PR implements).

@kenXengineering

This comment has been minimized.

Show comment
Hide comment
@kenXengineering

kenXengineering Apr 29, 2016

Contributor

@calavera @thaJeztah Hello. I haven't seen any movement on the pull request. Do you have any questions or comments?

Contributor

kenXengineering commented Apr 29, 2016

@calavera @thaJeztah Hello. I haven't seen any movement on the pull request. Do you have any questions or comments?

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Apr 30, 2016

Contributor

Design LGTM. I moved it to code-review so others can give feedback.

Contributor

calavera commented Apr 30, 2016

Design LGTM. I moved it to code-review so others can give feedback.

name := d.zfsPath(id)
quota, err := parseStorageOpt(storageOpt)

This comment has been minimized.

@thaJeztah

thaJeztah Apr 30, 2016

Member

Should err be checked here?

@thaJeztah

thaJeztah Apr 30, 2016

Member

Should err be checked here?

This comment has been minimized.

@thaJeztah

thaJeztah Apr 30, 2016

Member

Perhaps return early on error (also below), because there's now a lot of nested if's (but that's just a suggestion)

@thaJeztah

thaJeztah Apr 30, 2016

Member

Perhaps return early on error (also below), because there's now a lot of nested if's (but that's just a suggestion)

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 3, 2016

Contributor

Looks ok to me, but I don't use zfs... apart from @thaJeztah's comment.

Contributor

cpuguy83 commented May 3, 2016

Looks ok to me, but I don't use zfs... apart from @thaJeztah's comment.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 5, 2016

Member

ping @chosenken could you address my comment? Also don't forget to update the relevant documentation and man-pages, but let me know if you need hints for which ones need an update

Member

thaJeztah commented May 5, 2016

ping @chosenken could you address my comment? Also don't forget to update the relevant documentation and man-pages, but let me know if you need hints for which ones need an update

@Mic92

This comment has been minimized.

Show comment
Hide comment
@Mic92

Mic92 May 11, 2016

Contributor

For reference: I also made a pull request for quotas for zfs in the past: #12520

I introduced a global flag for that, which can be run per container not global for the daemon. Maybe you can copy&paste the tests I made.

Contributor

Mic92 commented May 11, 2016

For reference: I also made a pull request for quotas for zfs in the past: #12520

I introduced a global flag for that, which can be run per container not global for the daemon. Maybe you can copy&paste the tests I made.

@kenXengineering

This comment has been minimized.

Show comment
Hide comment
@kenXengineering

kenXengineering May 18, 2016

Contributor

@thaJeztah I've updated that section to return on error, thanks for the review! As for documentation, I think the details added in this commit b16decf#diff-505c72218d90da970c16fdbf0b4f613c should cover it. I'm using the same option from #19367. I'm also going to add tests for this, so I will update when I get them added.

Contributor

kenXengineering commented May 18, 2016

@thaJeztah I've updated that section to return on error, thanks for the review! As for documentation, I think the details added in this commit b16decf#diff-505c72218d90da970c16fdbf0b4f613c should cover it. I'm using the same option from #19367. I'm also going to add tests for this, so I will update when I get them added.

kenXengineering and others added some commits Apr 11, 2016

Add support for setting storage size on zfs containers
Now supports setting a containers storage size when using zfs as the
storage engine.  By passing in `--storage-opt size=<size>`, the created
container's storage size will be limited to the given size.  Note that
the way zfs works, the given specified storage size will be given in
addition to the base container size.

Example:

The node image reports a size of `671M` from `df -h` when started.
Setting `--storage-opt size=2G` will result in a drive the size of
`671M` + `2G`, `2.7G` in total.  Available space will be `2.0G`.

The storage size is achieved by setting the zfs option `quota` to the
given size on the zfs volume.

Signed-off-by: Ken Herner <kherner@progress.com>
Ken Herner
Add error check after parseStorageOpt
Signed-off-by: Ken Herner <kherner@progress.com>
Ken Herner
Add test to ZFS for disk quota
Signed-off-by: Ken Herner <kherner@progress.com>
@kenXengineering

This comment has been minimized.

Show comment
Hide comment
@kenXengineering

kenXengineering May 20, 2016

Contributor

I've added a test case based off of @Mic92 pull request #12520.

Contributor

kenXengineering commented May 20, 2016

I've added a test case based off of @Mic92 pull request #12520.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera May 20, 2016

Contributor

LGTM.

It looks like there is a flaky test:

19:26:02 FAIL: docker_cli_run_unix_test.go:947: DockerSuite.TestRunSeccompDefaultProfile
19:26:02 
19:26:02 docker_cli_run_unix_test.go:1045:
19:26:02     c.Assert(err, checker.IsNil)
19:26:02 ... value *errors.errorString = &errors.errorString{s:"expected No such file or directory, got: /go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/../binary-client/docker: Error response from daemon: rpc error: code = 2 desc = \"containerd: container did not start before the specified timeout\".\n"} ("expected No such file or directory, got: /go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/../binary-client/docker: Error response from daemon: rpc error: code = 2 desc = \"containerd: container did not start before the specified timeout\".\n")
Contributor

calavera commented May 20, 2016

LGTM.

It looks like there is a flaky test:

19:26:02 FAIL: docker_cli_run_unix_test.go:947: DockerSuite.TestRunSeccompDefaultProfile
19:26:02 
19:26:02 docker_cli_run_unix_test.go:1045:
19:26:02     c.Assert(err, checker.IsNil)
19:26:02 ... value *errors.errorString = &errors.errorString{s:"expected No such file or directory, got: /go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/../binary-client/docker: Error response from daemon: rpc error: code = 2 desc = \"containerd: container did not start before the specified timeout\".\n"} ("expected No such file or directory, got: /go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/../binary-client/docker: Error response from daemon: rpc error: code = 2 desc = \"containerd: container did not start before the specified timeout\".\n")
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 8, 2016

Contributor

LGTM

Contributor

cpuguy83 commented Jun 8, 2016

LGTM

@cpuguy83 cpuguy83 merged commit d85491f into moby:master Jun 8, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 18704 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 5561 has succeeded
Details
janky Jenkins build Docker-PRs 27887 has succeeded
Details
userns Jenkins build Docker-PRs-userns 9711 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 26072 has succeeded
Details
windowsTP5 Jenkins build Docker-PRs-WoW-TP5 1833 has succeeded
Details

@thaJeztah thaJeztah added this to the 1.12.0 milestone Jun 8, 2016

@barrachri

This comment has been minimized.

Show comment
Hide comment
@barrachri

barrachri Sep 20, 2017

Hello, does this work only with containers?

Because if I follow the docs from here: https://docs.docker.com/engine/userguide/storagedriver/zfs-driver/#configure-docker-with-the-zfs-storage-driver

I just get this error #33847

barrachri commented Sep 20, 2017

Hello, does this work only with containers?

Because if I follow the docs from here: https://docs.docker.com/engine/userguide/storagedriver/zfs-driver/#configure-docker-with-the-zfs-storage-driver

I just get this error #33847

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Add test to ZFS for disk quota
cherry-pick from: moby#21946
we need this pr to solve the conflicts when cherry-picking
overlay2.

Signed-off-by: Ken Herner <kherner@progress.com>
Signed-off-by: Lei Jitang <leijitang@huawei.com>
(cherry picked from commit 04b4e3e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment