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

lvm2 pool: Export immutable snapshot of volume #461

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DemiMarie
Copy link
Contributor

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

@DemiMarie DemiMarie marked this pull request as ready for review April 1, 2022 18:16
@codecov
Copy link

codecov bot commented Apr 1, 2022

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

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

@DemiMarie DemiMarie force-pushed the snapshot-lvm2-backup branch 3 times, most recently from c19f2d2 to 9f8dbc0 Compare May 5, 2022 14:39
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.
There is no need to invalidate a cache if nothing will access it before
it is invalidated again.
It isn't used anymore.
@marmarek
Copy link
Member

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

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

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 DemiMarie marked this pull request as draft May 30, 2022 14:36
@DemiMarie
Copy link
Contributor Author

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.

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.

Restore failure - Unexpected EOF - retrieved data backup size exceed expected size
2 participants