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

Multiple profile configuration #45

Merged
merged 38 commits into from Feb 1, 2022
Merged

Conversation

gerardbosch
Copy link
Contributor

@gerardbosch gerardbosch commented Jan 16, 2022

Improve organisation of configurations by extracting them to different environment files that enable different "backup profiles". Profiles specify a set of configurations and are loaded by passing them as a systemd parameter.

Resolves #12

mfeif and others added 4 commits January 6, 2019 20:10
Resolved conflicts:
	etc/restic/b2_env.sh.template
	etc/restic/b2_pw.txt
	etc/restic/b2_pw.txt.template
	etc/restic/pw.txt
	etc/systemd/system/restic-backup@.service
	usr/local/sbin/restic_backup.sh
Improve organization of configurations by extracting them to different
environment files that enable different "backup profiles". Profiles are
loaded by passing them as systemd parameter.
@gerardbosch
Copy link
Contributor Author

Hi @erikw, if we move ahead with this PR I would suggest to go with "squash-merge" as the log became a bit convoluted. Doing this the PR will be still co-authored I think :) Cheers!

for homedir in /home/*; do
if [ -f "$homedir/.backup_exclude" ]; then
exclusion_args+=" --exclude-file $homedir/.backup_exclude"
for backup_path in ${BACKUP_PATHS[@]}; do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question here: What will happen if BACKUP_PATHS=/home/ and, then there is a /home/someuser/.backup_exclude file? The file makes reference to a specific user, while the paths refer to the higher level directory /home.

The previous behaviour takes the someuser exclusion file in consideration, but after the change what will happen? I think it will be omitted, right? If that's right, may be confusing. What do you think? :D

Maybe aside of backup_paths/.backup_exclude, also all user homedir .backup_exclude can be considered.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this was the original intention, got-cha! Thanks

I had forgotten. We should look for .backup_exclude for each mount-point, and additionally each user dir in /home, if /home is in the backup path.

Will fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool 😊

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, please check 424bb0c

I tried it out locally, seemed to work... 😅

@mfeif
Copy link
Contributor

mfeif commented Jan 17, 2022

Thanks @gerardbosch for picking this up and running with it. I'm (also) having trouble navigating all of the changes since 2019, so I'll start from scratch and install this proposed merge on a new system and see if I find anything confusing or broken.

Good stuff! Thanks!

@erikw
Copy link
Owner

erikw commented Jan 17, 2022

@mfeif Hallo!

I'm happy to hear that you want to try this out. Perfect!

Could you maybe wait just 1 day or so, as there are still some changes coming in to this PR :)

But soon after that, having someone trying out the updated setup from scratch would be terrific!!

@mfeif
Copy link
Contributor

mfeif commented Jan 17, 2022

Of course!

@erikw
Copy link
Owner

erikw commented Jan 17, 2022

Great job @gerardbosch picking this up from @mfeif from the good start in #14. We're soon getting there!

Review status from me

  • Review changes
  • Contribute to README & small fixes
  • Test using the updated Makefile
  • Test using the new profiles on an actual system
  • After merge: update CHANGELOG
  • After merge: make new major release tag
  • After merge: Update PKGBUILD.
    • Just update to new tagged version, or also needing to update make envvars?

@gerardbosch @mfeif as I reviewed the changes I asked myself if it would make sense to make the /etc/restic/pw.txt & /etc/restic/backup_exclude not global but separate per profile. Maybe one would like to use different passwords for each profile? This is not a use case for me, as I'm using one profile only, but the question came up for me. Maybe this just makes it more complicated though

@mfeif
Copy link
Contributor

mfeif commented Jan 17, 2022

@gerardbosch @mfeif as I reviewed the changes I asked myself if it would make sense to make the /etc/restic/pw.txt & /etc/restic/backup_exclude not global but separate per profile. Maybe one would like to use different passwords for each profile? This is not a use case for me, as I'm using one profile only, but the question came up for me. Maybe this just makes it more complicated though

That seems an obvious thing that someone will eventually need/want. Good thinking. Putting those in .env files might be messy, so separate text files might be nice.

Can we still cascade well, in the event that there is NOT a per-target customization? Without the natural over-ride mechanism with ENV variables?

Or can those two configuration items just be IN ENV variables? On my (old) system, excludes are in there, but I have just one password.

@erikw
Copy link
Owner

erikw commented Jan 18, 2022

I think it should work with the same approach for overriding _default.env in a profile.env. To make it more clear, I'd suggest that we:

  • Rename /etc/restic/pw.txt to /etc/restic/pw_global.txt
  • Rename /etc/restic/backup_exclude to /etc/restic/backup_exclude_global.txt
  • Create a variable RESTIC_EXCLUDE_FILE=/etc/restic/backup_exclude_global.txt and put it in /etc/restic/_global.env and use it in restic_backup.sh.
  • Add two out-commented lines in the default.env just to demonstrate that this feature exist if needed
    # In simple setups e.g. only one backup profile, having a single password and exclude file is fine.
    # If you have multiple profiles and desire to have separate password for each, or exclude files, you can override _default.env values like this:
    #export RESTIC_PASSWORD_FILE="/etc/restic/pw_myprofile.txt"
    #export RESTIC_EXCLUDE_FILE=/etc/restic/backup_exclude_myprofile

@mfeif
Copy link
Contributor

mfeif commented Jan 31, 2022

No, it was the master branch on this repo, which doesn't include all the relevant changes. My notes will come in my next post in a little bit...

@mfeif
Copy link
Contributor

mfeif commented Jan 31, 2022

Hey friends, I'm finally getting to testing this. Here are the questions/concerns I have found while doing that.

First, I tried following the makefile instructions:

  1. The Makefile installs a /usr/local/sbin/nm-unmetered-connection.sh ... while I can see how that might be useful for folks, it seems out of scope for this utility. It doesn't seem like the restic scripts reference this script, but the .service files DO reference it, in fact, they depend on it.

  2. That script works with NetworkManager.service, which is not installed by default on a server, for example, Debian 11 doesn't have it.

  3. Ditto for /usr/local/sbin/systemd-email; I actually am grateful for that script, but is it about backups? This is a nitpick.

  4. The output of the makefile has some confusing stuff about installing things, then installing them again, and then rm'ing them:

    install -m 0600 etc/restic/_global.env.template etc/restic/_global.env
    install -m 0600 etc/restic/default.env.template etc/restic/default.env
    install -m 0600 etc/restic/pw.txt.template etc/restic/pw.txt
    install -d /etc/restic
    install -b -m 0600 etc/restic/_global.env etc/restic/backup_exclude etc/restic/default.env etc/restic/pw.txt /etc/restic
    rm -f etc/restic/_global.env etc/restic/default.env etc/restic/pw.txt
    

    It looks like there are two stanzas in the Makefile that do that; one is tagged the install-conf target, the other is just kinda hovering in the file.

  5. I like the starter kit for backup_exclude; those are sensible!

  6. In the Makefile, $(RM) is referenced, but not defined (maybe we get that for free from make?); maybe this falls back to just whatever rm is on the path; not sure. But also, I'm not sure why the script is rm'ing those (or attempting to)

  7. In my previous hacky version, there was also a way to have exclude definitions in the default.env... that seems cleaner than having path/target specific excludes in the global.env. Can we get that back?

  8. On step 4 (which links to the other step 3)... don't we have to source the _global.env first to be the same as the .service runner?

  9. In step 5, perhaps explain here whether the user needs to make symlinks, or if they're automatically created by systemd (or makefile?)

  10. In step 8, ditto about sourcing _global.env first?

  11. Regarding the timers; perhaps some prose about how as-is, all the timers that are symlinks that reference the same source file will fire at the same time (I think?); that's probably not what you'd want if you had say 4-5 targets.

The script file itself seems to work great! My test backup is showing up in B2...

That's a lot there; I'll clean this stuff off the server and go through the manual config notes next, but in case we want to talk about any of the above first, I'll stop here for the night.

@erikw
Copy link
Owner

erikw commented Jan 31, 2022

Really great feedback @mfeif! I add my replies inline below

  • The Makefile installs a /usr/local/sbin/nm-unmetered-connection.sh ... while I can see how that might be useful for folks, it seems out of scope for this utility. It doesn't seem like the restic scripts reference this script, but the .service files DO reference it, in fact, they depend on it.
  • That script works with NetworkManager.service, which is not installed by default on a server, for example, Debian 11 doesn't have it.

The addition of the unmeetered check comes from #17. I would agree that this is an optional feature and should not be installed by default. There could possibly be a separate make target like install-unmerted or something, or just manual. To keep the scope of this already quite large PR down, I created a follow-up issue for this at #58

  • Ditto for /usr/local/sbin/systemd-email; I actually am grateful for that script, but is it about backups? This is a nitpick.

Same here, #58. I would like to include the scripts in the repo as example or starts of custom solution, but it need not be installed by default. You are right.

  • The output of the makefile has some confusing stuff about installing things, then installing them again, and then rm'ing them:
    install -m 0600 etc/restic/_global.env.template etc/restic/_global.env
    install -m 0600 etc/restic/default.env.template etc/restic/default.env
    install -m 0600 etc/restic/pw.txt.template etc/restic/pw.txt
    install -d /etc/restic
    install -b -m 0600 etc/restic/_global.env etc/restic/backup_exclude etc/restic/default.env etc/restic/pw.txt /etc/restic
    rm -f etc/restic/_global.env etc/restic/default.env etc/restic/pw.txt
    

It looks like there are two stanzas in the Makefile that do that; one is tagged the install-conf target, the other is just kinda hovering in the file.

No worries about this. The Makefile does what is intended, but it does not matter because I will completely re-write the Makefile to solve #49 after this PR is merged. I have not decided exactly yet for now, but I think I will try to create a local build/ directory, modify the PREFIX paths, and then install all files in the build/ directory. Well have to see. But this will be addreseed after this PR is merged.

  • In the Makefile, $(RM) is referenced, but not defined (maybe we get that for free from make?); maybe this falls back to just whatever rm is on the path; not sure. But also, I'm not sure why the script is rm'ing those (or attempting to)

Yes you are correct. RM is set by make and defaults to rm -f. See https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html

This whole thinkg about copy and rm during install comes from #15 (check for background). However this will also be made in a different way when I fix #49. So no need to worry about this for now :)

  • In my previous hacky version, there was also a way to have exclude definitions in the default.env... that seems cleaner than having path/target specific excludes in the global.env. Can we get that back?

Hmm could you maybe elaborate on what you mean by definitions? I'm not sure I follow :).

Here's a comparison between default.env

To me, it looks like they both provide the same features and capabilities.

  • On step 4 (which links to the other step 3)... don't we have to source the _global.env first to be the same as the .service runner?

default.env will source _global.env :). See
https://github.com/erikw/restic-systemd-automatic-backup/blob/ef41587f84ce65a68325afa70641ae6411b7707a/etc/restic/default.env.template#L12

  • In step 5, perhaps explain here whether the user needs to make symlinks, or if they're automatically created by systemd (or makefile?)

$ systemctl enable creates the symlinks for automatically starting the service. I added a short note in the README step :). Thanks!

  • Regarding the timers; perhaps some prose about how as-is, all the timers that are symlinks that reference the same source file will fire at the same time (I think?); that's probably not what you'd want if you had say 4-5 targets.

Hmm that the targets use the same script (restic_backup.sh) should not be an issue. The system load could be a bit high if all timers are started at the same time though and multiple backups run at the same time. That is a good point!

I added a note in README about editing this value.

To solve the problem with multiple backup profiles firing off at the same time, we can sleep a random delay on each execution. This is a common way to solve the problem in cronjobs.

We can update restic-backup.service to

- ExecStart=bash -c 'source /etc/restic/%I.env && /usr/local/sbin/restic_backup.sh | systemd-cat'
+ ExecStart=bash -c 'sleep ${RANDOM:0:3600} && source /etc/restic/%I.env && /usr/local/sbin/restic_backup.sh | systemd-cat'

This would make it sleep some random time between 0 to 60 minutes.

@mfeif @gerardbosch sounds good? :)

The script file itself seems to work great! My test backup is showing up in B2...

This is the most important thing after all ✅ :D

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gerardbosch
Copy link
Contributor Author

+ ExecStart=bash -c 'sleep ${RANDOM:0:3600} && source /etc/restic/%I.env && >/usr/local/sbin/restic_backup.sh | systemd-cat'

Do you think the value could be reduced to 5 minutes or something like that? I'd expect my backup runs soon after my laptop starts instead of one hour later.

Could restic_backup.sh self delay in case another instance is running? or maybe better, monitor a lockfile or something like this before starting? For the people not dealing with multiple profiles, would be more predictable if the backup runs with consistent timing, like now, after you boot :)

@erikw
Copy link
Owner

erikw commented Jan 31, 2022

Sure! A user of multiple profiles can easily adjust the value anyways to optimize for their system.

The best solution would of course be something like a lock file (if load or bandwidth is a problem), but the complexity of this outweighs the benefits. For now, a 5 minute random sleep should suffice!

I'll push this in a moment.

Apart from that, I think the only remaining question was

in my previous hacky version, there was also a way to have exclude definitions in the default.env... that seems cleaner than having path/target specific excludes in the global.env. Can we get that back?

Here's a comparison between default.env

To me, it looks like they both provide the same features and capabilities.

Then we can soon finally merge this!

@mfeif
Copy link
Contributor

mfeif commented Jan 31, 2022

Great stuff, guys. I will do the manual instructions following today.

Thanks for addressing all of my questions point by point!

Apart from that, I think the only remaining question was

in my previous hacky version, there was also a way to have exclude definitions in the default.env... that seems cleaner than having path/target specific excludes in the global.env. Can we get that back?

Here's a comparison between default.env

To me, it looks like they both provide the same features and capabilities.

I agree that's the only thing; sorry I wasn't more clear. The comparision there is actually _global.env, not default.env, but really I just need to explain myself better: in my old version linked here; inspect the example.env file... we had local specific excludes:

BACKUP_EXCLUDES="--exclude-file /.backup_exclude --exclude-file /mnt/media/.backup_exclude --exclude-file /home/erikw/.backup_exclude"

If I'm reading the current set of files, one can only put excludes into _global.env... it seems like it would be common that different backup plans/targets might have their own exclude setups, and putting them all in _global kinda removes the point of even having the cascading/inheriting behavior. I'd recommend we put those back, and make sure that the script uses BOTH excludes from _global and specific .env files

@gerardbosch
Copy link
Contributor Author

Hi @mfeif if I'm understanding you fine, that's something already possible. Check if this comment helps: #45 (comment)

The thing is that you can override any variable defined in _global.env in any specific profile -this includes the $BACKUP_EXCLUDE. Does it make sense?

EDIT: Oh! I think I get what you mean! You want to be able to define multiple exclusion files, is it? I've seen somewhere in the repo something like $RESTIC_EXTRA_ARGS, that I think it could help (maybe it is a new feature not merged, but I've seen it somewhere, hehe). For sure @erikw can enlighten :)

@erikw
Copy link
Owner

erikw commented Jan 31, 2022

@gerardbosch exactly, I will add extra args feature here #56

@mfeif
Copy link
Contributor

mfeif commented Jan 31, 2022

Yeah, that's what I mean.

Looking at restic docs about excludes there are a number of ways to add excludes; if we want to keep that as files (rather than explicit mentions) we can pass more than one exclude file:

--exclude-file Specified one or more times to exclude items listed in a given file

I might suggest that the script just look in both places (specific and global) for RESTIC_BACKUP_EXCLUDE= declarations and pass multiple --exclude-file params to the ultimate restic command.

I would also suggest that we change that variable to RESTIC_BACKUP_EXCLUDES_FILE or even ADDITIONAL_EXCLUDES_FILE

Thanks!

@erikw
Copy link
Owner

erikw commented Jan 31, 2022

I would also suggest that we change that variable to RESTIC_BACKUP_EXCLUDES_FILE or even ADDITIONAL_EXCLUDES_FILE

@mfeif Good idea. Done ✅

After #56, one can for example do in some_profile.env

# Complete override of global exclude file set in _global.env
RESTIC_BACKUP_EXCLUDES_FILE=/different/exclude/file

# and/or add extra exclude files
RESTIC_BACKUP_EXTRA_ARGS="--exclude-file /exclude/file/a --exclude-file /exclude/file/b"

@mfeif
Copy link
Contributor

mfeif commented Jan 31, 2022

I'm not sure we need to use the --extra-args feature for my idea, since restic can take multiple --exclude-file arguments. Though I can imagine there will be other custom things that people want, so it's a good idea.

Would it be hard to modify the script to look for another env variable ADDITIONAL_EXCLUDES_FILE and add another --exclude-file parameter, just like _global? Splitting hairs here, I know, but I like the idea of it working the same way in both cases.

@mfeif
Copy link
Contributor

mfeif commented Jan 31, 2022

These comments are about the second set of instructions in the readme; the detailed ones:

  • In step 2, we better mention _global.env and the starter backup_exclude file.

  • In step 4, while it's true that a user can modify the script, I think our instructions on how to use the .env files would be better than the below:

    restic_backup.sh: A script that defines how to run the backup. Edit this file to respect your needs in terms of backup which paths to backup, retention (number of backups to save), etc.

  • In step 9, what if we change:

    There are some * check *-files in this git repo. Install these in the same way you installed the * -backup *-files and enable with sytemd

    to

    There are companion scripts to restic-backup.sh for checking; look in the repo in `etc/systemd/system` and `usr/local/sbin` and copy what you need over to their corresponding locations
    

I think everything else looks great! Nice and thorough. Good stuff!

@erikw
Copy link
Owner

erikw commented Feb 1, 2022

I'm not sure we need to use the --extra-args feature for my idea, since restic can take multiple --exclude-file arguments. Though I can imagine there will be other custom things that people want, so it's a good idea.

Would it be hard to modify the script to look for another env variable ADDITIONAL_EXCLUDES_FILE and add another --exclude-file parameter, just like _global? Splitting hairs here, I know, but I like the idea of it working the same way in both cases.

I see what you mean. It could certainly be done :). The aim of the project is to keep it simple and extensible, so have and EXTRA_ARGS would be more general and cover more cases, so let's go with this for now.

I'll get started with #56 asap!

  • In step 2, we better mention _global.env and the starter backup_exclude file.

✅ good point, fixed!

  • In step 4, while it's true that a user can modify the script, I think our instructions on how to use the .env files would be better than the below:

    restic_backup.sh: A script that defines how to run the backup. Edit this file to respect your needs in terms of backup which paths to backup, retention (number of backups to save), etc.

✅ good catch again. Fixed!

  • In step 9, what if we change:

    There are some * check *-files in this git repo. Install these in the same way you installed the * -backup *-files and enable with sytemd

    to
    ✅ sounds better, I inserted your suggestion

@erikw erikw merged commit d8f25cd into erikw:master Feb 1, 2022
@erikw
Copy link
Owner

erikw commented Feb 1, 2022

aaaaand merged!

Thanks a lot @mfeif & @gerardbosch for your collaboration :)

I will update CHANGELOG, make a new release and also update the arch PKGBUILD in a bit

@gerardbosch the squash-merge preserved co-authors like you said :)
Screen Shot 2022-02-01 at 10 10 43

@gerardbosch
Copy link
Contributor Author

Nice :) You're welcome!
I think I could show a couple of ideas/PoC to expand on usability or features in the next days, let's see

@gerardbosch gerardbosch deleted the multi-repo branch February 1, 2022 12:14
@mfeif
Copy link
Contributor

mfeif commented Feb 1, 2022

Thanks friends! Good stuff!

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.

Easier configuration
3 participants