K8SPG-1115: allow pausing pgbouncer connections on demand - #1719
Open
mayankshah1607 wants to merge 30 commits into
Open
K8SPG-1115: allow pausing pgbouncer connections on demand#1719mayankshah1607 wants to merge 30 commits into
mayankshah1607 wants to merge 30 commits into
Conversation
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an on-demand PgBouncer connection pause feature to the PostgreSQL operator by introducing a new paused flag in the proxy/PgBouncer spec, persisting pause intent across restarts via a marker file, and wiring operator logic to execute PAUSE/RESUME against PgBouncer instances.
Changes:
- Introduces
spec.proxy.pgbouncer.paused(upstream + Percona API) and propagates it through conversions/deepcopies and CRD schemas. - Adds pause/resume reconciliation in the PostgresCluster controller, including a
PGBouncerPausedstatus condition. - Adds a new
pgbouncer-startupbinary + init-container plumbing and config projections to re-apply pause state on PgBouncer restarts, with unit tests.
Reviewed changes
Copilot reviewed 23 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go | Deepcopy support for new Paused field in upstream PgBouncer spec. |
| pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go | Adds PGBouncerPaused condition type and PGBouncerPaused() helper. |
| pkg/apis/upstream.pgv2.percona.com/v1beta1/pgbouncer_types.go | Adds Paused *bool to upstream PGBouncerPodSpec. |
| pkg/apis/pgv2.percona.com/v2/zz_generated.deepcopy.go | Deepcopy support for new Paused field in Percona API PgBouncer spec. |
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go | Adds Paused to Percona PGBouncerSpec and maps it to Crunchy spec. |
| percona/controller/pgcluster/status.go | Ensures Percona status conditions reflect upstream PGBouncerPaused presence/removal. |
| internal/pgbouncer/startup/startup.go | Defines the shared “contract” constants for pause marker paths and admin env var. |
| internal/pgbouncer/reconcile.go | Adds pause marker ConfigMap key, admin password handling, and init/startup-probe wiring for PgBouncer pods. |
| internal/pgbouncer/reconcile_test.go | Adds unit tests for pause marker ConfigMap behavior, admin password generation, and pod probe/volumes/init container behavior. |
| internal/pgbouncer/config.go | Adds pause marker projection, admin user handling, and supports writing admin credentials to the auth file. |
| internal/pgbouncer/config_test.go | Adds unit tests for auth file admin user, ensureListed(), admin_users INI behavior, and pause marker projection. |
| internal/controller/runtime/pgbouncer/mock/admin_client.go | Generated mock for PgBouncer admin client interface (tests). |
| internal/controller/runtime/pgbouncer/gen.go | go:generate for mockery to generate the AdminClient mock. |
| internal/controller/runtime/pgbouncer/admin.go | Implements PgBouncer admin client used for issuing PAUSE/RESUME. |
| internal/controller/postgrescluster/pgbouncer.go | Adds pause/resume reconciliation logic and status condition management for PgBouncer. |
| internal/controller/postgrescluster/pgbouncer_test.go | Adds controller-level tests for pause/resume reconciliation via mocked admin client + fake client pods. |
| internal/controller/postgrescluster/controller.go | Injects newPGBouncerAdmin factory into reconciler for runtime/test wiring. |
| go.sum | Updates module sums for new dependencies (pq driver, mockery tooling, transitive deps). |
| go.mod | Adds github.com/lib/pq and mockery as a tool dependency plus new indirects. |
| deploy/cw-bundle.yaml | CRD bundle update to include paused schema field. |
| deploy/crd.yaml | CRD manifest update to include paused schema field. |
| deploy/bundle.yaml | CRD bundle update to include paused schema field. |
| config/crd/bases/upstream.pgv2.percona.com_postgresclusters.yaml | Upstream CRD base schema updated with paused. |
| config/crd/bases/pgv2.percona.com_perconapgclusters.yaml | Percona CRD base schema updated with paused. |
| cmd/pgbouncer-startup/main.go | New startup-probe binary that re-applies pause on container start when marker exists. |
| cmd/pgbouncer-startup/main_test.go | Unit tests for pause marker detection and retry/pause logic. |
| build/postgres-operator/init-entrypoint.sh | Installs the pgbouncer-startup binary into the shared crunchy bin volume. |
| build/postgres-operator/Dockerfile | Builds and ships the new pgbouncer-startup binary (CGO disabled). |
| build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml | Generated Percona CRD updated with paused schema field. |
Files not reviewed (3)
- internal/controller/runtime/pgbouncer/mock/admin_client.go: Generated file
- pkg/apis/pgv2.percona.com/v2/zz_generated.deepcopy.go: Generated file
- pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go: Generated file
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 28 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- internal/controller/runtime/pgbouncer/mock/admin_client.go: Generated file
- pkg/apis/pgv2.percona.com/v2/zz_generated.deepcopy.go: Generated file
- pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)
internal/controller/runtime/pgbouncer/admin.go:37
-
- Problem: The DSN format string does not include a password parameter, so the
passwordargument is ignored and PgBouncer admin connections will fail authentication.
- Problem: The DSN format string does not include a password parameter, so the
- Why it matters: Pause/resume (and the startup probe) depend on successfully connecting as the admin user.
- Fix: Include the password in the DSN parameters passed to
sql.Open.
dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=pgbouncer sslmode=require",
host, user, password)
internal/controller/runtime/pgbouncer/admin.go:38
-
- Problem:
NewAdminClienthas no way to specify the PgBouncer port, so it will always connect using lib/pq’s default (5432).
- Problem:
- Why it matters: PgBouncer’s listen port is configurable in the cluster spec (the Pod uses
*inCluster.Spec.Proxy.PGBouncer.Port), so pause/resume will fail when the port is not 5432. - Fix: Thread the configured port into the admin client (e.g., add a
port int32parameter and includeport=%din the DSN) and update callers (controller + pgbouncer-startup) to pass it.
func NewAdminClient(user, password, host string) (AdminClient, error) {
if user == "" {
return nil, errors.New("user is required")
}
if password == "" {
return nil, errors.New("password is required")
}
if host == "" {
return nil, errors.New("host is required")
}
dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=pgbouncer sslmode=require",
host, user, password)
db, err := sql.Open("postgres", dsn)
internal/controller/postgrescluster/pgbouncer.go:789
-
- Problem:
handlePGBouncerPausedereferencescluster.Spec.Proxy.PGBouncer.Replicaswithout a nil check, which can panic when replicas is unset.
- Problem:
- Why it matters: A panic in reconciliation can crash the operator and block further progress.
- Fix: Guard against nil replicas (consistent with other PgBouncer reconcile code in this file) before dereferencing.
if len(podList.Items) < int(*cluster.Spec.Proxy.PGBouncer.Replicas) {
return errors.Errorf("pgbouncer pods are not ready, expected %d, got %d", *cluster.Spec.Proxy.PGBouncer.Replicas, len(podList.Items))
}
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
mayankshah1607
marked this pull request as ready for review
August 4, 2026 09:15
mayankshah1607
requested review from
egegunes,
eleo007,
gkech,
jvpasinatto,
nmarukovich,
pooknull and
valmiranogueira
as code owners
August 4, 2026 09:15
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
egegunes
reviewed
Aug 5, 2026
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
egegunes
previously approved these changes
Aug 5, 2026
pooknull
requested changes
Aug 6, 2026
| } | ||
|
|
||
| func TestReconcilePause(t *testing.T) { | ||
| ctx := context.Background() |
Contributor
There was a problem hiding this comment.
Suggested change
| ctx := context.Background() | |
| ctx := t.Context() |
Comment on lines
+61
to
+62
| ctx, cancel := context.WithTimeout(context.Background(), pauseTimeout) | ||
| defer cancel() |
Contributor
There was a problem hiding this comment.
Can we use context from signal.NotifyContext as we do in cmd/pgbackrest/main.go instead of context.Background()? This would allow exit early on SIGTERM, preventing pod from being stuck for 30 seconds trying to terminate
| return nil, errors.New("host is required") | ||
| } | ||
|
|
||
| dsn := fmt.Sprintf("host=%s user=%s password=%s dbname=pgbouncer sslmode=require", |
Contributor
There was a problem hiding this comment.
I think we should add a port=%s and use .Spec.Proxy.PGBouncer.Port as value
Comment on lines
+810
to
+813
| password, ok := secret.Data[pgbouncer.AdminPasswordSecretKey] | ||
| if !ok { | ||
| return errors.New("pgbouncer admin password not found in secret") | ||
| } |
Contributor
There was a problem hiding this comment.
If proxy.PGBouncerEnabled() is false, then secret will be nil. We will get a panic here
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 53 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
cmd/pgbouncer-startup/main.go:26
- Problem: The startup log file is created with mode
0666(world-writable).
Why it matters: Overly-permissive file modes can allow unintended modification of logs inside the container.
Fix: Use a more restrictive mode like0644(or0600if only the current user should read it).
f, err := os.OpenFile(startup.LogAbsolutePath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0o666)
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
egegunes
previously approved these changes
Aug 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 53 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/controller/postgrescluster/pgbouncer.go:842
-
- Problem:
handlePGBouncerPauseexecutes PAUSE/RESUME using the controller reconcilectxwithout any additional timeout, so a hung TCP connect/query can block reconciliation indefinitely.
- Problem:
- Why it matters: A stuck reconcile can stall the controller and prevent other cluster updates from being processed.
- Fix: Wrap each PAUSE/RESUME call in a bounded
context.WithTimeout(e.g., 30s) and pass that derived context to the admin client call.
internal/controller/runtime/pgbouncer/admin.go:46
-
- Problem: The PgBouncer admin DSN is hard-coded to
sslmode=requireand does not provide any way to supply a client certificate/key.
- Problem: The PgBouncer admin DSN is hard-coded to
- Why it matters: If PgBouncer is configured to require verified client certificates (e.g.,
client_tls_sslmode=verify-ca), the operator/startup-probe admin connection will fail and pause/resume cannot work. - Fix: Extend
AdminClientOptionsto accept optional TLS parameters (sslrootcert/sslcert/sslkey) and include them in the DSN (or explicitly detect/guard against mTLS-required configs and surface a clear error).
pooknull
previously approved these changes
Aug 7, 2026
oksana-grishchenko
previously approved these changes
Aug 7, 2026
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
mayankshah1607
dismissed stale reviews from egegunes, pooknull, and oksana-grishchenko
via
August 7, 2026 11:28
9b93373
Collaborator
commit: 9b93373 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGE DESCRIPTION
Adds a new
spec.proxy.pgbouncer.pausedfield that allows pausing pgbouncer connections.The following additions were made to support this feature:
_crunchypgbounceradmin. The operator uses this to connect to the pgbouncer admin. ConfigMap and Secret were updated accordinglyAdminClientinterface that connects to pgbouncer pods viasql.DBto issuePAUSEandRESUME(no execs)PAUSEis not persisted across restarts. In order to have this new startup probe, an init container is also set on pgbouncer to install the needed binaries from the operatorCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability