K8SPG-708 replace ready/live probe http check with custom command, change pg entrypoint #1099

gkech · 2025-03-21T13:50:33Z

CHANGE DESCRIPTION

Problem:
We are changing the probes from direct requests to patroni's restful api to a custom command so that we can get more flexibility on what can be executed for the live/ready checks of the pg container. For now this is needed so that we can introduce a sleep forever feature. Related PR: percona/percona-docker#1148

We are also introducing a new entrypoint for the database container which is defined in the percona-docker repo for now.

Testing sleep forever

PPG image used: perconalab/percona-postgresql-operator:K8SPG-708-ppg17-postgres

Cause:
Short explanation of the root cause of the issue if applicable.

Solution:
Short explanation of the solution we are providing with this PR.

CHECKLIST

Jira

Is the Jira ticket created and referenced properly?
Does the Jira ticket have the proper statuses for documentation (Needs Doc) and QA (Needs QA)?
Does the Jira ticket link to the proper milestone (Fix Version field)?

Tests

Is an E2E test/test case added for the new feature/change?
Are unit tests added where appropriate?

Config/Logging/Testability

Are all needed new/changed options added to default YAML files?
Are all needed new/changed options added to the Helm Chart?
Did we add proper logging messages for operator actions?
Did we ensure compatibility with the previous version or cluster upgrade process?
Does the change support oldest and newest supported PG version?
Does the change support oldest and newest supported Kubernetes version?

gkech · 2025-03-24T10:22:50Z

internal/patroni/reconcile.go

+	container.LivenessProbe.Exec = &corev1.ExecAction{
+		Command: []string{"/usr/local/bin/postgres-liveness-check.sh"},


Maybe we want to do this change only for >2.7.0 🤔

done here: c377050

egegunes · 2025-03-25T14:59:43Z

internal/patroni/reconcile.go

-		Path:   "/readiness",
-		Port:   intstr.FromInt(int(*cluster.Spec.Patroni.Port)),
-		Scheme: corev1.URISchemeHTTPS,
+	container.ReadinessProbe.ProbeHandler = readinessProbe(cluster)


please add comments for your changes in upstream code

@egegunes have a look on this: 2088d85, and let me know if we are looking for something different

nmarukovich · 2025-03-26T13:38:33Z

e2e-tests/vars.sh

@@ -14,7 +14,7 @@ export IMAGE_BASE=${IMAGE_BASE:-"perconalab/percona-postgresql-operator"}
 export IMAGE=${IMAGE:-"${IMAGE_BASE}:${VERSION}"}
 export PG_VER="${PG_VER:-16}"
 export IMAGE_PGBOUNCER=${IMAGE_PGBOUNCER:-"${IMAGE_BASE}:main-ppg$PG_VER-pgbouncer"}
-export IMAGE_POSTGRESQL=${IMAGE_POSTGRESQL:-"${IMAGE_BASE}:main-ppg$PG_VER-postgres"}
+export IMAGE_POSTGRESQL=${IMAGE_POSTGRESQL:-"${IMAGE_BASE}:K8SPG-708-ppg$PG_VER-postgres"}
 export IMAGE_BACKREST=${IMAGE_BACKREST:-"${IMAGE_BASE}:main-ppg$PG_VER-pgbackrest"}


probably you will return this value to export IMAGE_POSTGRESQL=${IMAGE_POSTGRESQL:-"${IMAGE_BASE}:main-ppg$PG_VER-postgres"} .

I think that once we merge and generate new ppg images with the updated scripts, we will open a new PR to revert this. But for now I think it should stay as is, otherwise the tests will fail.

nmarukovich · 2025-03-26T16:16:16Z

internal/patroni/reconcile.go

+// the database container is considered "alive" beyond basic checks.
+// Introduced with K8SPG-708.
+func readinessProbe(cluster *v1beta1.PostgresCluster) corev1.ProbeHandler {
+	if cluster.CompareVersion("2.7.0") >= 0 {


My favorite comment. I like more to use like this:
Motivation:
Easy to refactor, depricated value should be under condition since it doesn't use in version 2.7.0 and above (and when we should change something we should change new behaviour, not old one)

func livenessProbe(cluster *v1beta1.PostgresCluster) corev1.ProbeHandler { if cluster.CompareVersion("2.7.0") < 0 { return corev1.ProbeHandler{ HTTPGet: &corev1.HTTPGetAction{ Path: "/liveness", Port: intstr.FromInt(int(*cluster.Spec.Patroni.Port)), Scheme: corev1.URISchemeHTTPS, }, } } return corev1.ProbeHandler{ Exec: &corev1.ExecAction{ Command: []string{"bash", "-c", "/usr/local/bin/postgres-liveness-check.sh"}, }, } return corev1.ProbeHandler{ HTTPGet: &corev1.HTTPGetAction{ Path: "/liveness", Port: intstr.FromInt(int(*cluster.Spec.Patroni.Port)), Scheme: corev1.URISchemeHTTPS, }, } }

The main reason I prefer the existing approach is because it has as "default" non-conditional behaviour the old implementation of the probes, while the new approach is conditioned to only the latest CR version. Since it is already a very small function that is also unit tested, it is very compact and refactoring it internally in any way we want in the future will not cause us any trouble. Usually the case you are mentioning could cause problems in larger functions with more complicated logic.

nmarukovich · 2025-03-26T16:25:23Z

internal/patroni/reconcile_test.go

+			cluster := new(v1beta1.PostgresCluster)
+			err := cluster.Default(context.Background(), nil)
+			assert.NilError(t, err)
+			cluster.Name = "some-such"


Do we have any agreement on how to name variables in tests, or does it not matter in this case?

this is upstream code that was just moved to a different place. if you notice the diff you will see where it was before.

gkech commented Mar 24, 2025

View reviewed changes

gkech marked this pull request as ready for review March 24, 2025 11:25

gkech requested review from hors, egegunes, pooknull, nmarukovich, jvpasinatto and eleo007 as code owners March 24, 2025 11:25

gkech force-pushed the K8SPG-708 branch from a863bb8 to 8f3c32c Compare March 24, 2025 14:10

egegunes requested changes Mar 25, 2025

View reviewed changes

gkech requested a review from egegunes March 26, 2025 08:02

egegunes previously approved these changes Mar 26, 2025

View reviewed changes

nmarukovich reviewed Mar 26, 2025

View reviewed changes

gkech requested a review from nmarukovich March 26, 2025 18:04

gkech dismissed egegunes’s stale review via 84a7588 March 28, 2025 15:58

gkech requested a review from egegunes March 28, 2025 15:58

egegunes added this to the 2.7.0 milestone Mar 31, 2025

gkech changed the title ~~K8SPG-708 replace ready/live probe http check with custom command~~ K8SPG-708 replace ready/live probe http check with custom command, change pg entrypoint Apr 1, 2025

egegunes previously approved these changes Apr 1, 2025

View reviewed changes

gkech added 9 commits April 3, 2025 09:50

K8SPG-708 replace ready/live probe http check with custom command

7f5625f

use bash -c for the probes to work properly

9be2645

fix imports

e618bd7

cr check and improve unit test

85aada5

fix linter

27d6db0

e2e test use the K8SPG-708-ppg image for now

7eaae86

revert experiment

b759611

cr: add comments on the upstream code

4c6b426

add new entrypoint script and fix unit tests

84350e5

gkech mentioned this pull request Apr 3, 2025

K8SPG-708 introduce ready/live probes for postgres, add sleep forever checking percona/percona-docker#1148

Open

utilize init container for the new .sh

41132e6

gkech dismissed egegunes’s stale review via 41132e6 April 3, 2025 07:23

gkech force-pushed the K8SPG-708 branch from 595ea38 to 41132e6 Compare April 3, 2025 07:23

fix references to op paths

cdd7e32

gkech force-pushed the K8SPG-708 branch from 372f279 to cdd7e32 Compare April 3, 2025 07:52

gkech added 3 commits April 3, 2025 10:54

create /tmp/postgres

8e5f5c0

fix paths again

e410638

add init container to db pod

dcb5531

gkech requested a review from egegunes April 3, 2025 08:20

fix imports

c1f48de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

K8SPG-708 replace ready/live probe http check with custom command, change pg entrypoint #1099

K8SPG-708 replace ready/live probe http check with custom command, change pg entrypoint #1099

gkech commented Mar 21, 2025 •

edited

Loading

gkech Mar 24, 2025

gkech Mar 24, 2025

egegunes Mar 25, 2025

gkech Mar 26, 2025

nmarukovich Mar 26, 2025

gkech Mar 26, 2025 •

edited

Loading

nmarukovich Mar 26, 2025

gkech Mar 26, 2025

nmarukovich Mar 26, 2025

gkech Mar 26, 2025

		container.LivenessProbe.Exec = &corev1.ExecAction{
		Command: []string{"/usr/local/bin/postgres-liveness-check.sh"},

K8SPG-708 replace ready/live probe http check with custom command, change pg entrypoint #1099

Are you sure you want to change the base?

K8SPG-708 replace ready/live probe http check with custom command, change pg entrypoint #1099

Conversation

gkech commented Mar 21, 2025 • edited Loading

CHANGE DESCRIPTION

Testing sleep forever

CHECKLIST

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkech Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkech commented Mar 21, 2025 •

edited

Loading

gkech Mar 26, 2025 •

edited

Loading