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
Xfs quota for overlay #24807
Xfs quota for overlay #24807
Conversation
Some timing we have... nice work :-)
That's a smart move, but it has one problem. xfs inode is 64bit and project id is 32bit,
I did this for you :-)
it's not. not that it matters so much if only docker uses this specific project id. But most importantly, in my PR I wrote: So I may have been wrong with this statement, but are you sure that all the cases |
@amir73il Thanks for your comments :) |
Correction: xfs 64bit is an opt-in feature only available when the mount option inode64 is used. |
792042b
to
37c9774
Compare
37c9774
to
6f7b1d0
Compare
daemon/graphdriver/quota.go
Outdated
} | ||
if projectID <= uint32(fsx.fsx_projid) { | ||
projectID = uint32(fsx.fsx_projid) + 1 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you want to set this projid in the q.quotas[] map? if you don't, users won't be able to call getQuota() for those directories later on
2e88ece
to
0cc50ec
Compare
0e123e2
to
738acaa
Compare
@thaJeztah I get projectquota.go from #24771 and apply it (for rebase) |
@albamc #24771 was merged, so you can rebase on master also ping @crosbymichael @dmcgowan for review |
738acaa
to
e21a431
Compare
@thaJeztah I did rebase on master. |
@@ -420,7 +467,13 @@ func (d *Driver) ApplyDiff(id string, parent string, diff archive.Reader) (size | |||
} | |||
}() | |||
|
|||
if err = copyDir(parentRootDir, tmpRootDir, copyHardlink); err != nil { | |||
// if quota is used, hardlinks between different volumes are not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe this condition is too harsh.
anyone using overlay over xfs with pquota (even without any intention to use pquota with docker) will have much worse performance for starting containers.
Probably the right thing to do is to use d.quotaCtl.GetQuota() to find out if either the source or target directory have a pquota set. Otherwise hardlinks should be used.
Also, I did not look at the driver code closely, but I have the feeling that --size argument to create/run applies to both lower and upper layer directories. Is that correct? If this is the case, then avoiding to set project quota on the lower dir, should enable applying diff with hardlinks.
Truth is that if project quota is to be applied to both lower and upper dir, then they should be using the same project id and that is not what quotaCtl.SetQuota() does.
The semantics of setting quota only to upper dir is same as overlay2 (limited amount of modifications to base fs) while the semantics os setting quota on both lower and upper is more like devmapper (limited amount of space for entire composed image)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should limit this to just the upper layer as is the case with overlay2. We can set the quota on the upper
directory and not worry about the potential for project to apply to a root directory. Any directory that has ApplyDiff
called on it will not be used as a container layer and the root
directory will never be writable from a container.
Looks good besides my comment on hardlinks in ApplyDiff(). IMO, the exact use cases and exact consequences should be clarified before merge. And as I wrote, there is a possibility that this drawback can be completely avoided. |
I agree. Of course, hardlink problem must be solved before merge.
if 2 solutions doesn't work, at least check hardlink source/destinations and copy files only different quota boundries.
|
ad629c3
to
f89c64d
Compare
f89c64d
to
e21a431
Compare
@amir73il |
@albamc Let me know if you want some help with this. |
@cpuguy83 |
It's been a week or so since I've tried it, but IIRC this worked for me. |
@cpuguy83 |
@albamc It's definitely not in perfect condition, but somewhere to start. |
@albamc Any news? |
@cpuguy83
|
@albamc the reason for the error is written in my original PR #24771 under section Why only overlay2?: |
ping |
ping @albamc do you have time to work on this, or should this be closed? The command line reference docs should now be moved to the https://github.com/docker/cli repository, thus split from this PR |
I'll go ahead and forward that 👼 to @imkin who did a fine job on refactoring the storage options code in #32977 and to @cpuguy83 who has already offered his help on this PR and claims to have been able to solve the hardlink copying problem. My opinion remains that this PR is as good as overlay2.size in it's current form, give or take the improvements of #32977 and that IMO copying instead of hardlinks in case of commiting a limited quota container is simply good enough for overlay driver. As always I'm available for review. Cheers |
@thaJeztah |
@dmcgowan I think this thread has gone into an unnecessary spin, partly on my account, because I did not study how overlay driver works before replying on this thread. When you commented on December that quota should be applied only to upper to "match the behavior of the overlay2 quota" I did not read enough into it, but looking back, I do think that @albamc current work is a match to the overlay2 quota behavior, unless I am missing something. IIUC, overlay driver creates 2 types of dirs: images (ro) and containers (rw). is that correct? Now with ApplyDiff(), the solution that @albamc has implemented is really the only solution I can thing of, because as discussed on PR #32977, quota should NOT apply to images, and the only way to "release" the changed files out of the project quota account is by copying them to the new root dir. IMO, this PR should be rebased, an additional commit to match PR #32977 should be applied and it should be ready to go. @dmcgowan do you agree with my analysis? If not, please explain where am I wrong. w.r.t. @cpuguy83 comments about re-factoring, I do agree with them, but I suggest that since they apply to both overlay and overlay2, that this re-factoring be postponed to after merging this PR and perhaps done by someone who knows more about the design of the graphdriver. |
@amir73il @thaJeztah So if I got this correct: If the storage driver is "overlay2" and the XFS is used, then quota will be automatically ENABLED?. Is this correct?. No other daemon start options needed right? Pls clarify. Thx. |
@amir73il sorry I didn't respond sooner. I was coming up with a response but wasn't quite sure. Looking at how the quota is not applied on |
@bklau this PR is not related to |
@dmcgowan Hi Derek, how is the "quota" enabled?. Any special start option flags |
@thaJeztah |
Signed-off-by: albam.c <albam.c@navercorp.com>
0036c77
to
23977d6
Compare
ping @albamc what's the status here ? |
@AkihiroSuda let's close it for now. |
- What I did
Apply xfs project quota for overlay storage driver.
Basically it's same work for #24771 Xfs quota PR but I applied to overlay instead of overlay2
I planed to improve this PR and apply overlay2 too but better PR (#24771) is already summited...
- How I did it
Set xfs project quota to container's overlay storage root directory.
Use projectquota.go in #24771 and apply it to overlay driver.
The --storage-opt size=#(m/g) options to set a limit for containers.
- Known issues
This quota only applied to container's upper layer and file changes which made by container itself.
XFS quota doesn't allow hardlink across quota boundries. so basic ApplyDiff() hardlink operation is changed.
XFS project id is not deleted when quota is deleted by overlay driver (ex. container rm).
And there's no way to figure out current project id is used in another place.
So, If you want to create unique project id, set a project id to home directory of overlay for it. (use it to minimum project id for quota)
- How to verify it
- Description for the changelog
apply xfs project quota for overlay over xfs
- A picture of a cute animal (not mandatory but encouraged)
Signed-off-by: Jonghyun Lee albam.c@navercorp.com