Skip to content
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

Wait until Deployment Status Ready #65

Merged
merged 3 commits into from
May 26, 2022
Merged

Conversation

guicassolato
Copy link
Collaborator

@guicassolato guicassolato commented May 24, 2022

Closes #63

Verification steps

Deploy this branch:

kind create cluster --name authorino
make docker-build OPERATOR_IMAGE=authorino-operator:local
kind load docker-image authorino-operator:local --name authorino
kubectl create namespace authorino-operator
make install deploy OPERATOR_IMAGE=authorino-operator:localmake install deploy OPERATOR_IMAGE=authorino-operator:local

Create an Authorino instance whose Deployment will fail:

kubectl apply -f -<<EOF
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
spec:
  clusterWide: true
  image: authorino:wrong
  listener:
    tls:
      enabled: false
  oidcServer:
    tls:
      enabled: false
EOF

Check the status of the Authorino CR:

kubectl get authorino/authorino -o jsonpath='{.status.conditions}'
# [{"lastTransitionTime":"2022-05-26T16:11:27Z","message":"Authorino Deployment resource not ready","reason":"DeploymentNotReady","status":"False","type":"Ready"}]

Fix the Deployment:

kubectl apply -f -<<EOF
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
spec:
  clusterWide: true
  image: quay.io/kuadrant/authorino:latest
  listener:
    tls:
      enabled: false
  oidcServer:
    tls:
      enabled: false
EOF

Check the status of the Authorino CR again:

kubectl get authorino/authorino -o jsonpath='{.status.conditions}'
# [{"lastTransitionTime":"2022-05-26T16:11:50Z","reason":"Provisioned","status":"True","type":"Ready"}]

@guicassolato guicassolato self-assigned this May 24, 2022
@guicassolato guicassolato changed the title Wait until Deployment Status Ready to update the status of the CR Wait until Deployment Status Ready May 24, 2022
@guicassolato guicassolato requested a review from eguzki May 24, 2022 15:58
@guicassolato guicassolato requested a review from eguzki May 26, 2022 14:23
eguzki
eguzki previously approved these changes May 26, 2022
@@ -161,6 +165,7 @@ func (r *AuthorinoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
// SetupWithManager sets up the controller with the Manager.
func (r *AuthorinoReconciler) SetupWithManager(mgr ctrl.Manager) error {
return ctrl.NewControllerManagedBy(mgr).
Owns(&k8sapps.Deployment{}).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if adding watcher to owned deployments, there is no need to requeue when the deployment is not ready.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do not return in line

return ctrl.Result{Requeue: true}, nil
the reconciler will set the status of the CR to ready in line
return ctrl.Result{}, updateStatusConditions(logger, authorinoInstance, r.Client, statusReady())
regardless of the status of the deployment, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, instead of return ctrl.Result{Requeue: true}, nil , it should be:

return ctrl.Result{}, updateStatusConditions(logger, authorinoInstance, r.Client, statusNotReady())

Anyway, leave it as it is. It is working as expected

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new status condition DeploymentNotReady (message: 'Authorino Deployment resource not ready'), @eguzki

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think you need to requeue when deployments are not ready because the controller is watching for deployments updates.

But leave it as it is, it does not do any harm

@guicassolato guicassolato merged commit 1ba337d into main May 26, 2022
@guicassolato guicassolato deleted the wait-deployment-status branch May 26, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

authorino CR status conditions does not reflect deployment status
2 participants