Skip to content
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

Allow volume shrinking #479

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

a-barinov
Copy link

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.

Bypass size check for file and lvm if shrinking is allowed.
Implement size check for reflink as it wasn't there.
@codecov
Copy link

codecov bot commented May 28, 2022

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
Copy link
Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

marmarek commented Jun 6, 2022

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
Copy link
Contributor

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-barinov closed this Jun 12, 2022
@a-barinov a-barinov reopened this Jun 12, 2022
@a-barinov
Copy link
Author

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
Copy link
Member

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
Copy link
Member

marmarek commented Jul 8, 2022

PipelineRetry

@marmarek
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants