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.

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

When running under systemd, send notifications about server startup, shutdown, and config reload #11517

Merged
merged 4 commits into from May 4, 2021

Conversation

sgmiller
Copy link
Collaborator

@sgmiller sgmiller commented May 3, 2021

This is a noop on systems without systemd, and when not spawned by systemd. Note that "Type=notify" must be specified in the systemd service definition in order for systemd to spawn a notification socket and set the corresponding environment variable.

@vercel vercel bot temporarily deployed to Preview – vault May 3, 2021 19:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook May 3, 2021 19:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault May 3, 2021 19:39 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook May 3, 2021 19:39 Inactive
Copy link
Contributor

@swayne275 swayne275 left a comment

Choose a reason for hiding this comment

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

requesting changes because i don't think the log in question is accurate if err != nil

if sent {
c.logger.Debug("sent systemd notification", "notification", status)
} else {
c.logger.Debug("would have sent systemd notification (systemd not present)", "notification", status)
Copy link
Contributor

Choose a reason for hiding this comment

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

these are likely very infrequent, but why do we want this log?

Copy link
Contributor

Choose a reason for hiding this comment

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

also i think this can be inaccurate if err != nil - I looked at SdNotify and it'll return false with a non-nil error so this log might be confusing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logging was partially to assist implementation, but could also help people debugging since it requires correct configuration in systemd to work as well. You're right that the if sent/else block should only happen if err is nil.

@vercel vercel bot temporarily deployed to Preview – vault-storybook May 4, 2021 14:02 Inactive
@vercel vercel bot temporarily deployed to Preview – vault May 4, 2021 14:02 Inactive
@sgmiller sgmiller requested a review from swayne275 May 4, 2021 14:50
@vercel vercel bot temporarily deployed to Preview – vault-storybook May 4, 2021 16:30 Inactive
@vercel vercel bot temporarily deployed to Preview – vault May 4, 2021 16:30 Inactive
@sgmiller sgmiller merged commit 03c9933 into master May 4, 2021
@sgmiller sgmiller deleted the systemd-notif branch May 4, 2021 19:47
AndreyZamyslov pushed a commit to yandex-cloud/vault that referenced this pull request Jun 10, 2021
jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants