K8SPG-938 fix conditions by nmarukovich · Pull Request #1435 · percona/percona-postgresql-operator

nmarukovich · 2026-02-10T10:00:57Z

CHANGE DESCRIPTION

Problem:
The syncConditionsFromPostgresToPercona function wasn't updating existing conditions - it only added new ones.
Additionally, the updateConditions function was checking conditions from the status parameter (PostgresCluster status) instead of cr.Status.Conditions (where we just copied them), which could lead to inconsistencies.

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

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

kg pg monitoring-pmm3  -o jsonpath='{.status.conditions}' | jq
[
  {
    "lastTransitionTime": "2026-02-10T08:17:31Z",
    "message": "",
    "reason": "AllConditionsAreTrue",
    "status": "True",
    "type": "ReadyForBackup"
  },
  {
    "lastTransitionTime": "2026-02-10T08:16:12Z",
    "message": "pgBackRest dedicated repository host is ready",
    "observedGeneration": 1,
    "reason": "RepoHostReady",
    "status": "True",
    "type": "PGBackRestRepoHostReady"
  },
  {
    "lastTransitionTime": "2026-02-10T08:16:39Z",
    "message": "pgBackRest replica create repo is ready for backups",
    "observedGeneration": 1,
    "reason": "StanzaCreated",
    "status": "True",
    "type": "PGBackRestReplicaRepoReady"
  },
  {
    "lastTransitionTime": "2026-02-10T08:17:31Z",
    "message": "pgBackRest replica creation is now possible",
    "observedGeneration": 1,
    "reason": "RepoBackupComplete",
    "status": "True",
    "type": "PGBackRestReplicaCreate"
  },
  {
    "lastTransitionTime": "2026-02-10T08:15:53Z",
    "message": "Deployment has minimum availability.",
    "observedGeneration": 1,
    "reason": "MinimumReplicasAvailable",
    "status": "True",
    "type": "ProxyAvailable"
  }
]

kg postgrescluster monitoring-pmm3  -o jsonpath='{.status.conditions}' | jq
[
  {
    "lastTransitionTime": "2026-02-10T08:16:12Z",
    "message": "pgBackRest dedicated repository host is ready",
    "observedGeneration": 3,
    "reason": "RepoHostReady",
    "status": "True",
    "type": "PGBackRestRepoHostReady"
  },
  {
    "lastTransitionTime": "2026-02-10T08:16:41Z",
    "message": "pgBackRest replica create repo is ready for backups",
    "observedGeneration": 3,
    "reason": "StanzaCreated",
    "status": "True",
    "type": "PGBackRestReplicaRepoReady"
  },
  {
    "lastTransitionTime": "2026-02-10T08:17:33Z",
    "message": "pgBackRest replica creation is now possible",
    "observedGeneration": 3,
    "reason": "RepoBackupComplete",
    "status": "True",
    "type": "PGBackRestReplicaCreate"
  },
  {
    "lastTransitionTime": "2026-02-10T08:15:53Z",
    "message": "Deployment has minimum availability.",
    "observedGeneration": 3,
    "reason": "MinimumReplicasAvailable",
    "status": "True",
    "type": "ProxyAvailable"
  }
]

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?

egegunes

is it possible to cover this fix with some unit tests?

gkech · 2026-02-11T09:18:14Z

percona/controller/pgcluster/status.go

 	syncPgbackrestFromPostgresToPercona(cr, status)

-	repoCondition := meta.FindStatusCondition(status.Conditions, postgrescluster.ConditionRepoHostReady)
+	repoCondition := meta.FindStatusCondition(cr.Status.Conditions, postgrescluster.ConditionRepoHostReady)


Since status and cr are already synced, does it matter switching to cr.Status from status? Just trying to understand if this change needs to be made after all.

You're right. After sync they're the same. I'm suggesting cr.Status.Conditions mainly for consistency - setClusterNotReadyCondition uses it, so the checks should too. But it's not critical, we can keep status.Conditions.
WDYT?

sounds good!

gkech · 2026-02-11T09:19:54Z

percona/controller/pgcluster/status.go

-		}
-
-		newCond := metav1.Condition{
+		_ = meta.SetStatusCondition(&cr.Status.Conditions, metav1.Condition{


Adding this comment here regarding unit testing. Agreeing with @egegunes, IMO we could add comprehensive unit test func TestUpdateConditions(t *testing.T) checking the updateConditions function.

updateConditions is already covered by integration tests. Should I add unit tests for it as well, or replace the integration tests with unit tests?

gkech · 2026-02-12T11:41:45Z

percona/controller/pgcluster/status_test.go

 	})
 })

+var _ = Describe("syncConditionsFromPostgresToPercona", func() {


I had in mind something like that:

func TestUpdateConditions(t *testing.T) { switchover := "switching" switchoverTimeline := int64(3) tests := []struct { name string crConditions []metav1.Condition statusConditions []metav1.Condition pgBackRestStatus *v1beta1.PGBackRestStatus patroniStatus v1beta1.PatroniStatus expectedReadyForBackupStatus metav1.ConditionStatus expectedReadyForBackupReason string expectedSyncedConditions []metav1.Condition expectedPGBackRest *v1beta1.PGBackRestStatus expectedPatroni *v1beta1.PatroniStatus }{ { name: "no conditions set - RepoHostReady missing", expectedReadyForBackupStatus: metav1.ConditionFalse, expectedReadyForBackupReason: postgrescluster.ConditionRepoHostReady, }, { name: "RepoHostReady is false", statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionFalse, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, }, expectedReadyForBackupStatus: metav1.ConditionFalse, expectedReadyForBackupReason: postgrescluster.ConditionRepoHostReady, expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionFalse, Reason: "test"}, {Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test"}, }, }, { name: "RepoHostReady is true but ReplicaCreate is missing", statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, }, expectedReadyForBackupStatus: metav1.ConditionFalse, expectedReadyForBackupReason: postgrescluster.ConditionReplicaCreate, expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test"}, }, }, { name: "RepoHostReady is true but ReplicaCreate is false", statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionFalse, Reason: "test", }, }, expectedReadyForBackupStatus: metav1.ConditionFalse, expectedReadyForBackupReason: postgrescluster.ConditionReplicaCreate, expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test"}, {Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionFalse, Reason: "test"}, }, }, { name: "both conditions are true", statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, }, expectedReadyForBackupStatus: metav1.ConditionTrue, expectedReadyForBackupReason: "AllConditionsAreTrue", expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test"}, {Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test"}, }, }, { name: "existing ReadyForBackup condition gets updated when RepoHostReady becomes true", crConditions: []metav1.Condition{ { Type: pNaming.ConditionClusterIsReadyForBackup, Status: metav1.ConditionFalse, Reason: postgrescluster.ConditionRepoHostReady, }, }, statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, }, expectedReadyForBackupStatus: metav1.ConditionTrue, expectedReadyForBackupReason: "AllConditionsAreTrue", expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test"}, {Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test"}, }, }, { name: "existing true condition stays true when both conditions remain true", crConditions: []metav1.Condition{ { Type: pNaming.ConditionClusterIsReadyForBackup, Status: metav1.ConditionTrue, Reason: "AllConditionsAreTrue", }, }, statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, }, expectedReadyForBackupStatus: metav1.ConditionTrue, expectedReadyForBackupReason: "AllConditionsAreTrue", expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test"}, {Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test"}, }, }, { name: "syncs conditions from postgres status to percona cr", statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, { Type: "SomeOtherCondition", Status: metav1.ConditionTrue, Reason: "other", }, }, expectedReadyForBackupStatus: metav1.ConditionTrue, expectedReadyForBackupReason: "AllConditionsAreTrue", expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test"}, {Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test"}, {Type: "SomeOtherCondition", Status: metav1.ConditionTrue, Reason: "other"}, }, }, { name: "existing condition on cr is not updated by sync", crConditions: []metav1.Condition{ { Type: "SomeOtherCondition", Status: metav1.ConditionFalse, Reason: "original-reason", }, }, statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, { Type: "SomeOtherCondition", Status: metav1.ConditionTrue, Reason: "updated-reason", }, }, expectedReadyForBackupStatus: metav1.ConditionTrue, expectedReadyForBackupReason: "AllConditionsAreTrue", expectedSyncedConditions: []metav1.Condition{ {Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test"}, {Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test"}, {Type: "SomeOtherCondition", Status: metav1.ConditionTrue, Reason: "updated-reason"}, }, }, { name: "syncs pgbackrest status from postgres to percona", statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, }, pgBackRestStatus: &v1beta1.PGBackRestStatus{ RepoHost: &v1beta1.RepoHostStatus{Ready: true}, }, expectedReadyForBackupStatus: metav1.ConditionTrue, expectedReadyForBackupReason: "AllConditionsAreTrue", expectedPGBackRest: &v1beta1.PGBackRestStatus{ RepoHost: &v1beta1.RepoHostStatus{Ready: true}, }, }, { name: "syncs patroni status from postgres to percona", statusConditions: []metav1.Condition{ { Type: postgrescluster.ConditionRepoHostReady, Status: metav1.ConditionTrue, Reason: "test", }, { Type: postgrescluster.ConditionReplicaCreate, Status: metav1.ConditionTrue, Reason: "test", }, }, patroniStatus: v1beta1.PatroniStatus{ SystemIdentifier: "12345", Switchover: &switchover, SwitchoverTimeline: &switchoverTimeline, }, expectedReadyForBackupStatus: metav1.ConditionTrue, expectedReadyForBackupReason: "AllConditionsAreTrue", expectedPatroni: &v1beta1.PatroniStatus{ SystemIdentifier: "12345", Switchover: &switchover, SwitchoverTimeline: &switchoverTimeline, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cr := &v2.PerconaPGCluster{ Status: v2.PerconaPGClusterStatus{}, } if tt.crConditions != nil { cr.Status.Conditions = tt.crConditions } status := &v1beta1.PostgresClusterStatus{ Patroni: tt.patroniStatus, } if tt.statusConditions != nil { status.Conditions = tt.statusConditions } if tt.pgBackRestStatus != nil { status.PGBackRest = tt.pgBackRestStatus } updateConditions(cr, status) condition := meta.FindStatusCondition(cr.Status.Conditions, pNaming.ConditionClusterIsReadyForBackup) require.NotNil(t, condition) assert.Equal(t, tt.expectedReadyForBackupStatus, condition.Status) assert.Equal(t, tt.expectedReadyForBackupReason, condition.Reason) for _, expected := range tt.expectedSyncedConditions { synced := meta.FindStatusCondition(cr.Status.Conditions, expected.Type) if !assert.NotNil(t, synced) { continue } assert.Equal(t, expected.Status, synced.Status) assert.Equal(t, expected.Reason, synced.Reason) } if tt.expectedPGBackRest != nil { require.NotNil(t, cr.Status.PGBackRest) require.NotNil(t, cr.Status.PGBackRest.RepoHost) assert.Equal(t, tt.expectedPGBackRest.RepoHost.Ready, cr.Status.PGBackRest.RepoHost.Ready) } if tt.expectedPatroni != nil { require.NotNil(t, cr.Status.Patroni.Status) assert.Equal(t, tt.expectedPatroni.SystemIdentifier, cr.Status.Patroni.Status.SystemIdentifier) assert.Equal(t, tt.expectedPatroni.Switchover, cr.Status.Patroni.Status.Switchover) assert.Equal(t, tt.expectedPatroni.SwitchoverTimeline, cr.Status.Patroni.Status.SwitchoverTimeline) } }) } }

Firstly because we dont need to use ginkgo formating for it, and secondly because instead of just testing syncConditionsFromPostgresToPercona, we can check the whole updateConditions func.

egegunes · 2026-02-13T05:48:37Z

percona/controller/pgcluster/status_test.go

+// Test coverage:
+//   - ClusterIsReadyForBackup condition logic:
+//   - no conditions set (RepoHostReady missing)
+//   - RepoHostReady is false
+//   - RepoHostReady is true but ReplicaCreate is missing
+//   - RepoHostReady is true but ReplicaCreate is false
+//   - both conditions are true
+//   - existing condition gets updated when status changes
+//   - existing condition stays unchanged when status remains the same
+//   - Condition synchronization from Postgres to Percona:
+//   - syncs all conditions from postgres status
+//   - updates existing conditions on CR
+//   - Status synchronization:
+//   - syncs PGBackRest status
+//   - syncs Patroni status


is this comment needed?

I put it by reason, yes, because we test many cases and I thought it could be nice to see all of them as the list. I can delete it. No problem.

yea I agree, it is not needed, we have names on each test case already, this will be hard to maintain

yes, it'll be forgotten and quickly become outdated

…percona-postgresql-operator into K8SPG-938_fix_conditions_update

JNKPercona · 2026-02-13T13:23:12Z

Test Name	Result	Time
backup-enable-disable	passed	00:05:52
builtin-extensions	passed	00:05:02
custom-envs	passed	00:18:20
custom-extensions	passed	00:13:34
custom-tls	passed	00:05:15
database-init-sql	passed	00:02:12
demand-backup	passed	00:24:42
finalizers	passed	00:03:31
init-deploy	passed	00:03:01
huge-pages	passed	00:02:53
monitoring	passed	00:06:55
monitoring-pmm3	passed	00:07:55
one-pod	passed	00:05:30
operator-self-healing	passed	00:07:46
pitr	passed	00:11:52
scaling	passed	00:05:18
scheduled-backup	passed	00:26:04
self-healing	passed	00:09:54
sidecars	passed	00:02:27
standby-pgbackrest	passed	00:11:45
standby-streaming	passed	00:09:11
start-from-backup	passed	00:10:30
tablespaces	passed	00:06:51
telemetry-transfer	passed	00:03:44
upgrade-consistency	passed	00:05:33
upgrade-minor	passed	00:06:17
users	passed	00:04:29

Summary	Value
Tests Run	27/27
Job Duration	01:23:49
Total Test Time	03:46:34

commit: 2898177
image: perconalab/percona-postgresql-operator:PR-1435-28981776f

K8SPG-938 fix conditions

1534740

nmarukovich requested review from egegunes, gkech, hors, mayankshah1607, oksana-grishchenko and pooknull as code owners February 10, 2026 10:00

egegunes requested changes Feb 11, 2026

View reviewed changes

gkech reviewed Feb 11, 2026

View reviewed changes

nmarukovich and others added 2 commits February 11, 2026 15:27

Merge branch 'main' into K8SPG-938_fix_conditions_update

86212bb

add test

88b5083

gkech reviewed Feb 12, 2026

View reviewed changes

egegunes added this to the v2.9.0 milestone Feb 12, 2026

nmarukovich and others added 3 commits February 12, 2026 19:00

add unit test

fb68b5f

Merge branch 'main' into K8SPG-938_fix_conditions_update

cc51389

Merge branch 'main' into K8SPG-938_fix_conditions_update

877564e

nmarukovich requested review from egegunes and gkech February 12, 2026 18:35

egegunes requested changes Feb 13, 2026

View reviewed changes

nmarukovich and others added 3 commits February 13, 2026 12:56

delete comment

16509e8

Merge branch 'K8SPG-938_fix_conditions_update' of github.com:percona/…

ba5919f

…percona-postgresql-operator into K8SPG-938_fix_conditions_update

Merge branch 'main' into K8SPG-938_fix_conditions_update

2898177

gkech approved these changes Feb 13, 2026

View reviewed changes

hors requested a review from egegunes February 13, 2026 12:15

hors approved these changes Feb 13, 2026

View reviewed changes

Conversation

nmarukovich commented Feb 10, 2026

CHANGE DESCRIPTION

CHECKLIST

Uh oh!

egegunes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JNKPercona commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants