Delay pgbouncer preStop shutdown to avoid dropped connections during …

[bramhanandlingala]

Description

pgbouncer's preStop hook was signaling shutdown immediately with killall -INT pgbouncer, which meant the pod could exit before Kubernetes finished removing it from Service routing. Since pgbouncer exits as soon as its in-flight queries drain — often in well under a second for a lightly loaded pod — new connections kept getting routed to a pod that had already shut down, causing the dropped connections during rolling restarts described in #69571.

The fix adds a short sleep before signaling shutdown, so Kubernetes has time to mark the pod as Terminating and propagate that to Service endpoints while pgbouncer is still accepting connections. Only after that does it send the interrupt signal to drain and stop pgbouncer. The updated command is sleep 10 && killall -INT pgbouncer && sleep 20, and the same string was updated consistently across values.yaml, values.schema.json, and the existing test in test_container_lifecycle.py that asserts on it.

related: #69571

[Miretpl]

Suggested change

	"exec": {"command": ["/bin/sh", "-c", "sleep 10 && killall -INT pgbouncer && sleep 20"]}
	"exec": {"command": ["/bin/sh", "-c", "sleep 10 && killall -INT pgbouncer"]}

I think that it doesn't do anything really (unless I'm missing something).

[Miretpl]

Suggested change

	"sleep 10 && killall -INT pgbouncer && sleep 20"
	"sleep 10 && killall -INT pgbouncer"

[Miretpl]

Suggested change

	# Sleep first so the pod is marked Terminating and removed from
	# Service endpoints before pgbouncer stops accepting connections,
	# then gracefully drain existing queries.
	# Sleep first to give time to Kubernetes for removing Service endpoints
	# before PgBouncer stops accepting and draining existing connections.

A bit shorter version

[Miretpl]

Suggested change

	command: ["/bin/sh", "-c", "sleep 10 && killall -INT pgbouncer && sleep 20"]
	command: ["/bin/sh", "-c", "sleep 10 && killall -INT pgbouncer"]

[bramhanandlingala]

Hi @Miretpl

Thanks for the review — updated the comment wording per your suggestion.

On && sleep 20, I'd like to keep it. killall -INT pgbouncer returns as soon as the signal is sent, not once pgbouncer finishes draining — so without a delay after it, Kubernetes immediately sends SIGTERM next. Per pgbouncer's docs, SIGTERM arriving while a SIGINT-triggered shutdown is still in progress escalates to an immediate/hard shutdown, killing connections that hadn't finished draining yet — close to the exact bug this PR fixes.

The sleep 20 is what gives pgbouncer time to finish gracefully before that second signal can reach it.

I'd rather keep it as-is, and I'm happy to add a comment explaining this. Let me know if there's something
pgbouncer/Kubernetes-specific I'm missing.

[Miretpl]

I've dug deeper into this, and I think that the analysis is a bit off, as PgBouncer exits right after the signal, because the signal is SIGINT and PgBouncer closed all its connections to the database quickly enough that it seems instantaneous. As far as I looked at the PgBouncer codebase, since 1.23.0, there are two distinct behaviours of shutting down PgBouncer, and both are good depending on the case:

1. SIGTERM - it waits for closing all of the application connections (Airflow) to PgBouncer and shuts down
2. SIGINT - it closes all PgBouncer connections to the database and shuts down

I would say that the resolution to the issue is removing the preStop condition and making the transition handled by the load balancer and client instead of trying to synchronise it with sleeps, which may vary vastly depending on the environment and may be hard to adjust properly. By removing preStop, we will have something like:

1. Kubernetes deploys a new PgBouncer pod, which is in a ready state
2. Kubernetes sends SIGTERM to the old PgBouncer pod, which awaits application connections to close
3. During that time, EndpointSlice has information that this pod is ready: false and termination: true, which should make load balancers not direct traffic to it
4. 'if termination grace period will be high enough' EndpointSlice will have information serving: false when the readiness probe starts to fail
5. Two possible scenarios at the end:
   1. All Airflow connections were closed, and PgBouncer exits itself
   2. TerminationGracePeriod passes, and Kubernetes kills the old PgBouncer pod

If I'm missing something, let me know.

It would be good to test it in a real environment with some reasonable amount of executions (unfortunately, I will not be able to do that within a week or two probably). @bramhanandlingala @andrew-stein-sp would you be able to look at it?

[andrew-stein-sp]

@Miretpl That's an interesting theory...and simple. I'll test it on Monday and let you know.

[bramhanandlingala]

@Miretpl You're right, and thanks for digging into the actual pgbouncer changelog — I was going off older pre-1.23 signal semantics, which is backwards for the 1.23.1 we're pinned to. Makes sense now why SIGINT exits so fast. I'll wait for @andrew-stein-sp test before touching the code further — happy to rework the PR to drop the custom preStop and rely on SIGTERM + terminationGracePeriodSeconds once that's confirmed.