Notify service manager when start up is completed #1148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neilalexander
force-pushed
the
develop
branch
from
94ae0c5
to
01e7379
Compare
|
Not intentionally ignoring this PR BTW, but it has a knock-on effect to quite a few packages, so probably not for a patch release. |
klemensn
reviewed
Nov 3, 2024
| if _, err = notifyStartupCompleted(); err != nil { | ||
| log.Warnln("Error while sending start up notification:", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
There is no guarantee at this point (in the main goroutine) that startup tasks in other goroutines have been finished, since they don't synchronise.
So I think you are just racing against tun(4) setup just like -chuser is already racing against the control socket setup over at #927 (comment)
klemensn
reviewed
Nov 3, 2024
| @@ -272,6 +272,10 @@ func main() { | |||
| } | |||
| } | |||
|
|
|||
| if _, err = notifyStartupCompleted(); err != nil { | |||
There was a problem hiding this comment.
The first bool return value is never used, so why have it at all?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently there is no way to notify service manager when yggdrasil start up process is truly completed, i.e its TUN interface and admin API endpoints are up and running. This makes naive services, set to depend on the yggdrasil network, periodically crash on startup, requiring sub-optimal complications for trivial tasks.
This PR solves the issue by notifying the service manager after TUN interface and admin sockets are ready. Systemd support is implemented by sending a short
READY=1message to a UNIX datagram socket defined in aNOTIFY_SOCKETenvironment variable, seesd_notify(3).Changes
cmd/yggdrasil/main.go- Added invocation fornotifyStartupCompletedright after TUN interface and admin sockets are set up.cmd/yggdrasil/notify_startup_*.go- AddednotifyStartupCompletedfunction handling the process on linux and immediately returning on other platforms.contrib/systemd/yggdrasil.service*- ServiceTypeis changed tonotify, added a comment aboutNotifyAccessproperty.