enzingerm / snapborg

Synchronize snapper snapshots to a borg repository
GNU General Public License v3.0
35 stars 6 forks source link

Allow using bind mounts when backing up #31

Closed dflemstr closed 8 months ago

dflemstr commented 8 months ago

Before this PR, snapborg uses whatever is the "real path" of a snapshot to perform the backup. However, it turns out that borg uses the full canonical path of backed-up files when populating the file cache.

By using bind mounts, we can ensure that the path is always the same. This PR adds a flag which always bind mounts the snapshot under /var/run/snapborg/<config> before performing a backup. This increases the cache hit rate by a lot when running regular backups.

enzingerm commented 8 months ago

Thanks very much for this PR! Actually, just a few weeks ago I've become aware of this problem as I wondered about the slow backup execution. Bind mounts have also been the first solution that crossed my mind. I'll have a look at the code and merge this asap. Maybe it should even be opt-out instead of opt-in. Existing configurations shouldn't be hampered by the changed backup path as long as there are no custom (restore) scripts expecting the .snapshots/<id>/snapshot parent path.

dflemstr commented 8 months ago

Thanks for the comment. Let me know if you want me to make the changes to have the --bind-mount flag be on by default (ie. to maybe have a --no-bind-mount flag instead?) or if we can leave that to a subsequent PR

dflemstr commented 8 months ago

Maybe the error handling could be refined because if run as non-root, the first os.makedirs call would just raise an exception.

Gonna dwell on this for a bit to figure out what to do. Conceivably the snapborg binary could be setuid to root, or somebody might set up a special snapborg user that has write access to the /run/snapborg dir and so on. Actually I thought about doing that with the systemd units PR, but I couldn't find a way to reliably ensure/create a specific user in setup.py.

So in other words, I could add a check for os.geteuid() here, but the dir might still be writable despite running as a different user.

I added a commit which does the "ask for forgiveness not permission" thing for now though.

I see you merged the PR, so this will end up in a new PR :)