Don't recycle connections on RELOAD if TLS config is unchanged by cosgroveb · Pull Request #1157 · pgbouncer/pgbouncer

cosgroveb · 2024-09-03T09:48:50Z

This addresses #1145.

We run pgbouncers client-side that connect to 80+ databases. Separately we are addressing this bottleneck by running more pgbouncers client-side and striping them but in order to eliminate the unnecessary queuing that this kind of aggressive connection recycling can cause I propose taking a more judicious approach.

Essentially, I propose:

If going from TLS disabled => enabled, recycle.
If changing sslmode, recycle.
If changing TLS config, recycle.
Otherwise do nothing.

It's kind of gross that I have to inspect each member of the lengthy TLS config struct. Comments in-line.

cosgroveb · 2024-09-03T10:32:13Z

3 out of 4 of my new tests fail in CI under a --without-opensslbuild :|

FAILED test/test_operations.py::test_servers_disconnect_when_changing_tls_config - AssertionError
FAILED test/test_operations.py::test_servers_disconnect_when_enabling_ssl - AssertionError
======= 3 failed, 144 passed, 9 skipped, 9 warnings in 96.52s (0:01:36) ========

Maybe I'm missing a marker/annotation. 👀

cosgroveb · 2024-09-03T10:46:33Z

Okay, moved my tests into test_ssl so they only run in the right builds and addressed the uncrustify build.

JelteF

Overall looks like a good improvement. Currently we indeed recycle connections way too aggressively when TLS is enabled.

In pgbouncer, we'd like to be able to compare TLS configs for equality to avoid over-aggressive connection recycling. Since knowledge about the structure for these configs live here it seems best to implement that here. Original PR: pgbouncer/pgbouncer#1157

…nects on reload

…-> test_ssl so they are only run with SSL tests in CI

…y build

…ol marking dirty' tls specs

… is checking out a connection and also at the full_maint_period

…ontains block

…ess agressive RELOAD connection recycling

…csp_mem

…air is not checked

In pgbouncer, we'd like to be able to compare TLS configs for equality to avoid over-aggressive connection recycling. Since knowledge about the structure for these configs live here it seems best to implement that here. Original PR: pgbouncer/pgbouncer#1157

… libusual

cosgroveb · 2024-09-06T13:17:52Z

Hey @JelteF I think I've addressed any outstanding feedback. I did force push the branch on Wednesday to rewrite my git ùser.email`. Sorry if that was bad form or made reviewing the final state of this harder.

I presume we need to wait for my outstanding libusual PR to be reviewed/merged to get this pull merged? Any tips on getting the libusual PR reviewed?

Thanks for being so responsive and helpful in getting this operational bottleneck in pgbouncer solved.

In pgbouncer, we'd like to be able to compare TLS configs for equality to avoid over-aggressive connection recycling. Since knowledge about the structure for these configs live here it seems best to implement that here. Original PR: pgbouncer/pgbouncer#1157

JelteF · 2024-09-07T22:51:00Z

I presume we need to wait for my outstanding libusual/libusual#57 to be reviewed/merged to get this pull merged? Any tips on getting the libusual PR reviewed?

All PgBouncer committers also have commit access on libusual (including me). So this should be fine. But yeah we should merge that one first.

In pgbouncer, we'd like to be able to compare TLS configs for equality to avoid over-aggressive connection recycling. Since knowledge about the structure for these configs live here it seems best to implement that here. Original PR: pgbouncer/pgbouncer#1157

cosgroveb · 2024-09-08T20:21:23Z

@JelteF I've branched a cleanup from this to remove strings_equal() and use the libusual library function as discussed there and will PR that as soon as the libusual PR and this one are merged.

JelteF · 2024-09-08T21:10:39Z

CI is reporting a failure on one of the newly added tests, looks like it's flaky since only one of the linux builds is hitting the issue.

In pgbouncer, we'd like to be able to compare TLS configs for equality to avoid over-aggressive connection recycling. Since knowledge about the structure for these configs live here it seems best to implement that here. Original PR: pgbouncer/pgbouncer#1157

cosgroveb · 2024-09-08T22:33:51Z

I haven't had success with any specs at all with valgrind enabled locally today to repro but will give it another try tonight

cosgroveb · 2024-09-09T18:26:31Z

Okay @JelteF these specs were genuinely flaky due to timing issues between full_maint_period refreshes and clients checking out a connection. This spec was one that didn't sleep for full_maint_period and didn't expect that style of refresh from the logs but got one:

2024-09-08 18:04:35.379 UTC [12865] LOG S-0x53ccd50: pTxnPool/bouncer@127.0.0.1:10201 closing because: database configuration changed (age=0s)

This isn't incorrect behavior. Updating the expectation to avoid this: e27c774

…be done by operators

`full_maint_period` refreshes clients checking out a connection. This spec was one that didn't sleep for `full_maint_period` and didn't expect that style of refresh from the logs but got one: ```2024-09-08 18:04:35.379 UTC [12865] LOG S-0x53ccd50: pTxnPool/bouncer@127.0.0.1:10201 closing because: database configuration changed (age=0s) ``` This isn't incorrect behavior. This commit updates the expectation to avoid this issue.

cosgroveb · 2024-09-10T17:22:52Z

Any further changes needed @JelteF ?

JelteF

LGTM now

cosgroveb commented Sep 3, 2024

View reviewed changes

Comment thread src/sbuf.c Outdated

cosgroveb commented Sep 3, 2024

View reviewed changes

Comment thread src/sbuf.c Outdated

cosgroveb force-pushed the skip_disconnect_if_tls_config_unchanged branch from 2d10623 to 73adecd Compare September 3, 2024 11:05

cosgroveb commented Sep 3, 2024

View reviewed changes

Comment thread test/test_ssl.py

jcoleman reviewed Sep 3, 2024

View reviewed changes

Comment thread src/sbuf.c Outdated

Comment thread src/sbuf.c Outdated

Comment thread test/test_ssl.py Outdated

JelteF requested changes Sep 3, 2024

View reviewed changes

Comment thread src/sbuf.c Outdated

Comment thread src/sbuf.c Outdated

Comment thread src/sbuf.c Outdated

Comment thread src/sbuf.c Outdated

Comment thread src/sbuf.c Outdated

Comment thread test/test_ssl.py Outdated

Comment thread test/test_ssl.py

cosgroveb force-pushed the skip_disconnect_if_tls_config_unchanged branch 2 times, most recently from 9e5aa5a to 129a01b Compare September 4, 2024 14:28

cosgroveb mentioned this pull request Sep 4, 2024

Add function to compare TLS configs for equality libusual/libusual#57

Merged

cosgroveb added 17 commits September 4, 2024 22:36

Add test that demonstrates current behavior with unconditional discon…

dda7507

…nects on reload

Skip tagging pools dirty if the TLS config has not changed

0bda96d

Do not skip marking pools dirty when going from no TLS => TLS enabled

73248df

Do not skip marking pools dirty when server sslmode changes

52e6067

Move RELOAD/tls config connection refresh tests from test_operations …

9ee36a5

…-> test_ssl so they are only run with SSL tests in CI

Clean up connection recycling check code and tests based on uncrustif…

0228e94

…y build

Clean up struct access syntax in 'tls_config_changed'

c1f61ae

Add assertions to server-side connection counts when testing 'skip po…

d36b914

…ol marking dirty' tls specs

Prove that connections are recycled on TLS changes both when a client…

375af2e

… is checking out a connection and also at the full_maint_period

Fix happy-path tls disconnect-on-reload test moving RELOAD into log_c…

f0a9c7a

…ontains block

Fix reload SSL test format-check failures

0db9ff6

Update docs to note new potential pitfall around *_tls_ca_file with l…

f3fa2de

…ess agressive RELOAD connection recycling

Use memcmp() to properly compare tls_config->ca_mem and tls_config->o…

76591bc

…csp_mem

Avoid double negative in determination of reconnect-needed state

ce9ce2b

Fix name of function that determines if tls config changed

559f588

Fix assertions in tests for less-aggressive connection refresh on reload

be114a3

Added comment to tl_config_unchanged to document why tls_config->keyp…

a9d1859

…air is not checked

cosgroveb force-pushed the skip_disconnect_if_tls_config_unchanged branch from 1c373c5 to a9d1859 Compare September 4, 2024 22:37

cosgroveb force-pushed the skip_disconnect_if_tls_config_unchanged branch 2 times, most recently from dabc457 to 60e8d71 Compare September 4, 2024 23:56

Move tls config equality check and generic string support function to…

58b77a5

… libusual

cosgroveb force-pushed the skip_disconnect_if_tls_config_unchanged branch from 60e8d71 to 58b77a5 Compare September 5, 2024 20:31

JelteF reviewed Sep 7, 2024

View reviewed changes

Comment thread doc/config.md Outdated

cosgroveb added 5 commits September 9, 2024 18:12

Restore strings_equal and update usage of tls_config_equal

c95365a

Clarify docs on when and why RECONNECT is triggered and when it must …

4c14c39

…be done by operators

Use latest libusual

fb4db51

Fix linting failure

3b76c63

cosgroveb force-pushed the skip_disconnect_if_tls_config_unchanged branch from dfd74cb to 3b76c63 Compare September 9, 2024 23:13

Merge branch 'master' into skip_disconnect_if_tls_config_unchanged

041bd71

JelteF approved these changes Sep 14, 2024

View reviewed changes

JelteF enabled auto-merge (squash) September 14, 2024 11:06

JelteF merged commit b0c5d98 into pgbouncer:master Sep 14, 2024

cosgroveb deleted the skip_disconnect_if_tls_config_unchanged branch October 19, 2024 14:11

Uh oh!

Conversation

cosgroveb commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cosgroveb commented Sep 3, 2024

Uh oh!

cosgroveb commented Sep 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JelteF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cosgroveb commented Sep 6, 2024

Uh oh!

JelteF commented Sep 7, 2024

Uh oh!

Uh oh!

cosgroveb commented Sep 8, 2024

Uh oh!

JelteF commented Sep 8, 2024

Uh oh!

cosgroveb commented Sep 8, 2024

Uh oh!

cosgroveb commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cosgroveb commented Sep 10, 2024

Uh oh!

JelteF left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cosgroveb commented Sep 3, 2024 •

edited

Loading

cosgroveb commented Sep 9, 2024 •

edited

Loading