Fix volume quota assignment #25417
Fix volume quota assignment #25417
Conversation
test/system/160-volumes.bats
Outdated
| skip_if_rootless "Quotas are only possible with root" | ||
|
|
||
| loop=$PODMAN_TMPDIR/loopdevice | ||
| dd if=/dev/zero of=$(loopfile) bs=1M count=25 |
There was a problem hiding this comment.
$loop here instead of $(loopfile)?
test/system/160-volumes.bats
Outdated
| umount $vol_path | ||
| losetup -d $loop_dev | ||
| rm -f $loop |
There was a problem hiding this comment.
This is fine for the quick test now but before merging it should be reworked to use a teardown function.
The reason is if anything in the test fails the cleanup here will never be run and we leak the loop devices.
Look at the test/system/180-blkio.bats test, maybe it is easiest to just place you test in that file as teardown() applies to all tests in the file.
There was a problem hiding this comment.
Makes me really miss defer() as we really should be adding the cleanup functions one at a time as we do the create/losetup/mount/etc
8ff2b0a to
612a37b
Compare
test/system/160-volumes.bats
Outdated
| # Minimum XFS filesystem size is 300mb | ||
| loop=$PODMAN_TMPDIR/loopdevice | ||
| dd if=/dev/zero of=$loop bs=1M count=300 | ||
| loop_dev=$(losetup -f) | ||
| losetup $loop_dev $loop | ||
| mkfs.xfs $loop_dev |
There was a problem hiding this comment.
# Minimum XFS filesystem size is 300mb
loop=$PODMAN_TMPDIR/disk.img
fallocate -l 300m ${loop}
run -0 losetup -f --show $loop
loop_dev="$output"
mkfs.xfs $loop_dev
This should be a bit more race safe I think and we may not have to copy 300mb of zeros.
And I still think you need to move this to 180 and then add proper teardown logic to unmount the fs and loop device.
There was a problem hiding this comment.
Would you object to me adding a 161-volume-quotas.bats and throwing this test in there along with the cleanup? I hesitate to throw things in unrelated files
test/system/160-volumes.bats
Outdated
| run_podman 1 $safe_opts exec $ctrname dd if=/dev/zero of=/two/onemb bs=1M count=1 | ||
| assert "$output" =~ "No space left on device" | ||
|
|
||
| run_podman rm -f -t 0 $ctrname |
There was a problem hiding this comment.
this is missing $safe_opts, this brings us to another cleanup problem.
Normally the default teardown takes care of deleting cotnainers. However as you used $safe_opts it has no idea they exist and the container is leaked so the tearodwn will have to deal with that as well.
test/system/160-volumes.bats
Outdated
| ctrname="testctr" | ||
| run_podman $safe_opts run -d --name=$ctrname -i -v $vol_one:/one -v $vol_two:/two $IMAGE top | ||
|
|
||
| run_podman $safe_opts exec $ctrname dd if=/dev/zero of=/one/onemb bs=1M count=1 |
There was a problem hiding this comment.
small nit, make the MB upper case then it will be easier to see the different file names and what they represent
|
Dropping WIP as c/storage patch is merged |
|
@Luap99 If we backport this, I don't think we strictly need the c/storage patch given it's just tightening restrictions on how the quota function is invoked; agree? |
Includes a patch for quotas that is needed for this PR. Signed-off-by: Matt Heon <mheon@redhat.com>
If the test passes without the c/storage change sure, not having to do backports in c/storage will make it much easier. |
There was a problem hiding this comment.
Please document the xfsprogs requirement in test/system/README.md and also add it to the test rpm package requirements (rpm/podman.spec line 145+)
While it is likely that xfs is always on fedora by default better safe then having to deal with failing gating tests due the missing mkfs.xfs
This resolves an ordering issue that prevented quotas from being applied. XFS quotas are applied recursively, but only for subdirectories created after the quota is applied; if we create `_data` before the quota, and then use `_data` for all data in the volume, the quota will never be used by the volume. Also, add a test that volume quotas are working as designed using an XFS formatted loop device in the system tests. This should prevent any further regressions on basic quota functionality, such as quotas being shared between volumes. Fixes containers#25368 Signed-off-by: Matt Heon <mheon@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
DO NOT MERGE until c/storage containers/storage#2264 merges.
Fixes #25368
Does this PR introduce a user-facing change?