Issues with LVM snapshots and patterns #1072
Labels
No labels
blocked
breaking
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No milestone
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
borgmatic-collective/borgmatic#1072
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What I'm trying to do and why
I am trying to run with LVM snapshots while specifying patterns like this:
When I create a backup I see the following patterns in the debug log:
Now the backup also includes stuff like
/home/borgmatic/lvm_snapshots/723905dc4c6e4bc71ac6/srv/rocketduck/data/paas-postgres/data/base/1/2838_vm. I have the feeling that borgmatic fails to rewrite the+/-patterns. Will need to dig in, but though I'd open an issue here if you have a quick solution.Steps to reproduce
No response
Actual behavior
No response
Expected behavior
No response
Other notes / implementation ideas
No response
borgmatic version
2.0.2
borgmatic installation method
container
Borg version
borg 1.4.0
Python version
Python 3.12.9
Database version (if applicable)
No response
Operating system and version
No response
So I do wonder if this is at fault:
https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/hooks/data_source/lvm.py#L79-L83
since it only handles
Rpatterns.Reading more about how patterns actually work I stumbled upon https://github.com/borgbackup/borg/issues/7964 and it really makes me think that all patterns should get rewritten. What do you think?
My initial thoughts on this:
contained_patternslist has any root patterns in it—which should be the case, becauseget_contained_patterns()returns the root pattern as one of the contained patterns ("self" counts as contained).Ah yes, you are absolutely right. I missread the code (the nested forloop makes it imo hard to digest). Will add some logging and see where it breaks down.
Ok so the problem seems to be (and I don't know whether this is on purpose or not) that
get_contained_patternschecks viaif candidate.device == parent_devicethat the candidate is on the same device as the parent directory. But device is currently not set for most patterns:Given that the include patterns as such are not returned from
get_contained_patternsbecause the device is not set they will also not get rewritten… I founddevice_map_patternswhich sets the device for all root patterns from the config. I am unsure at which level this should get fixed though since I don't know the codebase well enough. I think device mapping include patterns indevice_map_patternswould most likely be a valid fix but it is hard to "map" a pattern to a device if it includes wildcards (which the root pattern cannot, but we could simply ignore wildcards?).Thanks for diving into the code for this one. Excellent sleuthing! FWIW I think your analysis is entirely correct here. I originally added the
candidate.device == parent_devicecheck to solve #1048, because snapshots don't "cross" paths on different devices and therefore contained patterns shouldn't either. And, yes,if pattern.type == borgmatic.borg.pattern.Pattern_type.ROOTindevice_map_patterns()is indeed causing your included patterns not to have any.devicevalue set.The problem of how to "map" a pattern if it includes wildcards is indeed a tough one. Here is some brainstorming for one way to maybe accomplish that:
*characters, skip the following logic. For all other patterns, continue with the following.collect_special_file_paths()does something very much like this, with the difference that it does so for all patterns and also filters the results to just special files.).devicefor the pattern to that device. Otherwise, if the devices don't match, then that means the wildcard pattern spans multiple devices and therefore.deviceshould be left asNone.This is pretty intricate and kind of annoying, and it involves a bunch of separate calls to do Borg dry runs, one per pattern if they all have wildcards (or regular expressions). So it's not great from a performance perspective. But I think it has the distinct advantage that it would actually work. And then for the common case of no wildcards (like your include patterns above), it would still be plenty performant because Borg wouldn't have to do any dry runs.
Thoughts and/or alternative ideas welcome!
Hi @witten, great answer but I think/fear we are overthinking this.
First off, let me start off with an explanation of how borg patterns do work (just to make sure we are on the same page and I understand them correctly):
This will backup stuff inside
/home(due to the recursion root) but will ignore/srveven though it is added via+since it is outside a recursion root. The other patterns limit the inclusion inside/hometo everything but the userapo(and forapoonly theimportantfolder).Assuming this is correct borgmatic currently does the following (at least as long as I understand the code correct): find a volume to snapshot as parent of the current root (ie
/home). This could be/homeor/in this case. Create a snapshot of that and rewrite the root inclusion path. I would argue that this behavior isn't a 100% correct because/home/someonecould be on it's own volume and would not be snapshotted (is that correct?). Even worse if the snapshot doesn't include the subvolume the data would not get collected?So I wonder if it would make sense to document a limitation along the lines of: "Only volumes at or above the recursion root are considered". Then we could maybe just adjust
get_contained_patternsto also include all patterns that clearly match below the recursion root and error out if a pattern would span multiple recursion roots because we cannot rewrite it (?).As for the dry-runs: I am really afraid of the performance hit.
As for the dry-run, I also think we would need to do it with all patterns at once and then perform filtering since something like this:
would yield
/home/apoif solely started with the recursion root (and could be on it's own volume), even if it is supposed to get excluded later on. So we would need a "global" view of this anyways which really doesn't seem worth the added complexity (code and execution wise). The provided example is simple and maybe not realistic but think of something using! /home/apoand how that would feed into all of this (because now/home/apo/importantwould not match anymore).This is consistent with my understanding too.
You are correct that
/home/someonewould not be snapshotted in this scenario, but this is an intentional/documented limitation, and my understanding is that it's a pretty standard limitation with other backup tools. From the borgmatic docs: "During a backup, borgmatic automatically snapshots these discovered logical volumes (non-recursively), temporarily mounts the snapshots within its runtime directory, and includes the snapshotted files in the paths sent to Borg. " The key part there being "non-recursively." In this situation, the user is expected to list both/homeand/home/someoneinsource_directoriesorpatternsif both should be snapshotted and included in backups. I'd be happy to have this clarified in the docs.That makes sense to me. If Borg ignores such patterns (like
+ srvin your example), then we could safely ignore it too. But an error might be nicer for the user.Okay, I think I see what you're saying. The patterns are interdependent (in terms of how they resolve to actual file paths) and therefore they can't be applied independently. I buy that argument, and I agree that it shoots a hole in my initial proposal.
But let's say we instead do a global Borg dry run of all patterns (which is conveniently way more performant) to get the corresponding actual paths. And then we lookup the device for each actual path. What then? How do we map those paths and devices back to the originating patterns so we can set the
.deviceon each pattern? I might just be missing something from what you suggested above.Sorry for not being more clear before. I am not suggesting to do a dry-run at all since mapping back the files to the patterns is hard to impossible as you say.
What I am suggesting is that we try to evaluate the patterns against the root patterns and if the pattern prefix matches the root pattern we should also return it from
get_contained_patterns. This obviously will not work for something like:since we don't know whether the wildcard would match
aorb(or both for that matter) but at least would be a able to handle simple cases where the pattern prefix matches a root pattern.Ah okay. That certainly seems like an improvement over the current behavior, but yeah, it does leave behind patterns whose path prefix can't be matched against the root pattern path. I guess that might be okay though because that's already a documented limitation...? From the docs:
Fixed in main. Here's the direction I went with on this one:
get_contained_patterns()basically already works as you describe, so I didn't change anything there.device_map_patterns()had the problem you identified of only setting the device for root patterns. So I expanded it to also include non-root patterns and only consider the start of a pattern's path (up until any globs / non-literal characters) for purposes of determining the device. This insures that, per your example, non-root include and exclude patterns also get their device set and therefore get their paths rewritten in the downstreamget_contained_patterns().Feel free to shoot holes in this approach.
Ah well, that is certainly a fix that works as well (at least for my usecase). No immediate holes I can see, but I did test it against my dataset and it worked fine. Thank you for the quick turnaround!
Awesome, thanks for testing and I'm glad to hear it worked!
Released in borgmatic 2.0.4!