From 1f79d5963c6f64f60665a1538ebce605857b1826 Mon Sep 17 00:00:00 2001 From: Thomas Sapelza Date: Sun, 25 Oct 2026 17:40:48 +0100 Subject: [PATCH] rename CRStatus.setPhase to updatePhase so Jackson keeps the stored transition time Jackson called setPhase while it read a status from the cluster. The API server sends lastPhaseTransitionTime before phase, so the stored time was overwritten with now on each read. Assisted-by: Claude:claude-opus-5-5 --- .../postgresql/core/BaseReconciler.java | 4 +- .../aboutbits/postgresql/core/CRStatus.java | 8 +- .../ClusterConnectionReconciler.java | 2 +- .../crd/database/DatabaseReconciler.java | 10 +- .../DefaultPrivilegeReconciler.java | 6 +- .../postgresql/crd/grant/GrantReconciler.java | 6 +- .../postgresql/crd/role/RoleReconciler.java | 10 +- .../crd/schema/SchemaReconciler.java | 10 +- .../postgresql/core/CRStatusTest.java | 126 ++++++++++++++++++ .../ClusterConnectionReconcilerTest.java | 2 +- .../crd/database/DatabaseReconcilerTest.java | 2 +- .../DefaultPrivilegeReconcilerTest.java | 6 +- .../crd/grant/GrantReconcilerTest.java | 12 +- .../crd/role/RoleReconcilerTest.java | 6 +- .../crd/schema/SchemaReconcilerTest.java | 2 +- 15 files changed, 171 insertions(+), 41 deletions(-) create mode 100644 operator/src/test/java/it/aboutbits/postgresql/core/CRStatusTest.java diff --git a/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java index 6acd76c..18452a2 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java @@ -100,7 +100,7 @@ public void setErrorPhase( message ); - status.setPhase(CRPhase.ERROR) + status.updatePhase(CRPhase.ERROR) .setMessage(message); } @@ -117,7 +117,7 @@ public UpdateControl handleError( exception ); - status.setPhase(CRPhase.ERROR) + status.updatePhase(CRPhase.ERROR) .setMessage(exception.getMessage()); return UpdateControl.patchStatus(resource) diff --git a/operator/src/main/java/it/aboutbits/postgresql/core/CRStatus.java b/operator/src/main/java/it/aboutbits/postgresql/core/CRStatus.java index fcdf601..ed5e950 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/core/CRStatus.java +++ b/operator/src/main/java/it/aboutbits/postgresql/core/CRStatus.java @@ -39,11 +39,15 @@ public class CRStatus { private long observedGeneration = 0; /// Update the current phase. When the phase changes, the [#lastPhaseTransitionTime] - /// is updated to the current UTC time and the message is set to `null`. + /// is updated to the current UTC time. + /// + /// Deliberately not a setter: Jackson would call it while it reads a status from the cluster. + /// The API server sends `lastPhaseTransitionTime` before `phase`, so the stored time would be + /// overwritten with now. /// /// @param newPhase the new phase /// @return this status instance - public CRStatus setPhase(CRPhase newPhase) { + public CRStatus updatePhase(CRPhase newPhase) { if (this.phase == newPhase) { return this; } diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java index d958019..c139bd5 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconciler.java @@ -39,7 +39,7 @@ public UpdateControl reconcile( try (var dsl = contextFactory.getDSLContext(resource)) { var version = dsl.fetchSingle("select version()").into(String.class); - status.setPhase(CRPhase.READY).setMessage(version); + status.updatePhase(CRPhase.READY).setMessage(version); return UpdateControl.patchStatus(resource); } catch (RuntimeException e) { diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/database/DatabaseReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/database/DatabaseReconciler.java index 2ef47c2..2c94568 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/database/DatabaseReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/database/DatabaseReconciler.java @@ -57,7 +57,7 @@ public UpdateControl reconcile( ); if (clusterConnectionOptional.isEmpty()) { - status.setPhase(CRPhase.PENDING) + status.updatePhase(CRPhase.PENDING) .setMessage("The specified ClusterConnection does not exist or is not ready yet [resource=%s/%s]".formatted( getResourceNamespaceOrOwn(resource, clusterRef.getNamespace()), clusterRef.getName() @@ -110,7 +110,7 @@ public DeleteControl cleanup( ); if (status.getPhase() != CRPhase.DELETING) { - status.setPhase(CRPhase.DELETING); + status.updatePhase(CRPhase.DELETING); if (spec.getReclaimPolicy() == ReclaimPolicy.DELETE) { status.setMessage("Database deletion in progress"); @@ -201,7 +201,7 @@ private UpdateControl reconcileWithoutTransaction( spec ); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); @@ -218,7 +218,7 @@ private UpdateControl reconcileWithoutTransaction( name ); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); @@ -232,7 +232,7 @@ private UpdateControl reconcileWithoutTransaction( databaseService.changeDatabaseOwner(dsl, spec); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage("Database owner changed [previousOwner=%s, newOwner=%s]".formatted( currentOwner, expectedOwner diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java index f2becae..40c3f5b 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconciler.java @@ -84,7 +84,7 @@ public UpdateControl reconcile( } if (clusterConnectionOptional.isEmpty()) { - status.setPhase(CRPhase.PENDING) + status.updatePhase(CRPhase.PENDING) .setMessage("The specified ClusterConnection does not exist or is not ready yet [resource=%s/%s]".formatted( getResourceNamespaceOrOwn(resource, clusterRef.getNamespace()), clusterRef.getName() @@ -138,7 +138,7 @@ public DeleteControl cleanup( ); if (status.getPhase() != CRPhase.DELETING) { - status.setPhase(CRPhase.DELETING) + status.updatePhase(CRPhase.DELETING) .setMessage("DefaultPrivilege deletion in progress"); context.getClient().resource(resource).patchStatus(); @@ -290,7 +290,7 @@ private UpdateControl reconcileInTransaction( ); } - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java index fb8a594..c416942 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/grant/GrantReconciler.java @@ -89,7 +89,7 @@ public UpdateControl reconcile( } if (clusterConnectionOptional.isEmpty()) { - status.setPhase(CRPhase.PENDING) + status.updatePhase(CRPhase.PENDING) .setMessage("The specified ClusterConnection does not exist or is not ready yet [resource=%s/%s]".formatted( getResourceNamespaceOrOwn(resource, clusterRef.getNamespace()), clusterRef.getName() @@ -143,7 +143,7 @@ public DeleteControl cleanup( ); if (status.getPhase() != CRPhase.DELETING) { - status.setPhase(CRPhase.DELETING) + status.updatePhase(CRPhase.DELETING) .setMessage("Grant deletion in progress"); context.getClient().resource(resource).patchStatus(); @@ -421,7 +421,7 @@ private UpdateControl reconcileInTransaction( ); } - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(message); return UpdateControl.patchStatus(resource); diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java index d4f2d76..9d10e1b 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java @@ -83,7 +83,7 @@ public UpdateControl reconcile( ); if (clusterConnectionOptional.isEmpty()) { - status.setPhase(CRPhase.PENDING) + status.updatePhase(CRPhase.PENDING) .setMessage("The specified ClusterConnection does not exist or is not ready yet [resource=%s/%s]".formatted( getResourceNamespaceOrOwn(resource, clusterRef.getNamespace()), clusterRef.getName() @@ -169,7 +169,7 @@ public DeleteControl cleanup( ); if (status.getPhase() != CRPhase.DELETING) { - status.setPhase(CRPhase.DELETING) + status.updatePhase(CRPhase.DELETING) .setMessage("Role deletion in progress"); context.getClient().resource(resource).patchStatus(); @@ -290,7 +290,7 @@ private UpdateControl reconcileInTransaction( serverPassword ); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); @@ -310,7 +310,7 @@ private UpdateControl reconcileInTransaction( name ); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); @@ -357,7 +357,7 @@ private UpdateControl reconcileInTransaction( ); } - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/schema/SchemaReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/schema/SchemaReconciler.java index 09356c6..5f36a68 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/schema/SchemaReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/schema/SchemaReconciler.java @@ -57,7 +57,7 @@ public UpdateControl reconcile( ); if (clusterConnectionOptional.isEmpty()) { - status.setPhase(CRPhase.PENDING) + status.updatePhase(CRPhase.PENDING) .setMessage("The specified ClusterConnection does not exist or is not ready yet [resource=%s/%s]".formatted( getResourceNamespaceOrOwn(resource, clusterRef.getNamespace()), clusterRef.getName() @@ -113,7 +113,7 @@ public DeleteControl cleanup( ); if (status.getPhase() != CRPhase.DELETING) { - status.setPhase(CRPhase.DELETING); + status.updatePhase(CRPhase.DELETING); if (spec.getReclaimPolicy() == ReclaimPolicy.DELETE) { status.setMessage("Schema deletion in progress"); @@ -205,7 +205,7 @@ private UpdateControl reconcileInTransaction( spec ); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); @@ -222,7 +222,7 @@ private UpdateControl reconcileInTransaction( name ); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage(null); return UpdateControl.patchStatus(resource); @@ -236,7 +236,7 @@ private UpdateControl reconcileInTransaction( schemaService.changeSchemaOwner(tx, spec); - status.setPhase(CRPhase.READY) + status.updatePhase(CRPhase.READY) .setMessage("Schema owner changed [previousOwner=%s, newOwner=%s]".formatted( currentOwner, expectedOwner diff --git a/operator/src/test/java/it/aboutbits/postgresql/core/CRStatusTest.java b/operator/src/test/java/it/aboutbits/postgresql/core/CRStatusTest.java new file mode 100644 index 0000000..76642ca --- /dev/null +++ b/operator/src/test/java/it/aboutbits/postgresql/core/CRStatusTest.java @@ -0,0 +1,126 @@ +package it.aboutbits.postgresql.core; + +import io.fabric8.kubernetes.client.utils.KubernetesSerialization; +import org.jspecify.annotations.NullMarked; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.time.OffsetDateTime; + +import static org.assertj.core.api.Assertions.assertThat; + +@NullMarked +class CRStatusTest { + @Nested + class UpdatePhase { + @Test + @DisplayName("A new status should start out pending without a transition time") + void newStatus_isPending() { + // when + var status = new CRStatus(); + + // then + assertThat(status.getPhase()).isEqualTo(CRPhase.PENDING); + assertThat(status.getLastPhaseTransitionTime()).isNull(); + } + + @Test + @DisplayName("Changing the phase should stamp the transition time") + void phaseChange_stampsTransitionTime() { + // given + var status = new CRStatus(); + + // when + status.updatePhase(CRPhase.READY); + + // then + assertThat(status.getPhase()).isEqualTo(CRPhase.READY); + assertThat(status.getLastPhaseTransitionTime()).isNotNull(); + } + + @Test + @DisplayName("Changing the phase again should replace the previous transition time") + void secondPhaseChange_replacesTransitionTime() { + // given + var json = """ + {"lastPhaseTransitionTime":"2026-01-01T00:00:00Z","phase":"READY"} + """; + + var status = new KubernetesSerialization().unmarshal( + json, + CRStatus.class + ); + + // when + status.updatePhase(CRPhase.ERROR); + + // then + assertThat(status.getPhase()).isEqualTo(CRPhase.ERROR); + assertThat(status.getLastPhaseTransitionTime()) + .isNotNull() + .isAfter(OffsetDateTime.parse("2026-01-01T00:00:00Z")); + } + + @Test + @DisplayName("Setting the same phase again should not touch the transition time") + void samePhase_keepsTransitionTime() { + // given + // A fixed time in the past, so the test fails if the time is overwritten with now. + var json = """ + {"lastPhaseTransitionTime":"2026-01-01T00:00:00Z","phase":"READY"} + """; + + var status = new KubernetesSerialization().unmarshal( + json, + CRStatus.class + ); + + // when + status.updatePhase(CRPhase.READY); + + // then + assertThat(status.getLastPhaseTransitionTime()) + .isNotNull() + .isAtSameInstantAs(OffsetDateTime.parse("2026-01-01T00:00:00Z")); + } + + @Test + @DisplayName("Updating the phase should be chainable with the remaining status setters") + void updatePhase_isChainable() { + // when + var status = new CRStatus() + .updatePhase(CRPhase.ERROR) + .setMessage("Something went wrong"); + + // then + assertThat(status.getPhase()).isEqualTo(CRPhase.ERROR); + assertThat(status.getMessage()).isEqualTo("Something went wrong"); + } + } + + @Nested + class Deserialize { + @Test + @DisplayName("Reading a status from the cluster should keep its transition time") + void statusFromCluster_keepsTransitionTime() { + // given + // The API server sends the status keys in alphabetical order. + var json = """ + {"lastPhaseTransitionTime":"2026-01-01T00:00:00Z","phase":"READY"} + """; + + // when + var status = new KubernetesSerialization().unmarshal( + json, + CRStatus.class + ); + + // then + assertThat(status.getPhase()).isEqualTo(CRPhase.READY); + assertThat(status.getLastPhaseTransitionTime()) + .isNotNull() + .isAtSameInstantAs(OffsetDateTime.parse("2026-01-01T00:00:00Z")); + } + } +} diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconcilerTest.java index 6af1cb8..017ed91 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionReconcilerTest.java @@ -255,7 +255,7 @@ private static void assertThatClusterConnectionHasExpectedStatus( private static CRStatus getInitialClusterConnectionStatus(ClusterConnection clusterConnection) { return new CRStatus() .setName(clusterConnection.getName()) - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); } } diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/database/DatabaseReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/database/DatabaseReconcilerTest.java index 7e59152..21e3e44 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/database/DatabaseReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/database/DatabaseReconcilerTest.java @@ -59,7 +59,7 @@ void createDatabase_andStatusReady() { // then var expectedStatus = new CRStatus() .setName(dbName) - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatDatabaseHasExpectedStatus( diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java index 0451b7e..8c47c79 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/defaultprivilege/DefaultPrivilegeReconcilerTest.java @@ -409,7 +409,7 @@ void defaultPrivilegeOnSchema() { // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatDefaultPrivilegeHasStatus( @@ -526,7 +526,7 @@ void defaultPrivilegeOnTable( // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatDefaultPrivilegeHasStatus( @@ -651,7 +651,7 @@ void defaultPrivilegeOnSequence() { // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatDefaultPrivilegeHasStatus( diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java index f09ed14..4d91af7 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/grant/GrantReconcilerTest.java @@ -415,7 +415,7 @@ void grantOnDatabase() { // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatGrantHasStatus( @@ -536,7 +536,7 @@ void grantOnSchema() { var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatGrantHasStatus( @@ -666,7 +666,7 @@ void grantOnTable( // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatGrantHasStatus( @@ -808,7 +808,7 @@ void grantOnAllTables( // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatGrantHasStatus( @@ -1155,7 +1155,7 @@ void grantOnSequence() { // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatGrantHasStatus( @@ -1296,7 +1296,7 @@ void grantOnAllSequences() { // then var expectedStatus = new CRStatus() .setName("") - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatGrantHasStatus( diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/role/RoleReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/role/RoleReconcilerTest.java index f9705d3..1465571 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/role/RoleReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/role/RoleReconcilerTest.java @@ -81,7 +81,7 @@ void createRole_withLogin_andStatusReady() { // then: assert READY var expectedStatus = new RoleStatus() .setName(roleName) - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatRoleHasExpectedStatus( @@ -118,7 +118,7 @@ void createRole_withoutLogin_andStatusReady() { var expectedStatus = new RoleStatus() .setName(roleName) - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatRoleHasExpectedStatus( @@ -160,7 +160,7 @@ void toggleRoleLogin_updatesCorrectly() { role, new RoleStatus() .setName(roleName) - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L), now ); diff --git a/operator/src/test/java/it/aboutbits/postgresql/crd/schema/SchemaReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/crd/schema/SchemaReconcilerTest.java index a2f598c..33b9962 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/crd/schema/SchemaReconcilerTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/crd/schema/SchemaReconcilerTest.java @@ -72,7 +72,7 @@ void createSchema_andStatusReady() { // then var expectedStatus = new CRStatus() .setName(schemaName) - .setPhase(CRPhase.READY) + .updatePhase(CRPhase.READY) .setObservedGeneration(1L); assertThatSchemaHasExpectedStatus(