improve deployment status report #142

eguzki · 2024-05-28T12:48:53Z

What

Fixes #140

After initial deployment, some changes in the limitador CR trigger pods rolling out to capture latest desired configuration. During that rolling out process, the deployment object's status condition Available remains true.

Deployment status object during steady state

availableReplicas: 1
conditions:
  - lastTransitionTime: "2024-05-28T11:14:18Z"
    lastUpdateTime: "2024-05-28T11:14:18Z"
    message: Deployment has minimum availability.
    reason: MinimumReplicasAvailable
    status: "True"
    type: Available
  - lastTransitionTime: "2024-05-28T11:14:07Z"
    lastUpdateTime: "2024-05-28T11:23:06Z"
    message: ReplicaSet "limitador-limitador-bd768d89d" has successfully progressed.
    reason: NewReplicaSetAvailable
    status: "True"
    type: Progressing
observedGeneration: 7
readyReplicas: 1
replicas: 1
updatedReplicas: 1

Deployment status object during pods being rolled out

availableReplicas: 1
conditions:
  - lastTransitionTime: "2024-05-28T11:14:18Z"
    lastUpdateTime: "2024-05-28T11:14:18Z"
    message: Deployment has minimum availability.
    reason: MinimumReplicasAvailable
    status: "True"
    type: Available
  - lastTransitionTime: "2024-05-28T11:14:07Z"
    lastUpdateTime: "2024-05-28T11:23:51Z"
    message: ReplicaSet "limitador-limitador-696b84d58f" is progressing.
    reason: ReplicaSetUpdated
    status: "True"
    type: Progressing
observedGeneration: 8
readyReplicas: 1
replicas: 2
unavailableReplicas: 1
updatedReplicas: 1

Few things change during rolling out. First and foremost, unavailableReplicas field is not 0. Additionally, readyReplicas is not equal to replicas.

This PR enhances controller status reporting capturing aforementioned new conditions to report limitador's state as Ready.

Verification steps

dev setup

make local-setup

Deploy limitador with defaults.

k apply -f - <<EOF
---
apiVersion: limitador.kuadrant.io/v1alpha1
kind: Limitador
metadata:
  name: limitador
spec: {}
EOF

Wait for limitador to be ready

kubectl wait --timeout=300s --for=condition=Ready limitador limitador

Check status

kubectl get limitador limitador -o jsonpath='{.status}' | yq e -P

It should report "Ready"

conditions:
  - lastTransitionTime: "2024-05-28T12:53:05Z"
    message: Limitador is ready
    reason: Ready
    status: "True"
    type: Ready
observedGeneration: 1
service:
  host: limitador-limitador.default.svc.cluster.local
  ports:
    grpc: 8081
    http: 8080

Let's update verbosity level to trigger pods rolling out. "Quickly", we need to check Limitador CR's status to verify that status reports not ready during rolling out. Eventually, status should report "Ready".

kubectl patch limitador limitador --type=merge --patch '{"spec": {"verbosity": 2}}'

Check status

kubectl get limitador limitador -o jsonpath='{.status}' | yq e -P

During rolling out,

conditions:
  - lastTransitionTime: "2024-05-28T12:58:38Z"
    message: Deployment has unavailable replicas
    reason: LimitadorNotAvailable
    status: "False"
    type: Ready
observedGeneration: 2
service:
  host: limitador-limitador.default.svc.cluster.local
  ports:
    grpc: 8081
    http: 8080

you should get Deployment has unavailable replicas. And eventually, back to Ready state.

kubectl wait --timeout=300s --for=condition=Ready limitador limitador

codecov-commenter · 2024-05-28T12:52:02Z

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 84.70%. Comparing base (8df92db) to head (1bc2191).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
- Coverage   86.06%   84.70%   -1.36%     
==========================================
  Files          19       19              
  Lines         990      994       +4     
==========================================
- Hits          852      842      -10     
- Misses         91       98       +7     
- Partials       47       54       +7

Flag	Coverage Δ
integration	`78.67% <63.63%> (-1.94%)`	⬇️
unit	`66.51% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1alpha1 (u)	`100.00% <ø> (ø)`
pkg/helpers (u)	`83.87% <ø> (ø)`
pkg/log (u)	`94.73% <ø> (ø)`
pkg/reconcilers (u)	`74.67% <ø> (ø)`
pkg/limitador (u)	`98.11% <ø> (ø)`
controllers (i)	`74.33% <63.63%> (-4.39%)`	⬇️
pkg/upgrades	`88.88% <ø> (ø)`

Files	Coverage Δ
controllers/limitador_status.go	`81.70% <63.63%> (-4.20%)`	⬇️

... and 1 file with indirect coverage changes

KevFan · 2024-06-06T09:55:51Z

controllers/limitador_status.go

+	if deployment.Status.ReadyReplicas != deployment.Status.Replicas {
+		return ptr.To("Deployment has replicas not ready yet"), nil
+	}
+


Any thoughts about an edge case where an user sets limitador.spec.replicas to 0, so deployment.status.replicas is 0. The current logic will mark the CR status as Ready: true, would this be the expected status for this edge case? 🤔

good point. I think we should follow the same approach as the replication controller for the Deployment object.

I set limtiador CR with replica: 0

spec: replicas: 0

The operator updates the deployment object to:

spec: replicas: 0 ....

And the status of the deployment is:

status: conditions: - lastTransitionTime: "2024-06-10T08:26:11Z" lastUpdateTime: "2024-06-10T08:26:11Z" message: Deployment has minimum availability. reason: MinimumReplicasAvailable status: "True" type: Available - lastTransitionTime: "2024-06-10T08:25:51Z" lastUpdateTime: "2024-06-10T08:26:11Z" message: ReplicaSet "limitador-limitador-76c6d7bd46" has successfully progressed. reason: NewReplicaSetAvailable status: "True" type: Progressing observedGeneration: 2

Note that as status.availableReplicas, status.readyReplicas, status.replicas are all 0, they are not shown in the response (considered "empty").

Yet, the status condition Available is True. Thus, we can account that as "available".

Thus, IMO, limitador CR status should also be considered "Ready". I agree, though, that "Ready" does not reflect the actual state. It would be more meaningful, "Reconciled" or "Stable" or something that means, limitador is what it should be, what the user wanted to be. When the replicas are 0, I can leave "Ready" to "True", and the message would be "Limitador is ready, but without running instances". (or something like that).

Cool, sounds good 👍

KevFan

Verified, looks good to me 👍

improve deployment status report

1bc2191

eguzki requested a review from a team May 28, 2024 12:49

eguzki marked this pull request as ready for review May 28, 2024 13:01

eguzki added the kind/bug Something isn't working label May 28, 2024

eguzki mentioned this pull request Jun 4, 2024

Limitador CR shows Ready State while Deployment is still rolling out changes #140

Closed

KevFan reviewed Jun 6, 2024

View reviewed changes

KevFan approved these changes Jun 11, 2024

View reviewed changes

eguzki merged commit 9187002 into main Jun 11, 2024
14 checks passed

eguzki deleted the improve-status-report branch June 11, 2024 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve deployment status report #142

improve deployment status report #142

eguzki commented May 28, 2024 •

edited

Loading

codecov-commenter commented May 28, 2024 •

edited

Loading

KevFan Jun 6, 2024

eguzki Jun 10, 2024

KevFan Jun 11, 2024

KevFan left a comment

improve deployment status report #142

improve deployment status report #142

Conversation

eguzki commented May 28, 2024 • edited Loading

What

Verification steps

codecov-commenter commented May 28, 2024 • edited Loading

Codecov Report

KevFan Jun 6, 2024

Choose a reason for hiding this comment

eguzki Jun 10, 2024

Choose a reason for hiding this comment

KevFan Jun 11, 2024

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

eguzki commented May 28, 2024 •

edited

Loading

codecov-commenter commented May 28, 2024 •

edited

Loading