New option disable snapshots to use a private volume directly#690
New option disable snapshots to use a private volume directly#690marmarek merged 19 commits intoQubesOS:mainfrom
Conversation
This method is called by API endpoints expecting a valid number. This commit adds a new parameter 'allow_negative' to be able to work with negative numbers. # Conflicts: # qubes/api/__init__.py
This property is only applicable to private volumes (snap_on_start = False, save_on_stop = True). This can be enabled by setting revisions_to_keep = -1. # Conflicts: # qubes/storage/__init__.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 70.51% 70.62% +0.10%
==========================================
Files 61 61
Lines 13312 13395 +83
==========================================
+ Hits 9387 9460 +73
- Misses 3925 3935 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Guiiix
changed the title
|
Thank you for tackling this! It should make a big difference with database workloads. For file-reflink IMO it would be neater if the snapshot_disabled volume only had a _path_dirty file without a _path_clean file, instead of the other way around: That would prevent some combinations which don't make sense, e.g. accidentally It would also keep _path_dirty = path = "the thing that's connected to Xen if the VM is running" constant in all cases. In lvm_thin, I always find the indirection in path/_vid_current and block_device a bit confusing. * Edit: Or rather hardlinking _path_clean to _path_dirty and then removing _path_clean, so there's no chance of accidentally overwriting an existing _path_dirty |
Sorry, I didn't get that export should be possible for a snapshot_disabled volume if it's stopped! Okay that might be a point in favor of the way you did it, because if export() instead returned _path_dirty to a caller that doesn't immediately open the file, and then the user re-enables snapshots and starts the volume, later on the caller would use the actual live data. :( Or is this also an issue with the current approach, but in the other direction? A volume with snapshots enabled is exported to a caller that doesn't immediately open the returned _path_clean file, and then the user disables snapshots and starts the volume. Another reason to make export() return an open file handle - instead of a file name, which is inherently racy. Maybe it doesn't even matter in practice, if all callers (even backup) of export() open the file quickly enough? |
|
Making |
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
Ok, I've read the whole thing, and it looks pretty solid! I have just one minor request below.
I'll do some testing now...
| " <-- The VM is running, backup will contain " | ||
| "its state from before its start!" | ||
| " <-- The VM is running and private volume snapshots " | ||
| "are disabled. Backup will fail!" |
There was a problem hiding this comment.
Squash this commit into "backup: prevent backup of running VM when private volume snapshots are disabled"?
|
With LVM pool, I was allowed to clone a running AppVM that has revisions_to_keep=-1 on its private volume... |
|
Thank you for all your feedbacks!
Ah yes, I have to add this, thanks!
The volume would be considered dirty even when stopped. This is not important?
Sorry about that, I did the check in quesadmin but this is not relevant. Now that we have volumes state files, I can do it in qubesd. |
If enabled, COW image is not created/commit/destroyed anymore. Original private.img file is provided to libvirt (no snapshot). # Conflicts: # qubes/storage/file.py
If enabled, all LVM snapshots are removed and the private volume is used directly provided to libvirt. # Conflicts: # qubes/storage/lvm.py
Disables snapshots of a private reflink volume if enabled. # Conflicts: # qubes/storage/reflink.py
This can lead to unexpected behavior or even data loss # Conflicts: # qubes/tests/api_admin.py
…e disabled Backup a running volume is prohibited # Conflicts: # qubes/backup.py # qubes/tests/api_admin.py
# Conflicts: # qubes/tests/api_admin.py
This allows to track if a volume is currently started or not without having to check the related VM state. # Conflicts: # qubes/storage/__init__.py
This allows to track if a volume is currently started or not without having to check the related VM state. # Conflicts: # qubes/storage/__init__.py
For exemple, if a DispVM has a TemplateVM having a private volume with snapshots disabled, startup must be denied.
A TemplateVM cannot be started when having snapshots disabled on its private volume and at least one DispVM running # Conflicts: # qubes/vm/mix/dvmtemplate.py
…vm is running # Conflicts: # qubes/tests/api_admin.py
It's not surfaced to the user, in fact nothing except for the individual storage drivers themselves ever calls is_dirty(). To me it seems like the abstract is_dirty() storage driver API method ought to be deleted from |
|
With the last commit, the file is renamed |
|
Nice, that actually feels like the most natural way to fit in the feature.
|
qubes/storage/__init__.py
Outdated
|
|
||
| async def import_volume(self, dst_volume, src_volume): | ||
| """Helper function to import data from another volume""" | ||
| src_volume = self.get_volume(src_volume) |
There was a problem hiding this comment.
Source volume for import usually is from another VM, so get_volume() won't work if you give it just a name. You can either require also source vm object in arguments, or simply require Volume object and don't support name in src_volume arg - in which case the get_volume call is unnecessary.
There was a problem hiding this comment.
Ah I see. That makes sense, thank you!
Thank you, if you want to try it on R4.2, you can use this branch: I am on R4.2 too :) |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025070405-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061004-4.3&flavor=update
Failed tests15 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 11 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:5 performance degradations
Remaining performance tests:67 tests
|
5 participants
This PR is related to issue QubesOS/qubes-issues#8767
A new option disable snapshots can be set on volumes (lvm, file, reflink and zfs supported). When enabled on a private volume, the volume is used directly instead of doing a snapshot and use it.
The option can be enabled by setting
revisions_to_keep = -1Using the volume directly saves space but requires features like backup, dispvm or cloning to be disabled when the volume is in use.