-
Notifications
You must be signed in to change notification settings - Fork 485
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
Add support for Type=notify-reload with systemd #1148
Conversation
ebfdcaf
to
736b87c
Compare
736b87c
to
76b1477
Compare
I'm afraid we cannot accept this PR. This service type ( |
@eulerto, sorry, I don’t understand your concerns. This PR makes a single change that could be incompatible with older systemd versions, that is, updates Type= in |
58ba82b
to
18b8647
Compare
And from what I understand, this file is used for reference/documentation purposes only. That is, it is installed under
|
4ed0d00
to
f0630a6
Compare
I think the But I do agree with @eulerto and the default systemd config which people copy paste should work with at least the majority of systems that people are expected to run in production environments. Since both the latest RHEL and latest Ubuntu LTS don't even support this feature, currently is not the time for that. I'd be okay with merging a change to the comments of the config file, mentioning what could be change for systemd 253+. So basically inverting the comment addition that you made. |
To be clear, I do agree that most people will use the systemd file from their package management. But even that being the case, showing a systemd file in the docs that is broken for pretty much all systems that we expect people to run at the moment seems like a bad choice. |
23d8874
to
339ca6e
Compare
@JelteF, rebased and inverted the comment. I’ve also noticed that I’ve wrongly assumed that |
339ca6e
to
e4b3291
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main objection is related to the pgbouncer.service
modification. It is just a comment, I'm fine.
Type=notify-reload requires CLOCK_MONOTONIC field to be set “to allow the service manager to properly synchronize reload cycles”. See https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html
e4b3291
to
0a8e8ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds support for using pgbouncer with
Type=notify-reload
systemd service.See https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html
Note that
Type=notify-reload
services do not needExecReload=
command and useReloadSignal=
that conveniently defaults to SIGHUP.