From a93a653b354ffbc0544a61a3e8f9cb2a38044f00 Mon Sep 17 00:00:00 2001 From: Sri Roopa Ramesh Babu Date: Tue, 16 Jun 2026 15:27:45 -0400 Subject: [PATCH] Add preStop lifecycle hook to postgres pod to prevent crash-loop after unclean shutdown --- internal/controller/postgres/assets_test.go | 14 +++++ internal/controller/postgres/deployment.go | 16 +++++- internal/controller/utils/constants.go | 11 ++++ internal/controller/utils/utils.go | 6 +- .../controller/utils/utils_comparison_test.go | 55 +++++++++++++++++++ test/e2e/all_features_test.go | 15 +++++ 6 files changed, 113 insertions(+), 4 deletions(-) diff --git a/internal/controller/postgres/assets_test.go b/internal/controller/postgres/assets_test.go index a26866cd5..c48065037 100644 --- a/internal/controller/postgres/assets_test.go +++ b/internal/controller/postgres/assets_test.go @@ -1,6 +1,7 @@ package postgres import ( + "fmt" "path" "strconv" @@ -87,6 +88,19 @@ var _ = Describe("App postgres server assets", func() { Expect(dep.Spec.Selector.MatchLabels).To(Equal(utils.GeneratePostgresSelectorLabels())) Expect(dep.Spec.RevisionHistoryLimit).To(Equal(&revisionHistoryLimit)) Expect(dep.Spec.Replicas).To(Equal(&replicas)) + // Verify preStop lifecycle hook for clean postgres shutdown + Expect(dep.Spec.Template.Spec.Containers[0].Lifecycle).NotTo(BeNil()) + Expect(dep.Spec.Template.Spec.Containers[0].Lifecycle.PreStop).NotTo(BeNil()) + Expect(dep.Spec.Template.Spec.Containers[0].Lifecycle.PreStop.Exec).NotTo(BeNil()) + expectedPreStopCommand := fmt.Sprintf("pg_ctl stop -D %s -m fast -w -t %d", utils.PostgresUserDataDir, utils.PostgresShutdownTimeoutSeconds) + Expect(dep.Spec.Template.Spec.Containers[0].Lifecycle.PreStop.Exec.Command).To(Equal([]string{ + "/bin/sh", "-c", + expectedPreStopCommand, + })) + // Verify terminationGracePeriodSeconds gives postgres time to shut down + Expect(dep.Spec.Template.Spec.TerminationGracePeriodSeconds).NotTo(BeNil()) + Expect(*dep.Spec.Template.Spec.TerminationGracePeriodSeconds).To(Equal(utils.PostgresTerminationGracePeriodSeconds)) + Expect(dep.Spec.Template.Spec.Containers[0].VolumeMounts).To(Equal([]corev1.VolumeMount{ { Name: "secret-" + utils.PostgresCertsSecretName, diff --git a/internal/controller/postgres/deployment.go b/internal/controller/postgres/deployment.go index fcc2e5351..79dbb58ea 100644 --- a/internal/controller/postgres/deployment.go +++ b/internal/controller/postgres/deployment.go @@ -2,6 +2,7 @@ package postgres import ( "context" + "fmt" "path" "strconv" @@ -234,10 +235,21 @@ func GeneratePostgresDeployment(r reconciler.Reconciler, ctx context.Context, cr Value: strconv.Itoa(cr.Spec.OLSConfig.ConversationCache.Postgres.MaxConnections), }, }, + Lifecycle: &corev1.Lifecycle{ + PreStop: &corev1.LifecycleHandler{ + Exec: &corev1.ExecAction{ + Command: []string{ + "/bin/sh", "-c", + fmt.Sprintf("pg_ctl stop -D %s -m fast -w -t %d", utils.PostgresUserDataDir, utils.PostgresShutdownTimeoutSeconds), + }, + }, + }, + }, }, }, - Volumes: volumes, - ServiceAccountName: utils.PostgreServiceAccountName, + Volumes: volumes, + ServiceAccountName: utils.PostgreServiceAccountName, + TerminationGracePeriodSeconds: &[]int64{utils.PostgresTerminationGracePeriodSeconds}[0], }, }, RevisionHistoryLimit: &revisionHistoryLimit, diff --git a/internal/controller/utils/constants.go b/internal/controller/utils/constants.go index bde82ded1..bc22ee974 100644 --- a/internal/controller/utils/constants.go +++ b/internal/controller/utils/constants.go @@ -330,6 +330,17 @@ ssl_ca_file = '/etc/certs/cm-olspostgresca/service-ca.crt' AlertsAdapterAlertmanagerURLEnvVar = "ALERTMANAGER_URL" // 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. + PostgresTerminationGracePeriodSeconds = int64(60) + + // PostgresShutdownTimeoutSeconds is the timeout for pg_ctl stop command. + // This must be less than PostgresTerminationGracePeriodSeconds to complete before SIGKILL. + PostgresShutdownTimeoutSeconds = 55 + + // 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" // PostgresPVCName is the name of the PVC for the OLS Postgres server PostgresPVCName = "lightspeed-postgres-pvc" diff --git a/internal/controller/utils/utils.go b/internal/controller/utils/utils.go index 804d4d0df..b644c4ea9 100644 --- a/internal/controller/utils/utils.go +++ b/internal/controller/utils/utils.go @@ -194,7 +194,8 @@ func DeploymentSpecEqual(a, b *appsv1.DeploymentSpec, compareInitContainers bool !apiequality.Semantic.DeepEqual(a.Strategy, b.Strategy) || // check strategy !PodVolumeEqual(a.Template.Spec.Volumes, b.Template.Spec.Volumes) || // check volumes *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 return false } @@ -239,7 +240,8 @@ func ContainerSpecEqual(a, b *corev1.Container) bool { a.ImagePullPolicy == b.ImagePullPolicy && // check image pull policy ProbeEqual(a.LivenessProbe, b.LivenessProbe) && // check liveness probe ProbeEqual(a.ReadinessProbe, b.ReadinessProbe) && // check readiness probe - ProbeEqual(a.StartupProbe, b.StartupProbe)) // check startup probe + ProbeEqual(a.StartupProbe, b.StartupProbe) && // check startup probe + apiequality.Semantic.DeepEqual(a.Lifecycle, b.Lifecycle)) // check lifecycle hooks } // EnvEqual compares two EnvVar slices ignoring order diff --git a/internal/controller/utils/utils_comparison_test.go b/internal/controller/utils/utils_comparison_test.go index f1130d5f6..e0df314e8 100644 --- a/internal/controller/utils/utils_comparison_test.go +++ b/internal/controller/utils/utils_comparison_test.go @@ -127,6 +127,46 @@ var _ = Describe("Container Comparison", func() { It("should handle empty container lists", func() { Expect(ContainersEqual([]corev1.Container{}, []corev1.Container{})).To(BeTrue()) }) + + It("should return false when lifecycle hooks differ", func() { + containers1 := []corev1.Container{ + { + Name: "app", + Image: "myapp:v1", + Lifecycle: &corev1.Lifecycle{ + PreStop: &corev1.LifecycleHandler{ + Exec: &corev1.ExecAction{ + Command: []string{"/bin/sh", "-c", "pg_ctl stop"}, + }, + }, + }, + }, + } + containers2 := []corev1.Container{ + { + Name: "app", + Image: "myapp:v1", + }, + } + Expect(ContainersEqual(containers1, containers2)).To(BeFalse()) + }) + + It("should return true when lifecycle hooks are equal", func() { + lifecycle := &corev1.Lifecycle{ + PreStop: &corev1.LifecycleHandler{ + Exec: &corev1.ExecAction{ + Command: []string{"/bin/sh", "-c", "pg_ctl stop"}, + }, + }, + } + containers1 := []corev1.Container{ + {Name: "app", Image: "myapp:v1", Lifecycle: lifecycle}, + } + containers2 := []corev1.Container{ + {Name: "app", Image: "myapp:v1", Lifecycle: lifecycle.DeepCopy()}, + } + Expect(ContainersEqual(containers1, containers2)).To(BeTrue()) + }) }) }) @@ -270,6 +310,21 @@ var _ = Describe("Resource Comparison Functions", func() { Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) }) + + It("should return false when terminationGracePeriodSeconds differs", func() { + gracePeriod := int64(60) + deployment2.Template.Spec.TerminationGracePeriodSeconds = &gracePeriod + + Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) + }) + + It("should return true when terminationGracePeriodSeconds is equal", func() { + gracePeriod := int64(60) + deployment1.Template.Spec.TerminationGracePeriodSeconds = &gracePeriod + deployment2.Template.Spec.TerminationGracePeriodSeconds = &gracePeriod + + Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeTrue()) + }) }) Describe("ServiceEqual", func() { diff --git a/test/e2e/all_features_test.go b/test/e2e/all_features_test.go index 5b97d7d71..291145f86 100644 --- a/test/e2e/all_features_test.go +++ b/test/e2e/all_features_test.go @@ -720,6 +720,21 @@ var _ = Describe("All Features Enabled", Ordered, Label("AllFeatures"), func() { } Expect(foundSharedBuffers).To(BeTrue(), "POSTGRESQL_SHARED_BUFFERS should be set to 512MB") Expect(foundMaxConnections).To(BeTrue(), "POSTGRESQL_MAX_CONNECTIONS should be set to 3000") + + By("Verifying postgres deployment has preStop lifecycle hook") + postgresContainer := postgresDeployment.Spec.Template.Spec.Containers[0] + Expect(postgresContainer.Lifecycle).NotTo(BeNil(), "postgres container should have a lifecycle configuration") + Expect(postgresContainer.Lifecycle.PreStop).NotTo(BeNil(), "postgres container should have a preStop hook") + Expect(postgresContainer.Lifecycle.PreStop.Exec).NotTo(BeNil(), "preStop hook should use exec") + expectedPreStopCommand := fmt.Sprintf("pg_ctl stop -D %s -m fast -w -t %d", utils.PostgresUserDataDir, utils.PostgresShutdownTimeoutSeconds) + Expect(postgresContainer.Lifecycle.PreStop.Exec.Command).To(Equal([]string{"/bin/sh", "-c", expectedPreStopCommand}), + "preStop hook should run pg_ctl stop for clean shutdown") + + By("Verifying postgres deployment has terminationGracePeriodSeconds") + Expect(postgresDeployment.Spec.Template.Spec.TerminationGracePeriodSeconds).NotTo(BeNil(), + "postgres pod should have terminationGracePeriodSeconds set") + Expect(*postgresDeployment.Spec.Template.Spec.TerminationGracePeriodSeconds).To(Equal(utils.PostgresTerminationGracePeriodSeconds), + "postgres pod should have correct termination grace period") }) // Test 10: Resource Limits Validation