Skip to content

Add size option to volumes on linux/unix via xfs pquota#41330

Merged
thaJeztah merged 6 commits intomoby:masterfrom
BtbN:vol-pquota
Oct 7, 2020
Merged

Add size option to volumes on linux/unix via xfs pquota#41330
thaJeztah merged 6 commits intomoby:masterfrom
BtbN:vol-pquota

Conversation

@BtbN
Copy link
Copy Markdown
Contributor

@BtbN BtbN commented Aug 9, 2020

- What I did

This PR uses the existing quota module to add an option to apply xfs project quotas on volumes.

- How I did it

The first part modifies the existing projectquota code to be ready for handling multiple directories.
It needs a way to avoid projectID re-use between the two, since otherwise a volume and a container could end up sharing a quota.
For this, a global singleton is introduced that manages the next free projectID in a thread-safe manner.
Since the local volume driver uses a subdir of the base dir for the actual storage (volumename/_data), the projectID scanning code in findNextProjectID is extended to take one level of sub directories into account, provided no projectID is set on the upper level dir already.

The second part then adds support for projectquota to the volume/local driver.
Some modifications were needed in order to allow for options being present without forcing a mount.
In turn, the mandatoryOpts now refer to a list of other options, which are all required to be set if the respective option itself is set, thus allowing size to be set without requiring all the other ones, while still maintaining the previous constraints.
Instead of checking if any options are set in order to determine if a mount call is needed, local.go now calls a new helper function specifically for that purpose, which is then implemented in the driver based on the set options.

Finally, the actual quota support is added, by adding a new size option, which when set is applied via the projectquota module on volume mount, utilizing the new postMount function which is called unconditionally after (potentially) mounting the volume.

- How to verify it

docker volume create -o size=1G testvol
docker run -ti --rm -v testvol:/home ubuntu:latest /bin/bash
# df -h /
# df -h /home

This obviously needs dockerd to run with /var/lib/docker being on xfs with pquota support enabled.

- Description for the changelog

Added support for volume size quotas via xfs project quotas.

- TODO

  • Add documentation
  • Potentially add an option to manually specify a projectID for both volumes and overlay2, to give users an option to intentionally share a quota across multiple containers/volumes. Though that's probably for a future PR.

@BtbN BtbN force-pushed the vol-pquota branch 3 times, most recently from 950a919 to 3c40e03 Compare August 9, 2020 11:07
@BtbN
Copy link
Copy Markdown
Contributor Author

BtbN commented Aug 9, 2020

This failure of docker-py in ContainerTest.test_remove looks unrelated? At least I don't see how this PR would interfere with that at all.

@AkihiroSuda
Copy link
Copy Markdown
Member

Please use real name for signing

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Also, could you add integration tests

@BtbN BtbN force-pushed the vol-pquota branch 2 times, most recently from a4d173d to 15c2250 Compare August 9, 2020 20:22
@AkihiroSuda
Copy link
Copy Markdown
Member

@thaJeztah @cpuguy83

Do you think this is safe to bring in 20.XX? Or better to defer until 21.XX?

@BtbN
Copy link
Copy Markdown
Contributor Author

BtbN commented Aug 10, 2020

Also, could you add integration tests

I'll look into that, though it's a bit complicated given that it relies on xfs and specific mount options.

Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

Discussing: looking at #33903; it may be good to put in the documentation that it's implemented as a quota.

Wondering if the option should be (e.g.) max-size instead of size to make clearer that it's restricting the maximum size that can be consumed, and doesn't reserve the specified size (or guarantee the size)

WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not very familiar these paths.
How big can this be?

I have a feeling we should read the dir with a fixed buffer size.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not too crazy, given that both overlay2 and local have another layer of subdirs below their top level dir, it's never going to actually reach a users FS to enumerate.
Also, this functions only runs once at daemon startup to build the initial state, and never at runtime.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ping @cpuguy83 ptal ^

@thaJeztah
Copy link
Copy Markdown
Member

Discussing size vs max-size with @cpuguy83 @kolyshkin and given that some drivers already use size, it's probably ok to keep as size 👍

@BtbN
Copy link
Copy Markdown
Contributor Author

BtbN commented Aug 20, 2020

I can absolutely see the argument that size might be a bit too generic of a name.
My idea would be "quota-size" or something along those lines. Reason I stuck to just "size" was to keep it in line with the already existing size option on the overlay2 graph driver.

@BtbN
Copy link
Copy Markdown
Contributor Author

BtbN commented Sep 6, 2020

Something that needs changing still? I'm not sure how exactly to adjust the initial scan, but I really don't think it's a performance issue, even if it scans thousands of dirs, it's still fast and only done once.

@thaJeztah
Copy link
Copy Markdown
Member

Reason I stuck to just "size" was to keep it in line with the already existing size option on the overlay2 graph driver.

Yes, I think "size" is ok to keep

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

Some questions on the implementation. I don't understand the changes to mandatory opts handlings.

Other than that, I tested this both on an unsupported config and a supported one and this all looks good.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe pass quotaEnabled as a bool rather than the whole volume

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea here is to keep the function signature more generic, so that potential future options which need info from the volume don't have to add another one.
Passing in quotaEnabled would also just mean the check for v.quotaCtl == nil would move one function up, to then be passed in as bool. I think I prefer the current approach.

Though it does cause a bit of code-duplication between this and the actual quota-application in setOpts.
So maybe the check here can be removed entirely, and setOpts checks for quota support itself and bails if it's missing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactored this to get rid of the issue entirely and avoid the logic duplication.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you explain the reason behind the changes around this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Prior to this, as soon as any option on the local_unix driver was set, all options in this map(or rather, set) had to be provided.
This is not desirable for the size option, since it can very well stand on its own.
So the new logic changes this to a map of string-arrays, where the array elements indicate which other options are required if the option which is they key is set.
That way all prior mandatory constraints are still maintained, while size can stand on its own without requiring any other options to be set.

@thaJeztah
Copy link
Copy Markdown
Member

@cpuguy83 ptal

@BtbN BtbN force-pushed the vol-pquota branch 2 times, most recently from cc1568b to 4fc1cac Compare September 24, 2020 20:58
@BtbN
Copy link
Copy Markdown
Contributor Author

BtbN commented Sep 24, 2020

I slightly refactored the validateOpts part to remove the logic-duplication it had.
The failing tests all look unrelated and seem to be caused by network issues on one test runner.

BtbN added 6 commits October 5, 2020 13:28
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
Signed-off-by: Timo Rothenpieler <timo@rothenpieler.org>
@BtbN BtbN requested a review from cpuguy83 October 5, 2020 13:30
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@max-wittig
Copy link
Copy Markdown

Is there an option to set a global size limit per container? Like have some sort of setting inside daemon.json?

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.

6 participants