feat: Add support for systemd notify #1954
Conversation
- Integrated systemd notification support using github.com/coreos/go-systemd/v22/daemon. - Updated manager/runner.go to signal readiness to systemd. - Modified .golangci.yml to include github.com/coreos/go-systemd in linters-settings. - Updated dependencies in go.mod and go.sum for github.com/coreos/go-systemd/v22 v22.5.0.
|
Hello @jmurret I'm sorry to bother you, maybe I got the wrong recipient, but could you please tell the owners of the code to perform the review? |
|
thanks @dyudin0821 Iam not familiar with go-systemd, but do you know how this will interact with non-linux operating systems that we have releases for? Ideally, I think in adding functionality we'd probably want to add similar behavior that is consistent across operating systems if possible. |
There was a problem hiding this comment.
In addition to my note about moving the signal into the top-level main, we might also want to look at whether we want to bring in this entire dependency. The systemd notify protocol is rather simple. In Nomad we implemented the protocol in a single file: sdnotify_linux.go, which allowed us to have a non-Linux code path that was a no-op in sdnotify_default.go
@jmurret Thank you so much for taking the time to review my change! Sending notify using go-systemd is implemented in such a way that if the OS does not support systemd, the NOTIFY_SOCKET environment variable will simply not be set, and the function will return (false, nil), indicating that the function will safely handle this situation. https://github.com/coreos/go-systemd/blob/main/daemon/sdnotify.go#L48 |
- Implemented a channel (`readyCh`) to signal application readiness to systemd - Ensured robust handling of readiness signaling with error logging
There was a problem hiding this comment.
Thanks for improving upon the original approach here @dyudin0821. I've left one more suggestion to tighten up shutdown. There was also a failing test but it looked like a panic in an unrelated area of the code? Once you've made the change I've suggested, I'd rebase on main just to make sure you've got the latest bug fixes from the CT team.
| @@ -163,6 +182,7 @@ func (cli *CLI) Run(args []string) int { | |||
| if err != nil { | |||
| return logError(err, ExitCodeRunnerError) | |||
| } | |||
| runner.SetReadyChannel(readyCh) | |||
There was a problem hiding this comment.
Given that we're shutting down the runners in these signal handlers, shouldn't we have a context on the ready channel that we can close to stop that goroutine as well?
There was a problem hiding this comment.
Hello @tgross
Thank you very much for your contribution and time spent!
If I understood your request correctly, then the idea is to create a context with cancellation, pass the context to the goroutine listening to the channel, and сanceled the context when shutting down the runners.
I apologize if this implementation isn't perfect, as this is my first experience making such changes. Please review the changes and let me know if further adjustments are needed.
There was a problem hiding this comment.
As for the fallen CI pipeline with tests, I noticed that there is a similar problem for neighboring PRs. I will be glad to help you fix the dropped tests
There was a problem hiding this comment.
I apologize if this implementation isn't perfect, as this is my first experience making such changes. Please review the changes and let me know if further adjustments are needed.
Looks very close. I've added a suggestion for two one-line fixes and this will be good-to-merge.
As for the fallen CI pipeline with tests, I noticed that there is a similar problem for neighboring PRs. I will be glad to help you fix the dropped tests
Oof, ok... let's not worry about it in this PR then.
- Introduced context management with cancellation in Run method. - Refactored signal handling to properly clean up and stop goroutines. - Added context cancellation to stop the readiness signaling to systemd.
| @@ -163,6 +182,7 @@ func (cli *CLI) Run(args []string) int { | |||
| if err != nil { | |||
| return logError(err, ExitCodeRunnerError) | |||
| } | |||
| runner.SetReadyChannel(readyCh) | |||
There was a problem hiding this comment.
I apologize if this implementation isn't perfect, as this is my first experience making such changes. Please review the changes and let me know if further adjustments are needed.
Looks very close. I've added a suggestion for two one-line fixes and this will be good-to-merge.
As for the fallen CI pipeline with tests, I noticed that there is a similar problem for neighboring PRs. I will be glad to help you fix the dropped tests
Oof, ok... let's not worry about it in this PR then.
- Removed redundant cancellation of context in signal handling. - Ensured consistency in context management throughout Run method.
There was a problem hiding this comment.
LGTM! Thanks @dyudin0821!
I'll merge once CI is done and that'll go out whenever CT gets released next.
Purpose:
See the description of PR #1794 and #1841 respectively for detail info.