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

Add support for Type=notify-reload with systemd #1148

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

tie
Copy link
Contributor

@tie tie commented Aug 26, 2024

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 need ExecReload= command and use ReloadSignal= that conveniently defaults to SIGHUP.

@tie tie force-pushed the notify-reloading branch from aa9cd7c to b21dd62 Compare August 26, 2024 09:12
@tie tie marked this pull request as ready for review August 26, 2024 09:18
@tie tie force-pushed the notify-reloading branch 4 times, most recently from ebfdcaf to 736b87c Compare August 26, 2024 15:20
@eulerto
Copy link
Member

eulerto commented Sep 14, 2024

I'm afraid we cannot accept this PR. This service type (notify-reload) was added in version 253. There are still supported operating systems that use old systemd versions like Ubuntu focal (245), Ubuntu jammy (249), Rocky 8 (239) and Rocky 9 (252). Once the main distributions support this feature we can merge this PR.

@tie
Copy link
Contributor Author

tie commented Sep 14, 2024

@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 etc/pgbouncer.service. If some distributions use the provided example unit as-is, we can document that pgbouncer supports Type=notify-reload in the comment above instead and update it later. sd_notifyf has been available for decades and Type=notify should simply ignore unknown fields like MONOTONIC_USEC.

@tie
Copy link
Contributor Author

tie commented Sep 14, 2024

etc/pgbouncer.service

And from what I understand, this file is used for reference/documentation purposes only. That is, it is installed under docdir and distributions ship their own service definitions, e.g.

@tie tie force-pushed the notify-reloading branch 2 times, most recently from 4ed0d00 to f0630a6 Compare September 14, 2024 04:55
@JelteF
Copy link
Member

JelteF commented Sep 14, 2024

I think the sd_notifyf call should be fine to merge. Assuming that it indeed does not break older systemd setups.

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.

@JelteF
Copy link
Member

JelteF commented Sep 14, 2024

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.

@tie tie force-pushed the notify-reloading branch 2 times, most recently from 23d8874 to 339ca6e Compare September 15, 2024 09:53
@tie
Copy link
Contributor Author

tie commented Sep 15, 2024

@JelteF, rebased and inverted the comment. I’ve also noticed that I’ve wrongly assumed that get_cached_time/get_time_usec from libusual uses CLOCK_MONOTONIC that is required for proper reload synchronization with systemd (currently get_time_usec uses gettimeofday. I’ve added explicit clock_gettime call in this PR.

Copy link
Member

@eulerto eulerto left a 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
Copy link
Member

@eulerto eulerto left a comment

Choose a reason for hiding this comment

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

LGTM

@eulerto eulerto merged commit c1d8f4d into pgbouncer:master Sep 16, 2024
7 of 10 checks passed
@tie tie deleted the notify-reloading branch September 17, 2024 09:50
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.

3 participants