Add vfs quota support #35231

sargun · 2017-10-17T22:57:00Z

- What I did
Added project quota support to the VFS graphdriver, and tests to ensure they work.

It relies on #35177, which adds the quota detection mechanism, and a form error to determine whether to short-circuit quota-related code (or not).

It addresses this issue: #34702

- How I did it
I reused the project quota code that came from overlay2. On initialization time, the VFS driver (on Linux) detects if quota is supported. If quota is supported, then it enables the MaxSize option, so new containers can be created with an assigned project quota.

- How to verify it
There are tests as part of the changeset. If you run the tests when your TMPDIR environment variable points to an XFS mount mounted with prjquota option, the quota check will run automatically, otherwise, it'll be skipped.

- Description for the changelog
Add project quota support to VFS graphdriver

AkihiroSuda · 2017-10-18T05:23:23Z

what motivates you to use vfs?

sargun · 2017-10-18T15:16:51Z

@AkihiroSuda "It just works" (mostly). My containers run from 2 minutes to 2 months. The median runtime of my containers is 4-5 minutes. Setting up VFS, with the improvements that come from #34670 has an unoptimized mean time of ~45s for copying ubuntu:latest. We can optimize that down to <10s.

If I can optimize these times, it prevents any potential hiccups from occurring at runtime, versus startup time.

Never having to worry about overlayfs, or such, there's some appeal there.

sargun · 2017-10-25T01:44:49Z

@dmcgowan We discussed this change earlier.

thaJeztah · 2017-10-30T17:19:22Z

daemon/graphdriver/graphtest/graphtest_unix.go

@@ -12,6 +12,8 @@ import (
 	"testing"
 	"unsafe"

+	"github.com/docker/docker/daemon/graphdriver/quota"
+


nit: can you remove this blank line?

thaJeztah · 2017-10-30T17:20:48Z

/cc @kolyshkin as well

sargun · 2017-10-30T20:20:28Z

Rebased.

sargun · 2017-10-31T22:10:28Z

This should be ready to go in.

kolyshkin · 2017-11-01T02:16:11Z

daemon/graphdriver/graphtest/graphtest_unix.go

 	driver := GetDriver(t, drivername)
 	defer PutDriver(t)

 	createBase(t, driver, "Base")
 	createOpts := &graphdriver.CreateOpts{}
 	createOpts.StorageOpt = make(map[string]string, 1)
 	createOpts.StorageOpt["size"] = "50M"
-	if err := driver.Create("zfsTest", "Base", createOpts); err != nil {
+	if err := driver.CreateReadWrite("zfsTest", "Base", createOpts); err == quota.ErrQuotaNotSupported && !required {


Perhaps it makes sense to rename "zfsTest" to drivername + "Test" as it's no longer zfs-specific. Or just "quotaTest"...

kolyshkin · 2017-11-01T02:18:49Z

daemon/graphdriver/graphtest/graphtest_unix.go

+	}
+
+	// Try to write a file bigger than quota, and ensure it does not work
+	if pathError, ok := err.(*os.PathError); ok && pathError.Err != unix.EDQUOT && pathError.Err != unix.ENOSPC {


Looks like you're missing a line here? Something like

+ err = writeRandomFile(path.Join(mountPath.Path(), "file"), quota*2)

Since you have already exhausted half the quota by doing a write above, you might either go with a smaller value than quota*2, or remove the file written above.

kolyshkin · 2017-11-01T02:23:43Z

daemon/graphdriver/graphtest/graphtest_unix.go

+
+	// Try to write a file bigger than quota, and ensure it does not work
+	if pathError, ok := err.(*os.PathError); ok && pathError.Err != unix.EDQUOT && pathError.Err != unix.ENOSPC {
+		t.Fatalf("expect write() to fail with %v or %v, got %v", unix.EDQUOT, unix.ENOSPC, err)


Perhaps you want to print pathError.Err here, not err, as this is what you use in comparisons?

kolyshkin · 2017-11-01T02:40:30Z

daemon/graphdriver/vfs/driver.go

-	home       string
-	idMappings *idtools.IDMappings
+	driverQuota
+	quotaSupported bool


I think this can be done in a simple manner, without an extra bool field. The idea is to add isQuotaSupported() method. In quota_linux.go, it is just return driver.quotaCtl != nil, in quota_unsupported.go, return false straight away. To my mind, it will be cleaner/simpler.

sargun · 2017-11-01T18:08:32Z

Updated based on review.

cpuguy83 · 2017-11-01T20:24:11Z

daemon/graphdriver/vfs/quota_unsupported.go

+}
+
+func (d *Driver) setupQuota(dir string, size uint64) error {
+	panic("Platform does not support quota; this should never be called")


Panic seems a little harsh here. Can we just return a not supported error?

This should never be called. If this is called, it means something has gone wrong, or the code was changed in an unexpected way.

justincormack · 2017-11-03T14:43:21Z

ping @cpuguy83

justincormack · 2017-11-03T14:44:50Z

daemon/graphdriver/graphtest/graphtest_unix.go

+	}
+	defer os.Remove(path.Join(mountPath.Path(), "smallfile"))
+
+	// Try to write a file bigger than quota, given that we've already written a file larger than quota, and ensure it does not work


this comment isn't right, we have written a file of half quota at this point?

kolyshkin

LGTM

cpuguy83 · 2017-11-05T12:48:45Z

daemon/graphdriver/quota/errors.go

+import "errors"
+
+// ErrQuotaNotSupported indicates if were found the FS didn't have projects quotas available
+var ErrQuotaNotSupported = errors.New("Filesystem does not support, or has not enabled quotas")


Can we make sure this error implements one of api/errdefs?

Which one do you think it should be?

ErrNotImplemented?

Or ErrUnavailable

NotImplemented sounds good.

This patch adds the capability for the VFS graphdriver to use XFS project quotas. It reuses the existing quota management code that was created by overlay2 on XFS. It doesn't rely on a filesystem whitelist, but instead the quota-capability detection code. Signed-off-by: Sargun Dhillon <sargun@sargun.me>

sargun · 2017-11-07T16:06:08Z

@cpuguy83 Anything else?

cpuguy83

LGTM

GordonTheTurtle added the status/0-triage label Oct 17, 2017

fntlnz added the rebuild/* label Oct 17, 2017

GordonTheTurtle removed the rebuild/* label Oct 17, 2017

sargun force-pushed the add-vfs-quota-support branch from 8a32b5e to 5b2a9d2 Compare October 17, 2017 23:27

yongtang added status/2-code-review and removed status/0-triage labels Oct 25, 2017

sargun mentioned this pull request Oct 25, 2017

Add tests to project quotas and detection mechanism #35177

Merged

thaJeztah reviewed Oct 30, 2017

View reviewed changes

sargun force-pushed the add-vfs-quota-support branch from 5b2a9d2 to d348809 Compare October 30, 2017 20:20

sargun force-pushed the add-vfs-quota-support branch 3 times, most recently from b37e83e to e85698e Compare October 31, 2017 17:28

GordonTheTurtle assigned vdemeester Nov 1, 2017

kolyshkin requested changes Nov 1, 2017

View reviewed changes

sargun force-pushed the add-vfs-quota-support branch from e85698e to 1dc0690 Compare November 1, 2017 15:18

cpuguy83 requested changes Nov 1, 2017

View reviewed changes

sargun force-pushed the add-vfs-quota-support branch from 1dc0690 to 2cbdc8c Compare November 1, 2017 20:42

justincormack reviewed Nov 3, 2017

View reviewed changes

kolyshkin approved these changes Nov 3, 2017

View reviewed changes

sargun force-pushed the add-vfs-quota-support branch from 2cbdc8c to ca5e654 Compare November 3, 2017 23:06

cpuguy83 requested changes Nov 5, 2017

View reviewed changes

sargun force-pushed the add-vfs-quota-support branch from ca5e654 to 1fa28b0 Compare November 6, 2017 23:31

sargun force-pushed the add-vfs-quota-support branch from 1fa28b0 to 5eb1c11 Compare November 6, 2017 23:36

sargun force-pushed the add-vfs-quota-support branch from 5eb1c11 to 7a1618c Compare November 6, 2017 23:54

cpuguy83 approved these changes Nov 10, 2017

View reviewed changes

justincormack approved these changes Nov 14, 2017

View reviewed changes

justincormack merged commit 0defc69 into moby:master Nov 14, 2017

sargun deleted the add-vfs-quota-support branch November 14, 2017 15:39

thaJeztah mentioned this pull request Nov 16, 2017

skip xfs quota tests when running in user namespace #35526

Merged

mdlinville mentioned this pull request Dec 18, 2017

Review vfs quota support for docs impact docker/docs#5556

Closed

This was referenced Jan 18, 2019

Do not support set the size option in the daemon.json for vfs quota #38592

Closed

add vfs quota for daemon storage-opts #38593

Merged

thaJeztah mentioned this pull request Sep 18, 2023

Add Quota Support to VFS Driver #34702

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vfs quota support #35231

Add vfs quota support #35231

sargun commented Oct 17, 2017 •

edited

Loading

AkihiroSuda commented Oct 18, 2017

sargun commented Oct 18, 2017 •

edited

Loading

sargun commented Oct 25, 2017

thaJeztah Oct 30, 2017

thaJeztah commented Oct 30, 2017

sargun commented Oct 30, 2017

sargun commented Oct 31, 2017

kolyshkin Nov 1, 2017

kolyshkin Nov 1, 2017

kolyshkin Nov 1, 2017

kolyshkin Nov 1, 2017

sargun commented Nov 1, 2017

cpuguy83 Nov 1, 2017

sargun Nov 1, 2017

justincormack commented Nov 3, 2017

justincormack Nov 3, 2017

kolyshkin left a comment

cpuguy83 Nov 5, 2017

sargun Nov 6, 2017

sargun Nov 6, 2017

sargun Nov 6, 2017

cpuguy83 Nov 6, 2017

sargun commented Nov 7, 2017

cpuguy83 left a comment

Add vfs quota support #35231

Add vfs quota support #35231

Conversation

sargun commented Oct 17, 2017 • edited Loading

AkihiroSuda commented Oct 18, 2017

sargun commented Oct 18, 2017 • edited Loading

sargun commented Oct 25, 2017

Choose a reason for hiding this comment

thaJeztah commented Oct 30, 2017

sargun commented Oct 30, 2017

sargun commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sargun commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincormack commented Nov 3, 2017

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sargun commented Nov 7, 2017

cpuguy83 left a comment

Choose a reason for hiding this comment

sargun commented Oct 17, 2017 •

edited

Loading

sargun commented Oct 18, 2017 •

edited

Loading