-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
(WiP) zfs pool draft #289
(WiP) zfs pool draft #289
Conversation
Extra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments, besides TODO already in the code.
Also some safety note: any unhandled exception while loading qubes.xml (__init__()
of both pool and volume classes, pool.init_volume()
) will prevent qubesd from starting, which is very bad (requires manual edit of qubes.xml to fix). Avoid any possibility of such case. One thing I'd be careful about is removing base zfs pool, while there are some encrypted pools based on it. I think you need to check for that in remove()
explicitly.
There is also more generic remark: since this adds non-trivial dependencies, I'd prefer it in a separate package (with correct dependencies set). The way it is now, it's impossible to do it right (it would either depend on zfs-stuff always, making it impossible to install without, or be broken by default with unfriendly error messages).
But we can keep it here for development and review time, for convenience.
@@ -45,6 +45,7 @@ properties: | |||
with its own state of the volume, or rather a snapshot of its template volume | |||
(pointed by a :py:attr:`~qubes.storage.Volume.source` property). This can be | |||
set to `True` only if a domain do have `template` property (AppVM and DispVM). | |||
TODO should `source` be folded into this arg? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm for it, but lets keep it compatible with the old API. For example: allow snap_on_start=Volume
(and then assert for source
not set), but also allow snap_on_start=True
, and then expect source
set. Alternatively add a new attribute replacing both, like snap_on_start_from
.
- :py:meth:`~qubes.storage.Volume.import_data` - return a path the data should | ||
be written to, to import volume data; for complex formats, this can be pipe | ||
(connected to some data-importing process) | ||
- :py:meth:`~qubes.storage.Volume.import_data_end` - finish data import | ||
operation (cleanup temporary files etc); this methods is called always after | ||
:py:meth:`~qubes.storage.Volume.import_data` regardless if operation was | ||
successful or not | ||
- :py:meth:`~qubes.storage.Volume.import_volume` - import data from another volume | ||
- :py:meth:`~qubes.storage.Volume.import_volume` - import data from another volume. TODO this is used when self.snap_on_data=True and self.source exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No snap_on_data
or snap_on_start
. It is using when cloning VM or otherwise importing data between already existing volumes (not importing data from "the outside").
It may be worth noting that even if the volume is created with an intention of its data being replaced with this function, volume.create()
is still called before it.
""" | ||
self.log.warning("zfs init_volume() vm={} volume_config={}".format( | ||
vm, volume_config | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.debug? also, it's duplicated below
I won't comment on similar lines further below, but most of the log messages looks more like debug not warnings (probably to be changed when moving out of draft state).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, the .warning was to make sure they all showed up. I think most if not all of the self.log
statements can be removed once this is more stable.
self.await_timeout() | ||
) | ||
|
||
async def await_timeout(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole function looks overly complex. I'd done it this way:
- when stopping last volume (
used_volumes
become empty) schedule a key unload and save a reference to scheduled task - when starting a volume, cancel scheduled unload task (also handling a case where it wasn't scheduled)
when no volumes are in use. | ||
""" | ||
self.used_volumes.discard(vm) | ||
self.used_volumes_last_empty = time.clock_gettime( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this version of of await_timeout
function, this looks wrong. Should update the timestamp only if used_volumes
is empty.
""" | ||
Returns the parent pool if found, otherwise raises an AssertionError. | ||
This function also moonlights as our method to retrieve a handle for | ||
'app' which we record and re-use when asking for passwords. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted elsewhere, there is a better way: you can get it via Pool.init_volume(vm)
-> vm.app
, which is guaranteed to be called before any volume.start()
- contrary to this function.
@@ -24,6 +24,7 @@ | |||
from qubes.storage import pool_drivers | |||
from qubes.storage.file import FilePool | |||
from qubes.storage.reflink import ReflinkPool | |||
from qubes.storage.zfs import ZFSQpool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice you import it here, but some actual tests would be even nicer ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, there would be general tests for all storage pools. @marmarek thoughts?
Thank you for the review, I will try to get around to addressing this at some point and keep this updated. This PR is relevant to the discussion at QubesOS/qubes-issues#1293 (I forgot to mention that in the brief). |
input=self.name.encode()+b'\n', # context for the prompt | ||
stdout=pw_pipe_out) | ||
# await pw_vm.run_service_for_stdio( | ||
# TODO THIS used to work, now it fucking broke. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you direct dom0, it needs R4.1 commit 50adc60. If you want to test on R4.0, you need to backport this commit.
Or maybe some other problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems likely. It used to work on 4.0, but after updating earlier this week it stopped working with the exception that run_service_for_stdio
for the AdminVM
class was not defined.
I'm extremely interested in this. Do you need funding to get this to pass tests and work? How much would you consider to pique your interest? |
@Rudd-O I am working on this on my own time (and am using this patch set as-is on my laptop), but if you are able to eventually allocate funds to the Qubes project towards @marmarek's time spent reviewing and answering questions about the Qubes APIs, that would make me feel less guilty for taking up his time. We also hit some sore spots re: the |
@marmarek would you like to discuss funding for this specific bug? |
(Lack of this is blocking me getting Qubes Network Server ported to Qubes 4.1 for a variety of reasons that would be super hard to explain shortly.) |
I'm fine with reviewing it whenever you give info it's ready for the next round of review. If you like to help financially, contribute to our OpenCollective, so we can fund more people offloading other tasks from me. |
Review comment here. The structure of the datasets is not quite what it should be. According to the inter-distro working group on getting ZFS on root file systems, the structure should be more or less like this:
The spec appears to be what will be followed by Fedora as well. Ref: https://docs.google.com/document/d/1m9VWOjfmdbujV4AQWzWruiXYgb0W4Hu0D8uQC_BWKjk/edit |
@marmarek 1) how do I contribute to the OpenCollective? 2) how do I earmark contributions for this project? |
@Rudd-O to clarify, which datasets do you propose prefixing with EDIT: Did you link to the right document? This one seems to be an unpublished draft detailing zfs-on-root for Ubuntu systems (neither of which applies here), and there's no mention of an inter-distro working group? EDIT 2: I didn't have a use-case for it, but adding an unencrypted wrapper in addition to the encrypted one, and allowing both to take arguments like |
Not sure I understand. Why do we need unencrypted anything? |
https://opencollective.com/qubes-os
You can add a comment when you donate, but read also this: https://www.qubes-os.org/donate/#do-donors-get-personalized-support-or-special-feature-requests |
@Rudd-O I'm assuming that people are using full-disk encryption, so additional encryption keys to enter for e.g. your VM templates or your jazz music collection might be overkill UX-wise. |
That was my assumption too — FDE or no game. I know people want to have
filesystem-level encryption, but that discloses too much — even the
names of the datasets are not okay for adversaries to know.
|
https://www.qubes-os.org/donate/#do-donors-get-personalized-support-or-special-feature-requests
Huge bummer. I would have loved to earmark the donation to this
specific feature. Lemme know when the policy changes so I can negotiate
what the developers need to get this through.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the various TODOs, there are various ways this code can be simplified quite a bit. Also, I suggest focusing on unencrypted pools, as QubesOS users generally use full-disk encryption.
pool=self.pool, | ||
vid=self.vid, | ||
rw=self.rw, | ||
size=self._size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size=self._size) | |
size=self._size, | |
**kwargs) |
|
||
self.save_on_stop = False | ||
self.snap_on_start = False | ||
self.rw = rw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.rw = rw |
if self.save_on_stop: | ||
raise qubes.storage.StoragePoolException( | ||
"TODO save_on_stop not supported" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is more important than encryption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I didn't (and still don't) understnad the expected semantics of save_on_stop
. If you can lay it out in a issue on the other repo I'd be happy to discuss and/or implement it.
As for being more important than encryption: That's entirely subjective. I needed (and need) encrypted, incremental backups - that's why I wrote this. I did not (and do not) need snapshots on start/stop, so I didn't implement that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had not considered the backups, sorry!
libzfs_core.lzc_create( | ||
self.vid.encode(), | ||
ds_type="zvol", | ||
props=i_totally_know_python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
props=i_totally_know_python | |
props={b'volsize': self.size}, |
#i_totally_know_python = DEFAULT_ZVOL_PROPS.copy() | ||
i_totally_know_python = {} | ||
i_totally_know_python[b"volsize"] = self.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#i_totally_know_python = DEFAULT_ZVOL_PROPS.copy() | |
i_totally_know_python = {} | |
i_totally_know_python[b"volsize"] = self.size |
if self.source: | ||
# we're supposed to clone from this, maybe. | ||
self.log.warning("zfs volume.create(): there is a .source!") | ||
# TODO probably want lzc_clone in some cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.source: | |
# we're supposed to clone from this, maybe. | |
self.log.warning("zfs volume.create(): there is a .source!") | |
# TODO probably want lzc_clone in some cases. |
i = run_command( | ||
[ | ||
"zpool", | ||
"list", | ||
"-Hp", | ||
"-o", | ||
"allocated", | ||
self.zfs_ns | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i = run_command( | |
[ | |
"zpool", | |
"list", | |
"-Hp", | |
"-o", | |
"allocated", | |
self.zfs_ns | |
] | |
) | |
i = run_command(["zpool", "list", "-Hp", "-o", "allocated", self.zfs_ns]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably be more idiomatic if the use of lists for run_command
was abolished and *args
was used on the receiver in question.
i = run_command( | ||
[ | ||
"zpool", | ||
"list", | ||
"-Hp", | ||
"-o", | ||
"size", | ||
self.zfs_ns, | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i = run_command( | |
[ | |
"zpool", | |
"list", | |
"-Hp", | |
"-o", | |
"size", | |
self.zfs_ns, | |
]) | |
i = run_command(["zpool", "list", "-Hp", "-o", "size", self.zfs_ns]) |
# TODO if any of this stuff fails we should likely revert the | ||
# whole thing to the previous state. | ||
|
||
# TODO at least use the qubes_cmd_coro() here | ||
# TODO get rid of sudo here if we can | ||
environ = os.environ.copy() | ||
environ["LC_ALL"] = "C.utf8" | ||
p = yield from asyncio.create_subprocess_exec( | ||
*[ | ||
"sudo", | ||
"zpool", | ||
"create", | ||
*DEFAULT_ZPOOL_CMDLINE_OPTIONS, | ||
self.zfs_ns, | ||
*self.block_devices, | ||
], | ||
stdout=subprocess.DEVNULL, | ||
stderr=subprocess.PIPE, # todo capture stderr | ||
close_fds=True, | ||
env=environ, | ||
) | ||
_, err = yield from p.communicate() | ||
if p.returncode != 0: | ||
raise qubes.storage.StoragePoolException( | ||
"unable to create ZFS zpool:[{}] {}".format(p.returncode, err) | ||
) | ||
|
||
# Set up permissions for new zpool to avoid having to run other | ||
# commands as root. This allows some basic protection from programming | ||
# mistakes, since we can whitelist the operations that can be performed | ||
# without root privileges: | ||
|
||
# TODO should we attempt cleanup if any of this fails? | ||
|
||
p = yield from asyncio.create_subprocess_exec( | ||
*[ | ||
"sudo", | ||
"zfs", | ||
"allow", | ||
"-ld", # this zpool + its descendants | ||
"-g", # -g TODO | ||
"qubes", # qubes gid / group TODO | ||
("encryption,load-key,refreservation,create,destroy,mount,clone,snapshot,hold," | ||
"bookmark,send,diff,sync,volmode,rollback,receive," | ||
"volsize,volblocksize,volmode" | ||
), | ||
self.zfs_ns, | ||
], | ||
stdout=subprocess.DEVNULL, | ||
stderr=subprocess.PIPE, | ||
close_fds=True, | ||
env=environ, | ||
) | ||
_, err = yield from p.communicate() | ||
if p.returncode != 0: | ||
raise qubes.storage.StoragePoolException( | ||
"unable to set permissions for ZFS zpool: {}".format(err) | ||
) | ||
|
||
p = yield from asyncio.create_subprocess_exec( | ||
*[ | ||
"sudo", | ||
"zfs", | ||
"allow", | ||
"-d", # ONLY descendants (NOT this zpool itself) | ||
"-g", | ||
"qubes", # qubes gid / group | ||
("refreservation,volmode,destroy,rollback,receive,volsize," | ||
"devices,volblocksize,volmode,encryption"), | ||
self.zfs_ns, | ||
], | ||
stdout=subprocess.DEVNULL, | ||
stderr=subprocess.PIPE, | ||
close_fds=True, | ||
env=environ, | ||
) | ||
_, err = yield from p.communicate() | ||
if p.returncode != 0: | ||
raise qubes.storage.StoragePoolException( | ||
"unable to set child permissions for ZFS zpool: {}".format( | ||
err | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO if any of this stuff fails we should likely revert the | |
# whole thing to the previous state. | |
# TODO at least use the qubes_cmd_coro() here | |
# TODO get rid of sudo here if we can | |
environ = os.environ.copy() | |
environ["LC_ALL"] = "C.utf8" | |
p = yield from asyncio.create_subprocess_exec( | |
*[ | |
"sudo", | |
"zpool", | |
"create", | |
*DEFAULT_ZPOOL_CMDLINE_OPTIONS, | |
self.zfs_ns, | |
*self.block_devices, | |
], | |
stdout=subprocess.DEVNULL, | |
stderr=subprocess.PIPE, # todo capture stderr | |
close_fds=True, | |
env=environ, | |
) | |
_, err = yield from p.communicate() | |
if p.returncode != 0: | |
raise qubes.storage.StoragePoolException( | |
"unable to create ZFS zpool:[{}] {}".format(p.returncode, err) | |
) | |
# Set up permissions for new zpool to avoid having to run other | |
# commands as root. This allows some basic protection from programming | |
# mistakes, since we can whitelist the operations that can be performed | |
# without root privileges: | |
# TODO should we attempt cleanup if any of this fails? | |
p = yield from asyncio.create_subprocess_exec( | |
*[ | |
"sudo", | |
"zfs", | |
"allow", | |
"-ld", # this zpool + its descendants | |
"-g", # -g TODO | |
"qubes", # qubes gid / group TODO | |
("encryption,load-key,refreservation,create,destroy,mount,clone,snapshot,hold," | |
"bookmark,send,diff,sync,volmode,rollback,receive," | |
"volsize,volblocksize,volmode" | |
), | |
self.zfs_ns, | |
], | |
stdout=subprocess.DEVNULL, | |
stderr=subprocess.PIPE, | |
close_fds=True, | |
env=environ, | |
) | |
_, err = yield from p.communicate() | |
if p.returncode != 0: | |
raise qubes.storage.StoragePoolException( | |
"unable to set permissions for ZFS zpool: {}".format(err) | |
) | |
p = yield from asyncio.create_subprocess_exec( | |
*[ | |
"sudo", | |
"zfs", | |
"allow", | |
"-d", # ONLY descendants (NOT this zpool itself) | |
"-g", | |
"qubes", # qubes gid / group | |
("refreservation,volmode,destroy,rollback,receive,volsize," | |
"devices,volblocksize,volmode,encryption"), | |
self.zfs_ns, | |
], | |
stdout=subprocess.DEVNULL, | |
stderr=subprocess.PIPE, | |
close_fds=True, | |
env=environ, | |
) | |
_, err = yield from p.communicate() | |
if p.returncode != 0: | |
raise qubes.storage.StoragePoolException( | |
"unable to set child permissions for ZFS zpool: {}".format( | |
err | |
) | |
) |
qubesd runs as root, so this code is not necessary.
@@ -24,6 +24,7 @@ | |||
from qubes.storage import pool_drivers | |||
from qubes.storage.file import FilePool | |||
from qubes.storage.reflink import ReflinkPool | |||
from qubes.storage.zfs import ZFSQpool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, there would be general tests for all storage pools. @marmarek thoughts?
@DemiMarie Thank you for your review. I've since split the code out into its own package as advised by @marmarek, so this particular PR is abandoned. I've used this module as the primary pool driver on my personal laptop and have been quite happy with it. You can find the revised code at: https://github.com/cfcs/qubes-storage-zfs Twice I had to boot from a recovery disk because upstream API changes broke |
You’re welcome! Even if you don’t intend to get this merged, it might be useful to have an open PR for code review purposes.
Would you be interested in submitting it as a contrib package? I understand that it isn’t ready for prime time yet, but if/when that changes, this would allow users to obtain it via
I have actually had to manually edit stuff in dom0 before (mostly One other question: what are the advantages of this over the BTRFS pool? RAID-Z is the only one that comes to my mind, but there could easily be others I am missing. |
That is the hope, that it can end up as a contrib package. Like I said I've been using it on my personal Qubes installations since I posted this, and it's worked pretty well for me, but there are still some gotchas that need to be hashed out, and I'd like to have it better documented as well. I'm kind of worried about releasing it as a contrib package and then having people unable to boot their VMs due to some kind of DKMS issue --- right now, if something goes wrong, all of
Note sure, but compiler and header files being out of sync does seem like a probable reason. It's been a while since I had anything as drastic as that happen though.
Main feature over BTRFS is encryption, so your incremental backups are encrypted at-rest and their integrity can be validated on the backup system without loading the key. Main drawback compared to BTRFS is that ZFS lacks a COW file cloning system call on Linux (BTRFS has that). Not really of big consequence for this particular codebase, since most of the magic happens with |
@cfcs : was the effort abandoned? Have you seen #7009 bounty offer for exactly this work/from @Rudd-O ? zpool live deduplication is a big advantage over brtfs from what I've been reading. Please feel free to shed some lights in the ongoing discussion of pool deduplication, here: https://forum.qubes-os.org/t/pool-level-deduplication/12654 |
https://github.com/cfcs/qubes-storage-zfs If this were released as a contrib package with the to-do items checked off such that I can trust my own systems with it, I would honor my promise of the bounty I offered before in #7009. |
@tlaurion (EDIT)
Ah, no, I had not seen that. In the sense that I use it every day on my own machines, the project is not abandoned, but it hasn't seen meaningful progress for a while now. This PR is effectively abandoned, since I deemed the approach @DemiMarie suggested of a plugin/contrib package better than merging it into Judging by the helpful issue posts on that repo, @DemiMarie has also taken it for a spin, but I don't know of any other users / testers. It feels to me like there's some way to go before this would be something I'd recommend to people who are not comfortable fixing DKMS build errors; I've personally had a number of issues with the kernel headers/kernel being out of sync with the fc32 DKMS packages for ZFS. Basically you need to plan 1-2 days of downtime whenever you type To sum up there are some things to deal with before I think it makes sense to offer this to non-expert users:
I'd be happy to review and merge patches to my repo, and I'd also be fine with someone forking it into their own contrib package, copying what they like and deleting the rest. As I've explained before, packaging is not really my strong side, so I doubt I will be doing any of that on my own in the near future. I basically did a lot of the easy lifting on this project to achieve what I wanted, and now I have a system that works for me, but I left the heavy lifting of polishing it up and making it mainstream-deployable as an exercise to the reader. I would be delighted to find such a reader here. :-) |
I actually haven’t. All of my suggestions have come from manual review of the code.
The best approach I can think of is to ship a DKMS package that accompanies this package. Fedora 32 is EOL so I am not surprised you are having problem with its DKMS package.
This is something that definitely needs to be worked on upstream. Writing a third-party storage driver should not be so difficult. I myself had a LOT of problems adding ephemeral volume encryption and ensuring that storage is cleaned up at system boot, so better documentation/cleaner APIs/etc would be amazing. Feel free to pester me with any questions you have, so that I can improve the documentation.
This is also a problem with all of the other storage drivers, sadly. |
@tlaurion I'm in about the same place as @cfcs as I use this driver every day, but havn't yet taken the time to clean it up. I did make a spec file, which you'll find in my fork. I did enough work to integrate building the zfs drivers within my I would like to collaborate with ya'll to get this in a good state, so don't hesitate to push tasks my way. By the way, I push my built packages to the following repo: https://repo.gpg.nz/qubes/r4.1. I can't make any guarantees, thouhg. |
Ah, okay! Well, thanks for doing that!
Yes, iirc I'm using fc34 or fc35, but the real issue is that
Thank you! I will take you up on that offer next time I nerdsnipe myself into working on this project.
That is great though, it means hopefully a solution can be found external to the people interested in ZFS integration?
Ah, that is cool. I think the best solution in terms of absolutely avoiding DKMS build errors would be to ship a custom Thanks for chiming in, I was happy to learn that I have a user! :-) |
@cfcs Indeed, the most fullproof solution is including |
Honestly I don't mind if this project doesn't deliver ZFS as part of the deliverables. I compile ZFS on my dom0s every time there is an update to either the kernel or ZFS (I run ZFS directly from |
@DemiMarie seconded. If CI needs to run integration tests, the |
It wouldn't be that much work for Qubes to have a backported |
@ayakael what they're saying here is that they don't want to use the |
I think I was maybe being dramatic re: the update story -- in practice it doesn't break that often, I just don't like to give people the impression that it's completely foolproof. |
I think that the separate package approach is useful to highlight potential shortcomings, even if the separate package is then installed by default? Anyway, we have a bunch of issues, some of which can be fixed in the separate package, and some of which require upstream work (I find the the template cow thing is the main thing I don't know how to fix). It would also be nice to have a clear view of where the functions are allowed to / expected to throw exceptions.
Maybe the solution is to structure my module differently, but I did not find a good solution regarding reporting of properties that are not available when the pool is faulted for example. |
I gave it a shot at writing my own module a while ago, and I gotta say the abstractions for the storage volumes just aren't there for me to understand how to implement one. |
Template CoW is 100% an upstream problem. No work required on your side. |
I assume ZFS-backed pools can always instaclone the current (or rather the pre-boot) snapshot of the template to produce a R/W block device for a VM using the template. |
The "problem" is that the r/w snapshot is not written to the Most of the time I don't really care where these are written; assuming both pools have enough space, it doesn't make much of a difference to the end-user. Given that they're only ever interpreted as sparse writes on top of an immutable base image, it would be nice if they would end up as sparse volume writes attributed to the VM since that helps with quotas and accounting. That would help you avoid scenarios where the thin provisioning blows up critical VMs (sys-net etc) when "less needed" VMs decide to overwrite the entire supposedly-sparse device, creating storage pressure in cases where the assumption that there is enough space doesn't hold. In practice the main problem I have with it is that if the VM storage is encrypted, like the per- |
It is placed on the VM’s template’s pool.
Volatile volume encryption (which includes swap) has been implemented. Encryption of the root volume has not yet been implemented. However, one can mark that volume as read-only, which will cause the VM to create a snapshot in the guest itself. This causes data that would be written to the root volume to be written to the volatile volume instead. @marmarek could this be made the default when volatile volume encryption is on? |
Ah, awesome! It seems to me that would make a lot of sense (same rationale as encryption for |
This can now be closed, right? |
This one got merged instead: #522 |
This PR aims to introduce two new
qvm-pool
pool modules with this interface:[user@dom0 ~]$ qvm-pool --help-drivers DRIVER OPTIONS zfs_encrypted zpool_name, ask_password_domain, unload_timeout zfs_zvol block_devices
zfs_zvol
which implements VM storage as ZFSzvol
volumes. In the current implementation it also deals withzpool
creation on eachblock_devices
parameter since that was the easiest way to get started from a clean slate when testing. If there are valid use-case arguments for splitting that off to a third module that could be done (i.e. if anyone thinks that it would be useful to implement azvol
pool on top of an existing dataset).zfs_encryption
which largely inherits fromzfs_zvol
, but on top of encrypted datasets insidezpool_name
that acts as "encryption key groups." The keys are optionally unloaded after a pool inactivity timeout (unload_timeout
). The keys are loaded by passing a raw file descriptor to an invocation ofzfs load-key {dataset}
from QRexec servicequbes.AskPassword
in a target domain.When used, this results in datasets and zvols in this form:
Adding those was a matter of:
For the purpose of key entry I am currently using this implementation of
/etc/qubes-rpc/qubes.AskPassword
in theask_password_domain
, which should probably be reworked (remember tochmod +x
):This requires having ZFS installed from
zfs-on-linux
with thepyzfs3
module built.To get that going (in addition to their instructions) I needed:
This is merely a draft, as witnessed by the many
TODO
comments, but it does "work," and I thought it might be useful to place here for review, and in case someone wants to collaborate on this with me.NOTABLY:
_import
functions are probably horribly brokenping:
qubes-core-admin