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

Add support for setting storage size on ZFS containers #21946

Merged
merged 3 commits into from
Jun 8, 2016

Conversation

kenXengineering
Copy link
Contributor

- 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 [email protected]

@kenXengineering kenXengineering changed the title Add support for setting storage size on zfs containers Add support for setting storage size on ZFS containers Apr 11, 2016
@calavera
Copy link
Contributor

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

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

@calavera
Copy link
Contributor

This would be easier to implement after merging #21139.

@calavera
Copy link
Contributor

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

@kenXengineering
Copy link
Contributor Author

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

@calavera
Copy link
Contributor

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

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

Choose a reason for hiding this comment

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

Should err be checked here?

Copy link
Member

Choose a reason for hiding this comment

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

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

@cpuguy83
Copy link
Member

cpuguy83 commented May 3, 2016

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

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

@Mic92
Copy link
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 kenXengineering force-pushed the add_disk_quota_to_zfs branch from b639e3c to c43ed33 Compare May 18, 2016 22:04
@kenXengineering
Copy link
Contributor Author

@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 3 commits May 19, 2016 14:49
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 <[email protected]>
@kenXengineering kenXengineering force-pushed the add_disk_quota_to_zfs branch from c43ed33 to 04b4e3e Compare May 19, 2016 18:49
@kenXengineering
Copy link
Contributor Author

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

@calavera
Copy link
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")

@cpuguy83
Copy link
Member

cpuguy83 commented Jun 8, 2016

LGTM

@barrachri
Copy link

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
cherry-pick from: moby#21946
we need this pr to solve the conflicts when cherry-picking
overlay2.

Signed-off-by: Ken Herner <[email protected]>
Signed-off-by: Lei Jitang <[email protected]>
(cherry picked from commit 04b4e3e)
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.

7 participants