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.

Sign up for GitHub

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

Improve systemd units #498

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

Improve systemd units #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87eeefa
contrib: systemd: btrbk.timer: run hourly
calestyo Nov 12, 2022
c9d95a6
contrib: systemd: run units only with existing config file
calestyo Nov 12, 2022
eb1d9e4
contrib: systemd: btrbk.timer: use default systemd `AccuracySec`
calestyo Nov 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions contrib/systemd/btrbk.service.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
[Unit]
Description=btrbk backup
Documentation=man:btrbk(1)
ConditionPathExists=|@CONFDIR@/btrbk/btrbk.conf
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not using systemd myself, and my knowledge is a bit limited here (and no, not going to read through all the docs, all those options are one of the reasons for me not to use systemd).

This sounds like a good thing to have. does that mean that the service will not fail if the config file does not exist, even if you start it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, AFAIU, it should not fail (that would require an AssertPathExists).

ConditionPathExists=|@CONFDIR@/btrbk.conf

[Service]
Type=oneshot
Expand Down
5 changes: 3 additions & 2 deletions contrib/systemd/btrbk.timer.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
[Unit]
Description=btrbk daily backup
ConditionPathExists=|@CONFDIR@/btrbk/btrbk.conf
Copy link
Owner

Choose a reason for hiding this comment

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

Is it a good idea to set this in the timer as well? a user might want to override the service with ExecStart=/bin/btrbk run -c some_other_config, which would somehow break the timer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... TBH, I'm not exactly sure how "bad" it is to run the time and let just the service not run because of the conditional.

Probably not at all, and at most it would be "conceptually" ugly, i.e. when the timer is considered to have been fired, but the service actually didn't.

But even this may not be considered that way by systemd experts.

On could argue, that if a user wants to override the .service he could as well just also override the .timer.

So.. yeah... I have no real strong opinion about this.

ConditionPathExists=|@CONFDIR@/btrbk.conf

[Timer]
OnCalendar=daily
Copy link
Owner

Choose a reason for hiding this comment

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

I think daily is a good default. Some want it hourly, some daily, and I know (at least one) doing it minutely ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forget about that commit... it was late in the night and I was pretty dumb ^^

I completely forgot that the times in the retention policies don't determine how often the tool runs, but how long the snapshots/etc. are kept... so hourly would be really bad.

It could however make sense, if somewhere we add the proper systemd way on how to override (described in systemd.unit(5)) the time, namely by adding a file:
/etc/systemd/system/btrbk.timer.d/foo.conf with just what shall be overridden:

[Timer]
OnCalendar=hourly

AccuracySec=10min
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure myself why I set this, must have read something when initially playing around with btrbk and systemd. Guess the idea was to tell the timer it's ok to be delayed a bit, as there might be other concurrent btrbk timers running (e.g. with different config and execution times)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well then it depends:

Was your idea to spread the time when these fire off (to not cause all load at the same time)? Then RandomizedDelaySec= would be the appropriate setting, AFAIU.

Otherwise, I, personally would say, 1min (default) would be enough.
Actually if you give system "so much time" just to fold multiple runs... it would rather cause more load (because more btrbks may run at the same time).

I could update the commit with whatever you like in the end. (The OnCaleder thing goes away of course)

OnCalendar=hourly
Persistent=true

[Install]
Expand Down