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

Merging #479 (0a1085b) into master (bcdf577) will decrease coverage by 0.03%.
The diff coverage is 57.89%.

❗ Current head 0a1085b differs from pull request most recent head 1daff6b. Consider uploading reports for the commit 1daff6b to get more accurate results

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
- Coverage   65.82%   65.78%   -0.04%     
==========================================
  Files          53       53              
  Lines       10015    10015              
==========================================
- Hits         6592     6588       -4     
- Misses       3423     3427       +4     

Flag	Coverage Δ
unittests	65.78% <57.89%> (-0.04%)	⬇️

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

Impacted Files	Coverage Δ
qubes/storage/reflink.py	65.80% <33.33%> (-0.39%)	⬇️
qubes/storage/lvm.py	83.17% <45.45%> (-0.46%)	⬇️
qubes/storage/__init__.py	68.17% <100.00%> (ø)
qubes/storage/callback.py	83.67% <100.00%> (ø)
qubes/storage/file.py	80.68% <100.00%> (ø)
qubes/vm/__init__.py	82.66% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcdf577...1daff6b. Read the comment docs.

[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.