lvm2 pool: Export immutable snapshot of volume

[DemiMarie]

Volumes returned by export() must be immutable, since otherwise the
backup will be inconsistent. Ensure this by exporting a snapshot of the
volume, no the volume itself.

Fixes QubesOS/qubes-issues#7198.

FIXME: tests

[codecov]

Codecov Report

Merging #461 (5eb6240) into master (4bfd808) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master     #461   +/-   ##
=======================================
  Coverage   65.81%   65.81%           
=======================================
  Files          53       53           
  Lines       10007    10019   +12     
=======================================
+ Hits         6586     6594    +8     
- Misses       3421     3425    +4     

Flag	Coverage Δ
unittests	65.81% <50.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
qubes/storage/lvm.py	83.36% <50.00%> (-0.40%)	⬇️

---

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 4bfd808...5eb6240. Read the comment docs.

[DemiMarie]

@marmarek how do you want me to deal with cleaning up the exported volumes?

[marmarek]

Volumes returned by export() must be immutable, since otherwise the
backup will be inconsistent.

The volume currently used for backup is immutable (in a sense that its content is not modified). VMs are writing to -snap snapshot (not the original volume), which is committed on VM shutdown.

While your change will make it more likely that VM size and data are taken from similar time, the behavior change here is actually harmful: it will backup older data than it could. In fact, I consider changing the backup export to take a snapshot of a "dirty" volume to backup newer version, not older. Theoretically, it should be sufficiently safe with journaled filesystem (as long as the snapshot operation itself is atomic).

Please note QubesOS/qubes-issues#7198 in practice not about the time disk usage is recorder, but about recording size of a different thing. And also, the consensus there is to fix it differently - changing how limit is enforced.

[DemiMarie]

While your change will make it more likely that VM size and data are taken from similar time, the behavior change here is actually harmful: it will backup older data than it could. In fact, I consider changing the backup export to take a snapshot of a "dirty" volume to backup newer version, not older. Theoretically, it should be sufficiently safe with journaled filesystem (as long as the snapshot operation itself is atomic).

That is true, and even for non-journaled filesystems it should at most require a call to fsck.

Please note QubesOS/qubes-issues#7198 in practice not about the time disk usage is recorder, but about recording size of a different thing. And also, the consensus there is to fix it differently - changing how limit is enforced.

Does this mean that I should close this PR as “won’t do”?

[marmarek]

Does this mean that I should close this PR as “won’t do”?

It may be a good base for the change I proposed above (which version to backup). But that needs a proper task description (extend QubesOS/qubes-issues#1239?), and also consider other storage drivers.

[DemiMarie]

Marking as draft for now.

It is worth noting that snapshotting in-use volumes breaks an invariant that currently holds, and which a direct dm-thin driver would need to consider. But that is doable.