[release-1.18] Assigned 0% traffic to the new revision before reaching the minimum replicas (#345)

Assigned 0% traffic to the new revision before reaching the minimum replicas

Fixed: #335

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

Author: knative-prow-robot

pkg/reconciler/service/service.go

@@ -989,23 +989,53 @@ func convertIntoTrafficTarget(name string, ro *v1.RolloutOrchestrator, rc *Rollo
 				if len(ro.Status.StageRevisionStatus) > 0 {
 					// If the ro has the StageRevisionStatus in the status, use it.
 					revisionTarget = ro.Status.StageRevisionStatus
+					found := false
 					for index := range revisionTarget {
 						if revisionTarget[index].RevisionName == spaTargetRevName {
+							found = true
 							continue
 						}
 						revisionTarget[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.
+						// The last one of ro.Spec.StageTargetRevisions is certainly the RevisionTarget for the
+						// new revision,
+						newRevisionTarget := ro.Spec.StageTargetRevisions[len(ro.Spec.StageTargetRevisions)-1]
+						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

@@ -911,7 +911,9 @@ func TestTransformService(t *testing.T) {
 		spaLister: MockSPALister{
 			ActualScale: ptr.Int32(2),
 		},
-		routeLister: MockRouteLister{},
+		routeLister: MockRouteLister{
+			WithNewRev: true,
+		},
 		rc: &RolloutConfig{
 			ProgressiveRolloutEnabled:  true,
 			ProgressiveRolloutStrategy: strategies.AvailabilityStrategy,
@@ -950,7 +952,9 @@ func TestTransformService(t *testing.T) {
 		spaLister: MockSPALister{
 			ActualScale: ptr.Int32(1),
 		},
-		routeLister: MockRouteLister{},
+		routeLister: MockRouteLister{
+			WithNewRev: true,
+		},
 		rc: &RolloutConfig{
 			ProgressiveRolloutEnabled:  true,
 			ProgressiveRolloutStrategy: strategies.AvailabilityStrategy,
@@ -975,6 +979,52 @@ func TestTransformService(t *testing.T) {
 							LatestRevision: ptr.Bool(false),
 							Percent:        ptr.Int64(100),
 						},
+						{
+							ConfigurationName: "test-name",
+							LatestRevision:    ptr.Bool(true),
+							Percent:           ptr.Int64(0),
+						},
+					},
+				},
+			},
+		},
+	}, {
+		name: "Test with StageTargetRevisions with revision name for the latest revision, with no ro status, but route status",
+		spaLister: MockSPALister{
+			ActualScale: ptr.Int32(1),
+		},
+		routeLister: MockRouteLister{
+			WithNewRev: false,
+		},
+		rc: &RolloutConfig{
+			ProgressiveRolloutEnabled:  true,
+			ProgressiveRolloutStrategy: strategies.AvailabilityStrategy,
+		},
+		service: &servingv1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-name",
+				Namespace: "test-ns",
+			},
+		},
+		ro: MockRolloutOrchestratorNoStatus,
+		ExpectedService: &servingv1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-name",
+				Namespace: "test-ns",
+			},
+			Spec: servingv1.ServiceSpec{
+				RouteSpec: servingv1.RouteSpec{
+					Traffic: []servingv1.TrafficTarget{
+						{
+							RevisionName:   "rev-001",
+							LatestRevision: ptr.Bool(false),
+							Percent:        ptr.Int64(100),
+						},
+						{
+							ConfigurationName: "test-name",
+							LatestRevision:    ptr.Bool(true),
+							Percent:           ptr.Int64(0),
+						},
 					},
 				},
 			},
@@ -984,7 +1034,9 @@ func TestTransformService(t *testing.T) {
 		spaLister: MockSPALister{
 			ActualScale: ptr.Int32(1),
 		},
-		routeLister: MockRouteLister{},
+		routeLister: MockRouteLister{
+			WithNewRev: true,
+		},
 		rc: &RolloutConfig{
 			ProgressiveRolloutEnabled:  true,
 			ProgressiveRolloutStrategy: strategies.AvailabilityStrategy,
@@ -1024,7 +1076,9 @@ func TestTransformService(t *testing.T) {
 			ActualScale: ptr.Int32(1),
 			Err:         fmt.Errorf("Unable to find the resource"),
 		},
-		routeLister: MockRouteLister{},
+		routeLister: MockRouteLister{
+			WithNewRev: true,
+		},
 		rc: &RolloutConfig{
 			ProgressiveRolloutEnabled:  true,
 			ProgressiveRolloutStrategy: strategies.AvailabilityStrategy,
@@ -1088,25 +1142,41 @@ func (spaLister MockSPALister) Get(_ string) (*v1.StagePodAutoscaler, error) {
 }
 
 type MockRouteLister struct {
+	WithNewRev bool
 }
 
 func (routeLister MockRouteLister) List(_ labels.Selector) (ret []*servingv1.Route, err error) {
 	return nil, nil
 }
 
 func (routeLister MockRouteLister) Get(_ string) (*servingv1.Route, error) {
+	if routeLister.WithNewRev {
+		return &servingv1.Route{
+			Status: servingv1.RouteStatus{
+				RouteStatusFields: servingv1.RouteStatusFields{
+					Traffic: []servingv1.TrafficTarget{
+						{
+							RevisionName:   "rev-001",
+							Percent:        ptr.Int64(90),
+							LatestRevision: ptr.Bool(false),
+						},
+						{
+							RevisionName:   "rev-002",
+							Percent:        ptr.Int64(10),
+							LatestRevision: ptr.Bool(true),
+						},
+					},
+				},
+			},
+		}, nil
+	}
 	return &servingv1.Route{
 		Status: servingv1.RouteStatus{
 			RouteStatusFields: servingv1.RouteStatusFields{
 				Traffic: []servingv1.TrafficTarget{
 					{
 						RevisionName:   "rev-001",
-						Percent:        ptr.Int64(90),
-						LatestRevision: ptr.Bool(false),
-					},
-					{
-						RevisionName:   "rev-002",
-						Percent:        ptr.Int64(10),
+						Percent:        ptr.Int64(100),
 						LatestRevision: ptr.Bool(true),
 					},
 				},