feat: Use override.conf files for systemd unit config #1596
Conversation
|
Making draft until we've had a chance to discuss in arch - particularly that this change will impact config stored in existing systemd unit files. |
|
@XavierChanth if you want to merge this then please go ahead. If not put it back to draft and I'll take another pass at how to migrate existing config. |
| # The line below runs the sshnpd service, with the options set in | ||
| # /etc/systemd/system/sshnpd.d/override.conf. | ||
| # You can edit that config with: sudo systemctl edit sshnpd | ||
| ExecStart=/usr/local/bin/sshnpd -a "$device_atsign" -m "$manager_atsign" -d "$device_name" "$delegate_policy" "$s" "$u" "$v" "$additional_args" |
There was a problem hiding this comment.
One thing I've been meaning to address, which may be suitable as part of this changeset is that when we use policy only the manager configuration will be -m "" which prevents the daemon from starting.
There was a problem hiding this comment.
Maybe we can change it to: $( [ -n "$manager_atsign" ] && "-m $manager_atsign" || "" ), and have a similar wrapper for policy.
There was a problem hiding this comment.
This can also be done in a later PR
There was a problem hiding this comment.
I was thinking this should actually be something like ${manager_atsign:+-m "$manager_atsign"}, but it turns out that systemd doesn't support parameter expansion
One option might be to wrap our command line inside a shell invocation, but I'm wary of creating a deeper process tree.
There was a problem hiding this comment.
Seems like the safest option is to allow sshnpd to try to pull those from the environment if they aren't explicitly passed.
I think we should improve the upgrade / migration path before we merge. |
| dest="$systemd_dir/$unit_name" | ||
| cp "$script_dir/systemd/$unit_name" "$dest" | ||
| if [ -f "$systemd_unit" ]; then | ||
| # migrate old config from systemd unit file to override.conf |
There was a problem hiding this comment.
This has been the focus of my testing for the past few days - making sure that we can migrate existing config into the new approach.
| launchd_plist="$HOME/Library/LaunchAgents/com.atsign.sshnpd.plist" | ||
| if [ -f "$launchd_plist" ]; then | ||
| echo "launchd config already in place" | ||
| # TODO: restart service? |
There was a problem hiding this comment.
@XavierChanth what would be the appropriate command here?
There was a problem hiding this comment.
From what I grok in the launchctl man page it's supposed to be this:
launchctl disable gui/$UID/com.atsign.sshnpd
launchctl enable gui/$UID/com.atsign.sshnpd
And I get a success code from both commands, but neither is actually stopping or starting the service...
There was a problem hiding this comment.
Ah, that doesn't work.. but this works:
launchctl kickstart -k gui/$UID/com.atsign.sshnpd
-k says to kill the running instance first.
There was a problem hiding this comment.
Confirmed by checking process ids for sshnpd
|
systemd part LGTM |
Fixes #1567 though we have more work to do to make the upgrade experience smoother
- What I did
override.conffiles for systemd unitsinstall.shto put in place (and not overwrite)override.confuniversal.shto place config intooverride.confand not disrupt existing config.- How to verify it
We'll have to do a pre-release once this is merged
- Description for the changelog
feat: Use override.conf files for systemd unit config