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

Jump to bottom

client, server systemd units: make Restart=always truly respected #184

Merged
merged 2 commits into from
Jan 11, 2022
Merged

client, server systemd units: make Restart=always truly respected #184

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ade7b8
client, server systemd units: make Restart=always truly respected
strohel Jan 6, 2022
d7fa255
Apply review suggestions, make changes to units more conservative
strohel Jan 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions client/innernet@.service
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,17 @@ Description=innernet client daemon for %I
After=network-online.target nss-lookup.target
Wants=network-online.target nss-lookup.target
PartOf=innernet.target
# Disable systemd's unit start rate limiting logic, which could override Restart=always.
# See https://unix.stackexchange.com/q/289629/352972
StartLimitIntervalSec=0

[Service]
Type=simple
ExecStart=/usr/bin/innernet up %i --daemon --interval 60
Restart=always
# When the daemon exits, wait this amount of secs before restarting. Prevents innernet from
# start-looping each 100ms for example when there is a problem reaching the server.
RestartSec=60
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's ok with you, let's make this 10 rather than 60 since it may be a temporary issue and we should be retrying harder if a failure has caused innernet to not fetch peers and update the interface.


[Install]
WantedBy=multi-user.target
6 changes: 6 additions & 0 deletions server/innernet-server@.service
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@
Description=innernet server for %I
After=network-online.target nss-lookup.target
Wants=network-online.target nss-lookup.target
# Disable systemd's unit start rate limiting logic, which could override Restart=always.
# See https://unix.stackexchange.com/q/289629/352972
StartLimitIntervalSec=0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not so sure we want to modify this unit much at all - if the server fails it's more likely to be a permanent problem and the typical systemd "give up" logic makes more sense, in my opinion.

Personally, I'd suggest we should only slightly increase the RestartSec value and leave it otherwise as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not so sure we want to modify this unit much at all - if the server fails it's more likely to be a permanent problem and the typical systemd "give up" logic makes more sense, in my opinion.

That's a good point. I've made changes to systemd units more conservative in a fixup commit to resolve this and the 2 other comments.


[Service]
Type=simple
Environment="RUST_LOG=info"
ExecStart=/usr/bin/innernet-server serve %i
Restart=always
# When the daemon exits, wait this amount of secs before restarting. Prevents innernet from
# start-looping each 100ms for example when there is a problem reaching the server.
RestartSec=60
Copy link
Member Author

Choose a reason for hiding this comment

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

For client, the restart interval of 60 secs is somewhat natural as it is equal to the fetch interval.

But I don't have an idea what the server restart interval should be. Perhaps a bit less?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think it should be closer to 1s than 60s.


[Install]
WantedBy=multi-user.target