Improving Postgres CR Status with Additional Details

[RavinaChidambaram]

Fixes #2669

Following things have been added:

• Added observedGeneration
• Added conditions of type ReconciliationSuccessful and Ready
• Added numberOfInstances and labelSelector for scale subresource
• Added scale subresource configuration in the CRD
• postgresClusterStatus is still part of the CRD

Detailed example flow:
Conditions are updated everytime there is a change in PostgresClusterStatus. And if the cluster is running successfully, the status looks like:

status:
  conditions:
  - lastTransitionTime: "2024-08-02T12:33:34Z"
    status: "True"
    type: ReconciliationSuccessful
  - lastTransitionTime: "2024-08-02T12:33:34Z"
    status: "True"
    type: Ready
  labelSelector: cluster-name=acid-minimal-cluster
  numberOfInstances: 3
  observedGeneration: 4
  postgresClusterStatus: Running

If there is a transition from Running state to a failed state, type ReconciliationSuccessful will be false.
When a failed case (Create failed,Update failed, Sync failed) is encountered, the error message will be printed in the message in conditions

status:
  conditions:
  - lastTransitionTime: "2024-08-02T13:05:38Z"
    message: 'could not create master endpoint: could not create master endpoint:
      endpoints "acid-minimal-cluster-test-1" already exists'
    reason: CreateFailed
    status: "False"
    type: ReconciliationSuccessful
  - lastTransitionTime: "2024-08-02T13:05:38Z"
    status: "False"
    type: Ready
  labelSelector: cluster-name=acid-minimal-cluster
  numberOfInstances: 0
  postgresClusterStatus: CreateFailed

[RavinaChidambaram]

Hi @FxKu , Is there any update on this PR?

[FxKu]

I had to concentrate on the 1.13.0 release. But we will definitely include it in the next release in autumn. This is a great addition. I have to focus on some internal projects for the next weeks, but still, I hope to find some time in between to review.

[FxKu]

@RavinaChidambaram can you fix the failing unit test so e2e pipeline can run? There are places where we logged the status assuming it's just a string. Now that it has become a struct you either have to change to %#v when formatting the log lines or specify the PostgresClusterStatus field.

[FxKu]

why use an extra type and not just metav1.Time?

[RavinaChidambaram]

Mostly to avoid semantic check inequality with metav1.time, with a wrapper around metav1.time there won't be a semantic inequality even if the times are different but everything else in the CR is same, this could be handy while writing unit tests or e2e tests I think

[RavinaChidambaram]

@FxKu , Is this okay?

[FxKu]

As half of my comments target the scale subresource, I would suggest to remove it from here and create a separate PR for it. Then you could concentrate only on the status and we get it merged quicker.

Have you tried to run code-generation. I think, the generated code has to get updated with the new status structure.

[RavinaChidambaram]

I have made changes to fix the unit tests and successfully ran code-generation, unit tests and e2e test downstream.
Kindly review the changes and if you still think it's necessary to create a new branch for scale subresource addition, I will proceed with it.

[RavinaChidambaram]

Hi @FxKu , any updates?

[FxKu]

@RavinaChidambaram sorry for not coming back sooner. I see that you did not reflect all my comments. I also do wonder if we have to change the code logic and move the SetPostgresCRDStatus into another package so that the signature can remain the same (clusterName spec.NamespacedName, status string) and we will the other fields inside this function? Because, right now we have so many code lines changed and as far as I can see:

• NumberOfInstances
• LabelSelector
• ObservedGeneration
• Conditions

would always be constructed in the same way. WDYT?

[RavinaChidambaram]

I think I have made all the changes that you mentioned, except for the metav1.time. And I have mentioned the reason for using that in the thread itself. Kindly let me know what you think about that.
Regarding keeping the SetPostgresCRDStatus signature unchanged, I agree - we can move the construction of fields like NumberOfInstances, LabelSelector, ObservedGeneration, and Conditions into the function itself. And I am working on implementing the same.

[zalando-robot]

Cannot start a pipeline due to:

No accountable user for this pipeline: no Zalando employee associated to this GitHub username

Click on pipeline status check Details link below for more information.

[RavinaChidambaram]

Hi @FxKu ,
As suggested, the construction of the status fields has been moved into the function itself.
But this required adding two new parameters to the function SetPostgresCRDStatus - clusterNameLabel and message.