[release-1.18] Avoid the cases when RolloutDuration is set for traffic shift (#362)

Author: houshengbo

pkg/reconciler/service/service.go

@@ -975,15 +975,16 @@ func convertIntoTrafficTarget(name string, ro *v1.RolloutOrchestrator, rc *Rollo
 
 		spa, err := spaLister.Get(spaTargetRevName)
 		// Check the number of replicas has reached the target number of replicas for the revision scaling up
-		if apierrs.IsNotFound(err) || (err == nil && targetNumberReplicas != nil && spa.Status.ActualScale != nil &&
-			minScale != nil && *targetNumberReplicas <= *minScale && *spa.Status.ActualScale < *targetNumberReplicas) {
+		if err != nil || spa.Status.ActualScale == nil || (err == nil && targetNumberReplicas != nil &&
+			spa.Status.ActualScale != nil && minScale != nil && *targetNumberReplicas <= *minScale &&
+			*spa.Status.ActualScale < *targetNumberReplicas) {
 			// If we have issues getting the spa, or the number of the replicas has reached the target number of
 			// the revision to scale up, we set the revisionTarget to ro.Spec.StageTargetRevisions.
 
 			// However, if there is no issue getting yhe spa, and the actual number of replicas is less than
 			// the target number of replicas, we need to use ro.Status.StageRevisionStatus or route.Status.Traffic
 			// as the traffic information for the route.
-			if strings.EqualFold(rc.ProgressiveRolloutStrategy, strategies.AvailabilityStrategy) {
+			if strings.EqualFold(rc.ProgressiveRolloutStrategy, strategies.AvailabilityStrategy) && rc.RolloutDuration == "0" {
 				// Comment out the following lines for further consideration with resourceUtil mode
 				// || (strings.EqualFold(rc.ProgressiveRolloutStrategy, strategies.ResourceUtilStrategy) && !trafficDriven && !lastStage) {
 				if len(ro.Status.StageRevisionStatus) > 0 {
@@ -997,7 +998,7 @@ func convertIntoTrafficTarget(name string, ro *v1.RolloutOrchestrator, rc *Rollo
 						}
 						revisionTarget[index].LatestRevision = ptr.Bool(false)
 					}
-					if !found {
+					if !found && spa != nil && spa.Status.ActualScale != nil && *spa.Status.ActualScale > 0 {
 						// We must assign the traffic to the new revision, even of it is 0%. Otherwise, the PA will
 						// sometimes report the error of "No traffic. The target is not receiving traffic", which
 						// will kill the pods of the new revision during the progressive rollout.
@@ -1007,37 +1008,6 @@ func convertIntoTrafficTarget(name string, ro *v1.RolloutOrchestrator, rc *Rollo
 						newRevisionTarget.Percent = ptr.Int64(int64(0))
 						revisionTarget = append(revisionTarget, newRevisionTarget)
 					}
-				} else {
-					// If the ro does not have the StageRevisionStatus in the status, use the existing one in route.
-					route, errRoute := routeLister.Get(ro.Name)
-					if errRoute == nil && len(route.Status.Traffic) > 0 {
-						traffics := route.Status.Traffic
-						found := false
-						for index := range traffics {
-							if traffics[index].RevisionName == spaTargetRevName {
-								found = true
-								continue
-							}
-							traffics[index].LatestRevision = ptr.Bool(false)
-						}
-						if !found {
-							// We must assign the traffic to the new revision, even of it is 0%. Otherwise, the PA will
-							// sometimes report the error of "No traffic. The target is not receiving traffic", which
-							// will kill the pods of the new revision during the progressive rollout.
-							newRevisionTarget := ro.Spec.StageTargetRevisions[len(ro.Spec.StageTargetRevisions)-1]
-							newRevisionTarget.Percent = ptr.Int64(int64(0))
-
-							newRevisionTraffic := servingv1.TrafficTarget{
-								ConfigurationName: name,
-								LatestRevision:    newRevisionTarget.LatestRevision,
-								Percent:           newRevisionTarget.Percent,
-								Tag:               newRevisionTarget.Tag,
-								URL:               newRevisionTarget.URL,
-							}
-							traffics = append(traffics, newRevisionTraffic)
-						}
-						return traffics
-					}
 				}
 			}
 		}

pkg/reconciler/service/service_test.go

@@ -958,6 +958,7 @@ func TestTransformService(t *testing.T) {
 		rc: &RolloutConfig{
 			ProgressiveRolloutEnabled:  true,
 			ProgressiveRolloutStrategy: strategies.AvailabilityStrategy,
+			RolloutDuration:            "0",
 		},
 		service: &servingv1.Service{
 			ObjectMeta: metav1.ObjectMeta{
@@ -1018,12 +1019,12 @@ func TestTransformService(t *testing.T) {
 						{
 							RevisionName:   "rev-001",
 							LatestRevision: ptr.Bool(false),
-							Percent:        ptr.Int64(100),
+							Percent:        ptr.Int64(80),
 						},
 						{
 							ConfigurationName: "test-name",
 							LatestRevision:    ptr.Bool(true),
-							Percent:           ptr.Int64(0),
+							Percent:           ptr.Int64(20),
 						},
 					},
 				},
@@ -1058,13 +1059,13 @@ func TestTransformService(t *testing.T) {
 					Traffic: []servingv1.TrafficTarget{
 						{
 							RevisionName:   "rev-001",
-							Percent:        ptr.Int64(90),
+							Percent:        ptr.Int64(80),
 							LatestRevision: ptr.Bool(false),
 						},
 						{
-							RevisionName:   "rev-002",
-							Percent:        ptr.Int64(10),
-							LatestRevision: ptr.Bool(true),
+							ConfigurationName: "test-name",
+							Percent:           ptr.Int64(20),
+							LatestRevision:    ptr.Bool(true),
 						},
 					},
 				},