Allow volume shrinking

[a-barinov]

Change Volume.resize call to allow volume shrinking.
Bypass size check for file and lvm if shrinking is allowed.
Implement size check for reflink as it wasn't there.

[codecov]

Codecov Report

❌ Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.80%. Comparing base (bcdf577) to head (1daff6b).
⚠️ Report is 1013 commits behind head on master.

Files with missing lines	Patch %	Lines
qubes/storage/lvm.py	63.63%	4 Missing ⚠️
qubes/storage/reflink.py	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
- Coverage   65.82%   65.80%   -0.02%     
==========================================
  Files          53       53              
  Lines       10015    10023       +8     
==========================================
+ Hits         6592     6596       +4     
- Misses       3423     3427       +4     

Flag	Coverage Δ
unittests	65.80% <68.42%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[a-barinov]

This PR should not change anything in how volumes are currently resized. An the next step, patch to allow volume when restoring from backup will be submitted.

I see Codecov complaining about reduced coverage, let me know if this needs to be dealt with before PR is accepted.

[marmarek]

I see Codecov complaining about reduced coverage, let me know if this needs to be dealt with before PR is accepted.

A test for new resize parameter would be useful. See qubes/tests/storage_lvm.py for example - you can add something based on test_006_resize that attempt to shrink the volume (either with allow_shrink=True and False - and check for expected outcome).

[rustybird]

Is the storage driver API really the right place to distinguish between extending and resizing? Duplicating this safety check in each driver looks strange to me. Why not something like:

1. Add a new admin API method admin.vm.volume.Extend, which would centralize the safety check and then call the volume object's existing resize() method
2. Wrap this new method as qubesadmin.storage.Volume.extend(), and switch existing callers of qubesadmin.storage.Volume.resize() over to it (except for cases like qvm-volume resize --force)
3. Make ThinVolume.resize() and FileVolume.resize() unrestricted like ReflinkVolume.resize()

[marmarek]

2. Wrap this new method as qubesadmin.storage.Volume.extend(), and switch existing callers of qubesadmin.storage.Volume.resize() over to it (except for cases like qvm-volume resize --force)

I'm a bit worried about this step - if you update core-admin but not core-admin-client, then you'll end up without the safety check at all. In dom0, a package dependency could be used to avoid this, but not in sys-gui. Maybe sys-gui is still rare enough to not care about it right now... ?

[rustybird]

In dom0, a package dependency could be used to avoid this, but not in sys-gui.

I hadn't thought of that... @a-barinov's approach of having the safety check in the storage driver might be the way to go then. That also avoids introducing a TOCTOU problem if there are concurrent API calls. (Is there something ReflinkVolume and FileVolume could use to enforce allow_shrink=False race free, similar to lvextend in ThinVolume? os.posix_fallocate() fits, but it probably has some side effects?)

[a-barinov]

A test for new resize parameter would be useful. See qubes/tests/storage_lvm.py for example - you can add something based on test_006_resize that attempt to shrink the volume (either with allow_shrink=True and False - and check for expected outcome).

Tried doing this, but can't find a way to re-run CI test to check if the tests I added do make sense / do not fail. Deleting this PR and creating a new one does not sound like a good solution.

[marmarek]

Tried doing this, but can't find a way to re-run CI test to check if the tests I added do make sense / do not fail.

We're doing some maintenance on CI, should be back online in few hours.

[marmarek]

PipelineRetry

[marmarek]

This looks good as is, but there needs to be further change to actually use the new parameter (expose it in api/admin.py, and then add support in qvm-volume tool). The backup case is handled by ImportWithSize method, so no need to do anything there.