-
Notifications
You must be signed in to change notification settings - Fork 974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving Postgres CR Status with Additional Details #2714
base: master
Are you sure you want to change the base?
Improving Postgres CR Status with Additional Details #2714
Conversation
Signed-off-by: RavinaChidambaram <[email protected]>
Signed-off-by: RavinaChidambaram <[email protected]>
Hi @FxKu , Is there any update on this PR? |
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. |
@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 |
pkg/apis/acid.zalan.do/v1/crds.go
Outdated
@@ -25,6 +25,12 @@ const ( | |||
OperatorConfigCRDResourceShort = "opconfig" | |||
) | |||
|
|||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these not const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to const
} | ||
|
||
// Condition contains the conditions of the PostgreSQL cluster | ||
type Condition struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be changed to PostgresqlCondition? This aligns with other K8s resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, changed to PostgresqlCondition
@@ -225,9 +226,48 @@ type Sidecar struct { | |||
// UserFlags defines flags (such as superuser, nologin) that could be assigned to individual users | |||
type UserFlags []string | |||
|
|||
type Conditions []Condition | |||
|
|||
type ConditionType string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type ConditionType string | |
type PostgresqlConditionType string |
type Condition struct { | ||
Type ConditionType `json:"type" description:"type of status condition"` | ||
Status v1.ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"` | ||
LastTransitionTime VolatileTime `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use an extra type and not just metav1.Time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
pkg/cluster/cluster.go
Outdated
@@ -255,16 +255,20 @@ func (c *Cluster) Create() (err error) { | |||
ss *appsv1.StatefulSet | |||
) | |||
|
|||
//Even though its possible to propogate other CR labels to the pods, picking the default label here since its propogated to all the pods by default. But this means that in order for the scale subresource to work properly, user must set the "cluster-name" key in their CRs with value matching the CR name. | |||
labelstring := fmt.Sprintf("%s=%s", "cluster-name", c.Postgresql.ObjectMeta.Labels["cluster-name"]) //TODO: make this configurable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Labels we propagate to all child resources are configurable with ClusterLabels and ClusterNameLabel. We should use the latter, I think and not hard code it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have changed it, using ClusterNameLabel
pkg/util/k8sutil/k8sutil.go
Outdated
@@ -192,10 +193,16 @@ func NewFromConfig(cfg *rest.Config) (KubernetesClient, error) { | |||
} | |||
|
|||
// SetPostgresCRDStatus of Postgres cluster | |||
func (client *KubernetesClient) SetPostgresCRDStatus(clusterName spec.NamespacedName, status string) (*apiacidv1.Postgresql, error) { | |||
func (client *KubernetesClient) SetPostgresCRDStatus(clusterName spec.NamespacedName, status string, numberOfInstances int32, labelSelector string, observedGeneration int64, existingConditions apiacidv1.Conditions, message string) (*apiacidv1.Postgresql, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of extending the list of arguments we could also pass a PostgresStatus struct, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, made modifications to use the PostgresStatus struct
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. |
Signed-off-by: RavinaChidambaram <[email protected]>
I have made changes to fix the unit tests and successfully ran code-generation, unit tests and e2e test downstream. |
Signed-off-by: RavinaChidambaram <[email protected]>
Hi @FxKu , any updates? |
Fixes #2669
Following things have been added:
observedGeneration
conditions
of typeReconciliationSuccessful
andReady
numberOfInstances
andlabelSelector
for scale subresourcepostgresClusterStatus
is still part of the CRDDetailed example flow:
Conditions are updated everytime there is a change in
PostgresClusterStatus
. And if the cluster is running successfully, the status looks like: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
inconditions