-
-
Notifications
You must be signed in to change notification settings - Fork 115
Qubes OS ZFS pool driver #522
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
Conversation
d8badfa to
a475cbf
Compare
|
All review comments resolved. Thank you! |
Codecov Report
@@ Coverage Diff @@
## main #522 +/- ##
==========================================
+ Coverage 65.80% 67.09% +1.28%
==========================================
Files 53 54 +1
Lines 10096 11072 +976
==========================================
+ Hits 6644 7429 +785
- Misses 3452 3643 +191
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
f188362 to
4a99727
Compare
b24c9a9 to
89a1c66
Compare
|
OK. This is ready for final review. I've ironed out a bunch of bugs and this works now. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2023031403-4.2&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2023021823-4.2&flavor=update
Failed tests27 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/60652#dependencies 9 fixed
Unstable tests
|
|
Looks like I didn't add any OpenQA failures! (They look a bit concerning tho.) |
|
And the pipeline failed again. |
|
See updated report...
Focus on this section. There are basically three concerning things:
|
|
https://openqa.qubes-os.org/tests/68435#step/switch_pool/43 |
|
I donno man. I have the exact same udev rules on my system, yet I can create, clone and start VMs no problem. Lemme see how to repro this. EDIT: I repro'd. Figuring a fix out right now. |
|
The fix is in! It had nothing to do with udev rules. Basically it was a new patch I added to resolve @DemiMarie 's request that clone / import be atomic, whereby renaming the dataset from a temporary one to its final destination caused failure if the parent of the final destination did not exist. Resolved easily with a It bears noting that the reason the test https://openqa.qubes-os.org/tests/68435#step/switch_pool/43 fails with that particular VM and in that particular case is because the root volume from its template is ZFS, but the other volumes aren't, which means the parent of the root volume clone doesn't exist (up until copy-on-write creation) when the VM is about to start. The fix I just pushed solves that issue. |
|
It irks me enormously that errors do not dump tracebacks in qubesd's log. This is not right! Show me the traceback please! |
|
The test failure is because of the kernel version in the test VM. I can't rerun the CI. |
|
The manually-edited OpenQA run succeeded with the change newly-applied to PR QubesOS/qubes-core-admin-linux#118 . https://openqa.qubes-os.org/tests/68567#step/system_tests/25 Woohoo! |
|
There is some weird failure with rpmcanon while installing the XFCE template here: https://openqa.qubes-os.org/tests/68657/video?filename=video.ogv (at the very end) I cannot reproduce this locally. |
Don't worry about it. It's because the template is not there at all. |
|
FYI before merging, I'd like you to squash this into a reasonable number of commits - especially fixes for things added in this PR should be merged into the commit that adds the code (in other words: don't leave known-buggy commits). If that means adding the driver in one big commit, I'm okay with that. |
The same code works fine in release 4.1 and is therefore safe to backport.
|
I've squashed it all. |
|
There are 3 not resolved comments. The one about snapshots on import you can decide to ignore - up to you. But other two need handling. |
Thanks. I don't kow how I missed these. They've been addressed. |
|
Yeah we can't do the trick of import data as we had speculated, because of this (in The Why would there be a |
|
Let me offer a precision here, which is that we can do the trick in one way, by issuing a `BLKDISCARD` operation on the block device within `import_data()` That would be instantaneous.
|
|
The builder is failing: |
|
PipelineRetryFailed |
|
Anything else missing? |
I think it's okay now. I'll let the current test run to finish first and do a few manual tests, but I hope it will be okay. |
|
The qrexec tests failed, but they failed for all storage variants, so I guess that's okay since every other test did pass. |
|
FYI, I have checked manually that even if you have some pool and/or qubes on zfs but zfs itself is not supported (no tools and/or no kernel module), qubesd works just fine, and other qubes can be used normally. |
This software allows a user who has a ZFS pool in his Qubes OS dom0 to create Qubes VM volumes directly on his ZFS pool. In this sense, it is analogous to the LVM or the reflink driver that Qubes OS ships with. With this software, it's possible for users of Qubes OS on ZFS to continue using ZFS as their storage backing, without having to choose one or the other.
Usage
Upon pool creation time, the user specifies the container dataset for the Qubes volumes like so:
From then on, Qubes OS stores VM volumes under the
mytankpool/vmsdataset, in the following convention:These will have corresponding backing device files
/dev/mytankpool/vms/<VM>/<volume name>.The driver automatically manages revisions of volumes as ZFS snapshots (but does not interfere with third party snapshots). A revision snapshot is taken of every persistent volume upon VM shutdown, which the user can later revert to. The standard
revisions_to_keepparameter is honored (by default it is1and cannot be lower than1).Testing
A decently-comprehensive test suite has been included, and the integration tests have been amended to also exercise the ZFS driver. Tests will only run if the command
zfsand the commandzpoolexist in the test system. Unit and integration tests create a ZFS pool backed by a sparse file in/rwor/var/tmp, depending on which one has more than 32 GiB free disk space. No preexisting ZFS pools are used by the tests, which avoids any potential data loss. The test step has been amended to install ZFS directly from the official project repository — thank you @marmarek for the necessary changes in the base image to be able to build kernel modules.Every line of the driver is typehinted using
typing; no type errors are present. It is suggested that the project adopttox, configuring it to runmypyon every checkin (perhaps initially deliberately ignoring most of the source code until such time that it is ported to use types).