Merge pull request #3 from Riskified/deployment-status

Reduce requeue and deployment status

controllers/dynamicenv_controller.go

@@ -19,10 +19,6 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"sort"
-	"strings"
-	"time"
-
 	istioapi "istio.io/api/networking/v1alpha3"
 	istionetwork "istio.io/client-go/pkg/apis/networking/v1alpha3"
 	appsv1 "k8s.io/api/apps/v1"
@@ -36,6 +32,8 @@ import (
 	ctrllog "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/source"
+	"sort"
+	"strings"
 
 	riskifiedv1alpha1 "github.com/riskified/dynamic-environment/api/v1alpha1"
 	"github.com/riskified/dynamic-environment/pkg/handlers"
@@ -312,7 +310,7 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	if nonReadyExists && rls.returnError == nil {
 		// Currently we don't get updates on resource's status changes, so we need to requeue.
 		log.V(1).Info("Requeue because of non running status")
-		return ctrl.Result{RequeueAfter: 2 * time.Second}, nil
+		return ctrl.Result{Requeue: true}, nil
 	}
 	return ctrl.Result{}, rls.returnError
 }
@@ -360,7 +358,6 @@ func (r *DynamicEnvReconciler) addFinalizersIfRequired(ctx context.Context, de *
 }
 
 func (r *DynamicEnvReconciler) cleanDynamicEnvResources(ctx context.Context, de *riskifiedv1alpha1.DynamicEnv, version string) (ctrl.Result, error) {
-	var resources int
 	log := ctrllog.FromContext(ctx)
 	log.Info("Dynamic Env marked for deletion, cleaning up ...")
 	if helpers.StringSliceContains(names.DeleteDeployments, de.Finalizers) {
@@ -369,26 +366,34 @@ func (r *DynamicEnvReconciler) cleanDynamicEnvResources(ctx context.Context, de
 			log.Error(err, "error removing cleanupDeployments finalizer")
 			return ctrl.Result{}, err
 		}
-		resources += count
+		if count == 0 {
+			if err := r.deleteFinalizer(ctx, names.DeleteDeployments, de); err != nil {
+				return ctrl.Result{}, err
+			}
+		}
 	}
 	if helpers.StringSliceContains(names.DeleteDestinationRules, de.Finalizers) {
 		count, err := r.cleanupDestinationRules(ctx, de)
 		if err != nil {
 			log.Error(err, "error removing DeleteDestinationRules finalizer")
 			return ctrl.Result{}, err
 		}
-		resources += count
+		if count == 0 {
+			if err := r.deleteFinalizer(ctx, names.DeleteDestinationRules, de); err != nil {
+				return ctrl.Result{}, err
+			}
+		}
 	}
 	if helpers.StringSliceContains(names.CleanupVirtualServices, de.Finalizers) {
 		if err := r.cleanupVirtualServices(ctx, de, version); err != nil {
 			log.Error(err, "error removing CleanupVirtualServices finalizer")
 			return ctrl.Result{}, err
 		}
+		if err := r.deleteFinalizer(ctx, names.CleanupVirtualServices, de); err != nil {
+			return ctrl.Result{}, err
+		}
 	}
 
-	if resources > 0 {
-		return ctrl.Result{RequeueAfter: 1 * time.Second}, nil
-	}
 	return ctrl.Result{}, nil
 }
 
@@ -417,10 +422,7 @@ func (r *DynamicEnvReconciler) cleanupDeployments(ctx context.Context, de *riski
 			return runningCount, fmt.Errorf("error deleting deployment %v: %w", found, err)
 		}
 	}
-	if runningCount > 0 {
-		return runningCount, nil
-	}
-	return runningCount, r.deleteFinalizer(ctx, names.DeleteDeployments, de)
+	return runningCount, nil
 }
 
 // Deletes created destination rules on controller deletion. Deletes the finalizer once all DRs are deleted. Returns the
@@ -445,7 +447,7 @@ func (r *DynamicEnvReconciler) cleanupDestinationRules(ctx context.Context, de *
 			return runningCount, fmt.Errorf("error deleting destination rule %v: %w", &found, err)
 		}
 	}
-	return runningCount, r.deleteFinalizer(ctx, names.DeleteDestinationRules, de)
+	return runningCount, nil
 }
 
 func (r *DynamicEnvReconciler) cleanupVirtualServices(ctx context.Context, de *riskifiedv1alpha1.DynamicEnv, version string) error {
@@ -476,7 +478,7 @@ func (r *DynamicEnvReconciler) cleanupVirtualServices(ctx context.Context, de *r
 			return err
 		}
 	}
-	return r.deleteFinalizer(ctx, names.CleanupVirtualServices, de)
+	return nil
 }
 
 func (r *DynamicEnvReconciler) deleteFinalizer(ctx context.Context, finalizer string, de *riskifiedv1alpha1.DynamicEnv) error {

pkg/handlers/deployment_handler.go

@@ -110,8 +110,13 @@ func (h *DeploymentHandler) GetStatus() (riskifiedv1alpha1.ResourceStatus, error
 	// h.Log.V(1).Info("status found for deployment", "deployment", searchName, "status", deployment.Status)
 
 	found, processing := getMostRecentConditionForType(appsv1.DeploymentProgressing, deployment.Status.Conditions)
-	if deployment.Status.AvailableReplicas > 0 {
-		if found && processing.Status == v1.ConditionTrue && processing.Reason != "NewReplicaSetAvailable" {
+	if !found {
+		return genStatus(riskifiedv1alpha1.Unknown), nil
+	}
+	if processing.Status == v1.ConditionTrue {
+		if processing.Reason == "NewReplicaSetAvailable" {
+			return genStatus(riskifiedv1alpha1.Running), nil
+		} else {
 			var status riskifiedv1alpha1.LifeCycleStatus
 			if h.Updating {
 				status = riskifiedv1alpha1.Updating
@@ -120,16 +125,9 @@ func (h *DeploymentHandler) GetStatus() (riskifiedv1alpha1.ResourceStatus, error
 			}
 			return genStatus(status), nil
 		}
-		return genStatus(riskifiedv1alpha1.Running), nil
 	} else {
-		if found && processing.Status == v1.ConditionTrue {
-			return genStatus(riskifiedv1alpha1.Initializing), nil
-		}
-		if found && processing.Status == v1.ConditionFalse {
-			return genStatus(riskifiedv1alpha1.Failed), nil
-		}
+		return genStatus(riskifiedv1alpha1.Failed), nil
 	}
-	return genStatus(riskifiedv1alpha1.Unknown), nil
 }
 
 func (h *DeploymentHandler) ApplyStatus(rs riskifiedv1alpha1.ResourceStatus) error {
@@ -289,8 +287,14 @@ func getMostRecentConditionForType(typ appsv1.DeploymentConditionType, condition
 	var result appsv1.DeploymentCondition
 	for _, c := range conditions {
 		if c.Type == typ {
-			found = true
-			result = c
+			if found {
+				if result.LastUpdateTime.Before(&c.LastUpdateTime) {
+					result = c
+				}
+			} else {
+				found = true
+				result = c
+			}
 		}
 	}
 	return found, result