Recursively set project ID for quota driver #2264
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mheon The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mheon
force-pushed
the
fix_podman_25368
branch
from
1b3bb5d to
b112e7b
Compare
| if err := setProjectID(filepath.Join(targetPath, dent.Name()), projectID); err != nil { | ||
| return fmt.Errorf("recursively setting project ID on subdirectories of %s: %w", targetPath, err) | ||
| } |
There was a problem hiding this comment.
I am not sure if it matters for our use case but if we walk a dir and then try to open the path it is possible that it has been removed between the listing and the open call.
As such I think this API needs to ignore ErrNotExist (though as this uses cgo I have not checked if matching ErrNotExist will actually match the right error in that case, I think it does but better double check)
mheon
force-pushed
the
fix_podman_25368
branch
from
b112e7b to
1d83758
Compare
|
Once this merges, I'm going to vendor c/s main into Podman and add testing for quotas to CI there. |
There was a problem hiding this comment.
I don’t really understand the issue or how this is supposed to help.
As a structural concern, what if the existing files in that directory are already larger than the quota?
To be more specific about the situation, this code is called:
- in
drivers/overlay.Driver.create; in that case the target does not exist (or is explicitly removed before proceeding) - in
Runtime.newVolume, which AFAICS does not proceed if the volume already exists.
So, how do we get into a situation where there are pre-existing files/directories? At a high level, that seems to be something that shouldn’t happen (or I misunderstand what “create a volume” means, quite possible — I didn’t read the whole of the Podman issue).
As a guess, would reordering Runtime.newVolume to set the quota first before creating _data help?
| @@ -370,6 +371,20 @@ func setProjectID(targetPath string, projectID uint32) error { | |||
| return fmt.Errorf("failed to set projid for %s: %w", targetPath, errno) | |||
| } | |||
|
|
|||
| // While we are setting the PROJINHERIT flag, it only applies to files | |||
| // and directories created after the fact. | |||
There was a problem hiding this comment.
Nothing here changes properties of non-directories. Doesn’t the project ID have to be set for every file as well?
There was a problem hiding this comment.
No, it is a per-directory construct
There was a problem hiding this comment.
I agree the flag is per-directory. The project ID itself?
| // and directories created after the fact. | ||
| // Any pre-existing subdirectories will still need to have the project | ||
| // ID set manually, so recurse onto them. | ||
| if dents, err := os.ReadDir(targetPath); err == nil { |
There was a problem hiding this comment.
If we should do this, I’d expect to take advantage of existing WalkDir.
There was a problem hiding this comment.
Why? WalkDir seems to do more work we don't need here, this version seems to be much simpler than calling WalkDir() here.
|
@mtrmac RE: your structural question - if the directory is already populated with files whose size exceeds the quota, all further writes fail with ENOSPC until sufficient files are deleted to bring you below quota (per a brief experiment I just conducted). Interestingly, I would be amenable to reordering things in Podman to avoid the need for this, but if so we should update the docs here to be clear that quotas should only be set on empty directories. While it seems like things don't break if we have pre-existing files, they also don't seem to work as expected, and pre-existing subdirectories definitely break things. |
That seems hard to manage. (For comparison, I think https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/tree/quota/project.c#n174 means that they only affect that one directory, with no other checking.)
I’d even prefer if the |
… oops, I missed the |
|
Personally I would think it would be good to see a podman PR with it vendored first with the test you promise. Then it is much easier for me to check before/after without having to figure out how to setup quotas on a test system |
We have previously relied on the PROJINHERIT flag for XFS quotas, which causes the ID of the parent directory to be recursively applied to subdirectories under the volume's parent directory. However, PROJINHERIT only applies to directories created after the project ID was first set. Pre-existing directories do not get the project ID if we only set it on the parent. This means that quota enforcement is not complete if we allow quotas to be set on directories that are not empty. We could set recursively but that comes with its own problems; quotas on directories that contain pre-existing files behave strangely. Relevant to containers/podman#25368 but is not a fix for that PR, more of a cleanup to make sure we don't make the same mistake elsewhere. Signed-off-by: Matt Heon <mheon@redhat.com>
mheon
force-pushed
the
fix_podman_25368
branch
from
1d83758 to
6d5de33
Compare
|
Reworked to block setting quotas entirely if the directory is not empty. I'll reorder quota setting on the Podman side to account for this. |
|
LGTM /retest |
We have previously relied on the PROJINHERIT flag for XFS quotas, which causes the ID of the parent directory to be recursively applied to subdirectories under the volume's parent directory. However, PROJINHERIT only applies to directories created after the project ID was first set. Pre-existing directories do not get the project ID if we only set it on the parent. This means that quota enforcement is not complete unless we recurse to subdirectories and apply the project ID to those as well.
Fixes containers/podman#25368