OLS-3192 Add preStop lifecycle hook to postgres pod to prevent crash-loop afte… by sriroopar · Pull Request #1726 · openshift/lightspeed-operator

sriroopar · 2026-06-16T19:33:37Z

…r unclean shutdown

Description

Type of change

Related Tickets & Documents

Related Issue #
Closes #

Checklist before requesting a review

I have performed a self-review of my code.
PR has passed all pre-merge test jobs.
If it is a core feature, I have added thorough tests.

Testing

Please provide detailed steps to perform tests related to this code change.
How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

Release Notes

Bug Fixes
- Improved PostgreSQL shutdown by adding a graceful pre-stop stop command and enforcing an explicit pod termination grace period (60 seconds) for cleaner termination.
Tests
- Expanded automated checks to validate PostgreSQL lifecycle pre-stop configuration, shutdown/termination settings, and deployment comparison behavior for lifecycle hooks and termination grace period.

coderabbitai · 2026-06-16T19:33:59Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 885cc0e8-9210-4dc1-b67b-780ae67f41b7

📥 Commits

Reviewing files that changed from the base of the PR and between 9359380 and b67cf1d.

📒 Files selected for processing (6)

internal/controller/postgres/assets_test.go
internal/controller/postgres/deployment.go
internal/controller/utils/constants.go
internal/controller/utils/utils.go
internal/controller/utils/utils_comparison_test.go
test/e2e/all_features_test.go

🚧 Files skipped from review as they are similar to previous changes (5)

internal/controller/postgres/assets_test.go
internal/controller/postgres/deployment.go
internal/controller/utils/utils_comparison_test.go
test/e2e/all_features_test.go
internal/controller/utils/utils.go

📝 Walkthrough

Walkthrough

Adds PostgreSQL shutdown lifecycle wiring with a preStop pg_ctl stop hook and an explicit termination grace period. Updates deployment comparison helpers to include the new fields, and extends unit and e2e coverage.

Changes

Postgres graceful shutdown

Layer / File(s)	Summary
Shutdown constants `internal/controller/utils/constants.go`	Adds PostgreSQL constants for termination grace period, shutdown timeout, and the user data directory path.
Deployment lifecycle wiring `internal/controller/postgres/deployment.go`	Sets the Postgres container `Lifecycle.PreStop` exec command and pod `TerminationGracePeriodSeconds` in the generated deployment.
Comparison and validation `internal/controller/utils/utils.go`, `internal/controller/utils/utils_comparison_test.go`, `internal/controller/postgres/assets_test.go`, `test/e2e/all_features_test.go`	Extends deployment/container equality checks for `TerminationGracePeriodSeconds` and `Lifecycle`, and adds unit and e2e assertions for the generated shutdown behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding a preStop hook to the PostgreSQL pod to avoid crash-loops after unclean shutdowns.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-06-16T19:34:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bparees for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sriroopar · 2026-06-23T17:09:28Z

/retest

sriroopar · 2026-06-23T18:41:08Z

/retest

sriroopar

Adversarial Review

The preStop hook approach is sound — pg_ctl stop -m fast -w is the right tool for clean Postgres shutdown in Kubernetes. Here are concerns to address:

CI Note

The ci/prow/bundle-e2e-4-21 failure is unrelated to this PR. It's a pre-existing flaky proxy test (proxy_test.go:162) — the Squid PVC isn't cleaned up between retries (squid-volume-claim already exists), causing all retry attempts to fail. This eventually hits the 2-hour Prow timeout. A /retest should pass.

sriroopar · 2026-07-09T17:54:15Z

+										Command: []string{
+											"/bin/sh", "-c",
+											fmt.Sprintf("pg_ctl stop -D %s -m fast -w -t %d", utils.PostgresUserDataDir, utils.PostgresShutdownTimeoutSeconds),
+										},


Allocation for a pointer to constant: &[]int64{utils.PostgresTerminationGracePeriodSeconds}[0] allocates a slice just to get a *int64. This is a known Go pattern but it's fragile and non-obvious.

Consider a simple helper or inline variable:

terminationGracePeriod := utils.PostgresTerminationGracePeriodSeconds // ... TerminationGracePeriodSeconds: &terminationGracePeriod,

Or use k8s.io/utils/ptr.To(utils.PostgresTerminationGracePeriodSeconds) which is already a transitive dependency.

sriroopar · 2026-07-09T17:54:15Z

+	// PostgresShutdownTimeoutSeconds is the timeout for pg_ctl stop command.
+	// This must be less than PostgresTerminationGracePeriodSeconds to complete before SIGKILL.
+	PostgresShutdownTimeoutSeconds = 55
+


Type inconsistency: PostgresTerminationGracePeriodSeconds is int64(60) but PostgresShutdownTimeoutSeconds is untyped 55 (defaults to int). These two constants have a mathematical relationship (grace period must exceed shutdown timeout), but the type mismatch means you can't directly compute the delta without a cast.

Consider making both int64, or at minimum adding a compile-time assertion:

PostgresShutdownTimeoutSeconds = int64(55)

sriroopar · 2026-07-09T17:54:15Z

+
+	// PostgresUserDataDir is the PostgreSQL data directory path (relative to PostgresDataVolumeMountPath).
+	// PostgreSQL's initdb creates this structure: PGDATA/data/userdata
+	PostgresUserDataDir = "/var/lib/pgsql/data/userdata"


Path duplication risk: PostgresUserDataDir = "/var/lib/pgsql/data/userdata" hardcodes a path whose prefix is already defined as PostgresDataVolumeMountPath = "/var/lib/pgsql". If someone changes the mount path, this constant won't track.

Consider deriving it:

PostgresUserDataDir = PostgresDataVolumeMountPath + "/data/userdata"

Also — is data/userdata always the correct PGDATA subdirectory for the Red Hat postgres image? This path is an internal implementation detail of the container image's initdb. If the image ever changes its PGDATA layout, this will silently break (pg_ctl will report 'not a database directory' to stderr, but the preStop hook has no error propagation).

sriroopar · 2026-07-09T17:54:16Z

@@ -234,10 +235,21 @@ func GeneratePostgresDeployment(r reconciler.Reconciler, ctx context.Context, cr
 									Value: strconv.Itoa(cr.Spec.OLSConfig.ConversationCache.Postgres.MaxConnections),
 								},


Silent failure mode: If pg_ctl is not in $PATH inside the container (different base image, stripped image, etc.), or if the data directory doesn't exist yet (fresh pod that crashed before initdb), this preStop hook fails silently. Kubernetes will still send SIGTERM after the hook exits with a non-zero code, but you lose the clean-shutdown guarantee.

Consider adding a guard:

/bin/sh -c 'command -v pg_ctl >/dev/null 2>&1 && pg_ctl stop -D ... -m fast -w -t 55 || true'

The || true ensures the hook doesn't block termination if pg_ctl isn't available.

sriroopar · 2026-07-09T17:54:16Z

 	// AlertsAdapterComponentLabel is the app.kubernetes.io/component label value for alerts adapter resources
 	AlertsAdapterComponentLabel = "alerts-adapter"
+	// PostgresTerminationGracePeriodSeconds is the grace period for postgres pod termination.
+	// This must be greater than PostgresShutdownTimeoutSeconds to allow pg_ctl to complete.


5-second margin may be tight: The 5-second gap between PostgresShutdownTimeoutSeconds (55) and PostgresTerminationGracePeriodSeconds (60) needs to cover:

Kubelet receives termination signal

Kubelet invokes the preStop hook shell (/bin/sh -c ...)

pg_ctl starts and begins shutdown

Steps 1-3 happen before the 55-second pg_ctl timer starts. If these steps take >5 seconds (which can happen under node pressure), pg_ctl will be SIGKILLed mid-shutdown — exactly the crash-loop scenario this PR aims to prevent.

Consider either increasing the grace period to 90s (with 55s pg_ctl timeout) or reducing the pg_ctl timeout to 45s.

sriroopar · 2026-07-09T17:54:16Z

 		*a.Replicas != *b.Replicas || // check replicas
-		a.Template.Spec.ServiceAccountName != b.Template.Spec.ServiceAccountName { // check service account name
+		a.Template.Spec.ServiceAccountName != b.Template.Spec.ServiceAccountName || // check service account name
+		!apiequality.Semantic.DeepEqual(a.Template.Spec.TerminationGracePeriodSeconds, b.Template.Spec.TerminationGracePeriodSeconds) { // check termination grace period


Consistency note: The existing *a.Replicas != *b.Replicas on line 196 will panic if either Replicas pointer is nil. The new TerminationGracePeriodSeconds comparison correctly uses apiequality.Semantic.DeepEqual which is nil-safe. This is good — but worth noting the pre-existing nil-unsafety on Replicas as a follow-up.

…r unclean shutdown

- Increase termination grace period from 60s to 90s for adequate kubelet overhead - Make PostgresShutdownTimeoutSeconds int64 for type consistency - Derive PostgresUserDataDir from PostgresDataVolumeMountPath to avoid path duplication - Add || true guard to preStop command to prevent hook failure blocking termination - Replace &[]int64{...}[0] hack with local variable for pointer Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

openshift-ci · 2026-07-09T21:48:15Z

@sriroopar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/bundle-e2e-4-21	`b67cf1d`	link	true	`/test bundle-e2e-4-21`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci Bot requested review from blublinsky and joshuawilson June 16, 2026 19:34

sriroopar changed the title ~~Add preStop lifecycle hook to postgres pod to prevent crash-loop afte…~~ OLS-3192 Add preStop lifecycle hook to postgres pod to prevent crash-loop afte… Jun 24, 2026

sriroopar force-pushed the postgres-availability-fix branch from 9359380 to f3f335d Compare July 7, 2026 03:33

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2026

sriroopar force-pushed the postgres-availability-fix branch from f3f335d to a93a653 Compare July 7, 2026 03:33

openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2026

sriroopar commented Jul 9, 2026

View reviewed changes

sriroopar and others added 2 commits July 9, 2026 14:58

Add preStop lifecycle hook to postgres pod to prevent crash-loop afte…

6839499

…r unclean shutdown

sriroopar force-pushed the postgres-availability-fix branch from a93a653 to b67cf1d Compare July 9, 2026 18:59

		@@ -234,10 +235,21 @@ func GeneratePostgresDeployment(r reconciler.Reconciler, ctx context.Context, cr
		Value: strconv.Itoa(cr.Spec.OLSConfig.ConversationCache.Postgres.MaxConnections),
		},

Uh oh!

Conversation

sriroopar commented Jun 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

openshift-ci Bot commented Jun 16, 2026

Uh oh!

sriroopar commented Jun 23, 2026

Uh oh!

sriroopar commented Jun 23, 2026

Uh oh!

sriroopar left a comment

Choose a reason for hiding this comment

Adversarial Review

CI Note

Uh oh!

sriroopar Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sriroopar Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sriroopar Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sriroopar Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sriroopar Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

sriroopar Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sriroopar commented Jun 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading