[release-1.14] Updated the status before returning nil in reconcile (#196)

* Removed the call of resetObsoleteSPAs tentatively

* Updated the status before returning nil in reconcile

* Remove the revision if it is not in InitialRevisions and StageTargetRevisions

---------

Co-authored-by: Vincent Hou <shou73@bloomberg.net>

Author: knative-prow-robot

pkg/reconciler/rolloutorchestrator/rolloutorchestrator.go

@@ -120,18 +120,28 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ro *v1.RolloutOrchestrat
 		ro.Spec.TargetRevisions) {
 		// Start to move to the next stage.
 		ro.Status.LaunchNewStage()
+		return nil
 	}
 
+	ro.Status.MarkStageRevisionScaleUpReady()
+	ro.Status.MarkStageRevisionScaleDownReady()
+	ro.Status.MarkStageRevisionReady()
+	ro.Status.MarkLastStageRevisionComplete()
 	return nil
 }
 
 // resetObsoleteSPAs will set the StageMinScale to 0 and StageMaxScale to 1, if the revision with this spa is
 // not in ro.Spec.StageTargetRevisions.
 func (r *Reconciler) resetObsoleteSPAs(ctx context.Context, ro *v1.RolloutOrchestrator) error {
-	records := map[string]bool{}
+	records, recordsIni := map[string]bool{}, map[string]bool{}
 	for _, rev := range ro.Spec.StageTargetRevisions {
 		records[rev.RevisionName] = true
 	}
+
+	for _, rev := range ro.Spec.InitialRevisions {
+		recordsIni[rev.RevisionName] = true
+	}
+
 	// Get the list of all the SPAs for the knative service.
 	spaList, err := r.stagePodAutoscalerLister.StagePodAutoscalers(ro.Namespace).List(labels.SelectorFromSet(labels.Set{
 		serving.ServiceLabelKey: ro.Name,
@@ -142,9 +152,9 @@ func (r *Reconciler) resetObsoleteSPAs(ctx context.Context, ro *v1.RolloutOrches
 		return err
 	}
 	for _, spa := range spaList {
-		// The SPA and the revision share the same name. If the revision is not in the StageTargetRevisions,
-		// update the SPA to make sure the revision scaling down to 0.
-		if !records[spa.Name] && (spa.Status.DesiredScale == nil || *spa.Status.DesiredScale != 0) {
+		// The SPA and the revision share the same name. If the revision is not in the StageTargetRevisions and not in
+		// InitialRevisions, update the SPA to make sure the revision scaling down to 0.
+		if !records[spa.Name] && !recordsIni[spa.Name] && (spa.Status.DesiredScale == nil || *spa.Status.DesiredScale != 0) {
 			spa.Spec.StageMinScale = ptr.Int32(0)
 			spa.Spec.StageMaxScale = ptr.Int32(1)
 			_, err = r.client.ServingV1().StagePodAutoscalers(ro.Namespace).Update(ctx, spa, metav1.UpdateOptions{})

pkg/reconciler/rolloutorchestrator/strategies/base.go

@@ -52,7 +52,7 @@ type RolloutStep interface {
 		enqueueAfter func(interface{}, time.Duration)) (bool, error)
 	// ModifyStatus function changes the status of the ro accordingly after the completion of the scaling up or down for the
 	// revisions.
-	ModifyStatus(ro *v1.RolloutOrchestrator)
+	ModifyStatus(ro *v1.RolloutOrchestrator, ready bool)
 }
 
 // The BaseScaleStep struct defines golang clients, that are necessary to access the kubernetes resources.

pkg/reconciler/rolloutorchestrator/strategies/strategy.go

@@ -54,9 +54,8 @@ func (r *Rollout) Reconcile(ctx context.Context, ro *v1.RolloutOrchestrator, rev
 			if err != nil {
 				return false, err
 			}
-			if ready {
-				step.ModifyStatus(ro)
-			} else {
+			step.ModifyStatus(ro, ready)
+			if !ready {
 				return false, nil
 			}
 		}
@@ -111,8 +110,14 @@ func (s *ScaleUpStep) Verify(ctx context.Context, ro *v1.RolloutOrchestrator, re
 
 // ModifyStatus for ScaleUpStep modifies the status of the rolloutOrchestrator after the new revision has scaled up to
 // the expected number of pods.
-func (s *ScaleUpStep) ModifyStatus(ro *v1.RolloutOrchestrator) {
-	ro.Status.MarkStageRevisionScaleUpReady()
+func (s *ScaleUpStep) ModifyStatus(ro *v1.RolloutOrchestrator, ready bool) {
+	if ready {
+		ro.Status.MarkStageRevisionScaleUpReady()
+	} else {
+		ro.Status.MarkStageRevisionScaleUpInProgress(v1.StageRevisionStart, v1.RolloutNewStage)
+		ro.Status.MarkStageRevisionInProgress(v1.StageRevisionStart, v1.RolloutNewStage)
+		ro.Status.MarkLastStageRevisionInComplete()
+	}
 }
 
 // The ScaleDownStep struct is responsible for scaling down the pods for the old revisions.
@@ -268,8 +273,14 @@ func (s *ScaleDownStep) Verify(ctx context.Context, ro *v1.RolloutOrchestrator,
 
 // ModifyStatus for ScaleDownStep modifies the status of the rolloutOrchestrator after the old revision has scaled down to
 // the expected number of pods.
-func (s *ScaleDownStep) ModifyStatus(ro *v1.RolloutOrchestrator) {
-	ro.Status.MarkStageRevisionScaleDownReady()
+func (s *ScaleDownStep) ModifyStatus(ro *v1.RolloutOrchestrator, ready bool) {
+	if ready {
+		ro.Status.MarkStageRevisionScaleDownReady()
+	} else {
+		ro.Status.MarkStageRevisionScaleDownInProgress(v1.StageRevisionStart, v1.RolloutNewStage)
+		ro.Status.MarkStageRevisionInProgress(v1.StageRevisionStart, v1.RolloutNewStage)
+		ro.Status.MarkLastStageRevisionInComplete()
+	}
 }
 
 func NewRolloutStrategy(client clientset.Interface, kubeclient kubernetes.Interface, stagePodAutoscalerLister listers.StagePodAutoscalerLister) map[string]*Rollout {