Traffic split auto redirection should only consider the active revisions (#1617)

* Traffic split should consider only the active revisions

* Modified unit test for traffic split change

* Fix e2e test case

* e2e test fix

pkg/kn/traffic/compute.go

@@ -242,12 +242,12 @@ func errorSumGreaterThan100(sum int) error {
 // - if provided traffic portion are integers
 // - if traffic as per flags sums to 100
 // - if traffic as per flags < 100, can remaining traffic be automatically directed
-func verifyInput(trafficFlags *flags.Traffic, svc *servingv1.Service, revisions []servingv1.Revision, mutation bool) error {
+func verifyInput(trafficFlags *flags.Traffic, svc *servingv1.Service, targets []servingv1.TrafficTarget, existingRevisions []servingv1.Revision, mutation bool) error {
 	// check if traffic is being sent to @latest tag
 	var latestRefTraffic bool
 
-	// number of existing revisions
+	// number of existing targets
-	var existingRevisionCount = len(revisions)
+	var existingRevisionCount = len(targets)
 
 	err := verifyLatestTag(trafficFlags)
 	if err != nil {
@@ -272,7 +272,7 @@ func verifyInput(trafficFlags *flags.Traffic, svc *servingv1.Service, revisions
 		latestRefTraffic = true
 	}
 
-	// number of revisions specified in traffic flags
+	// number of targets specified in traffic flags
 	specRevPercentCount := len(trafficFlags.RevisionsPercentages)
 
 	// no traffic to route
@@ -307,8 +307,12 @@ func verifyInput(trafficFlags *flags.Traffic, svc *servingv1.Service, revisions
 	}
 
 	// remaining % to 100
-	for _, rev := range revisions {
+	for _, target := range targets {
-		if !checkRevisionPresent(revisionRefMap, rev) {
+		rev := getRevisionFromList(existingRevisions, target.RevisionName)
+		if rev == nil {
+			break
+		}
+		if !checkRevisionPresent(revisionRefMap, *rev) {
 			trafficFlags.RevisionsPercentages = append(trafficFlags.RevisionsPercentages, fmt.Sprintf("%s=%d", rev.Name, 100-sum))
 			return nil
 		}
@@ -317,6 +321,17 @@ func verifyInput(trafficFlags *flags.Traffic, svc *servingv1.Service, revisions
 	return errorTrafficDistribution(sum, errorDistributionRevisionNotFound)
 }
 
+func getRevisionFromList(revisions []servingv1.Revision, name string) *servingv1.Revision {
+	var revisionWithName *servingv1.Revision
+	for _, rev := range revisions {
+		if rev.Name == name {
+			revisionWithName = rev.DeepCopy()
+			break
+		}
+	}
+	return revisionWithName
+}
+
 func verifyRevisionSumAndReferences(trafficFlags *flags.Traffic) (revisionRefMap map[string]int, sum int, err error) {
 
 	revisionRefMap = make(map[string]int)
@@ -384,10 +399,12 @@ func checkRevisionPresent(refMap map[string]int, rev servingv1.Revision) bool {
 // total traffic per flags < 100, the params 'revisions' and 'mutation' are used to direct remaining
 // traffic. Param 'mutation' is set to true if a new revision will be created on service update
 func Compute(cmd *cobra.Command, svc *servingv1.Service,
-	trafficFlags *flags.Traffic, revisions []servingv1.Revision, mutation bool) ([]servingv1.TrafficTarget, error) {
+	trafficFlags *flags.Traffic, allRevisions []servingv1.Revision, mutation bool) ([]servingv1.TrafficTarget, error) {
 	targets := svc.Spec.Traffic
 	serviceName := svc.Name
-	err := verifyInput(trafficFlags, svc, revisions, mutation)
+	revisions := svc.Status.Traffic
+
+	err := verifyInput(trafficFlags, svc, revisions, allRevisions, mutation)
 	if err != nil {
 		return nil, err
 	}

pkg/kn/traffic/compute_test.go

@@ -82,6 +82,7 @@ func getService(name, latestRevision string, existingTraffic ServiceTraffic) *se
 		},
 	}}
 	service.Status.LatestReadyRevisionName = latestRevision
+	service.Status.Traffic = existingTraffic
 	return service
 }
 
@@ -553,7 +554,7 @@ func TestComputeErrMsg(t *testing.T) {
 		{
 			name:            "traffic split sum < 100 error when remaining revision not found",
 			existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("rev-00003", "rev-00001", 0, false), newTarget("", "rev-00002", 0, false), newTarget("", "rev-00003", 100, true)),
-			inputFlags:      []string{"--traffic", "rev-00003=10,rev-00002=20"},
+			inputFlags:      []string{"--traffic", "rev-00003=10,rev-00001=20"},
 			errMsg:          errorTrafficDistribution(30, errorDistributionRevisionNotFound).Error(),
 			existingRevisions: []servingv1.Revision{{
 				ObjectMeta: metav1.ObjectMeta{
@@ -565,13 +566,6 @@ func TestComputeErrMsg(t *testing.T) {
 						revision.RevisionTagsAnnotation: "rev-00003",
 					},
 				},
-			}, {
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "rev-00002",
-					Labels: map[string]string{
-						"serving.knative.dev/service": "serviceName",
-					},
-				},
 			}, {
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "rev-00003",
@@ -588,7 +582,7 @@ func TestComputeErrMsg(t *testing.T) {
 			testCmd.Execute()
 			svc := getService("serviceName", testCase.latestRevision, testCase.existingTraffic)
 			_, err := Compute(testCmd, svc, tFlags, testCase.existingRevisions, testCase.mutation)
-			assert.Error(t, err, testCase.errMsg)
+			assert.ErrorContains(t, err, testCase.errMsg)
 		})
 	}
 }

test/e2e/traffic_split_test.go

@@ -138,7 +138,7 @@ func TestTrafficSplit(t *testing.T) {
 
 		serviceName := test.GetNextServiceName(serviceBase)
 		test.ServiceCreate(r, serviceName)
-		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1")
+		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1", "--traffic", "80")
 
 		rev1 := fmt.Sprintf("%s-00001", serviceName)
 		rev2 := fmt.Sprintf("%s-00002", serviceName)
@@ -191,11 +191,12 @@ func TestTrafficSplit(t *testing.T) {
 		serviceName := test.GetNextServiceName(serviceBase)
 
 		rev1 := fmt.Sprintf("%s-00001", serviceName)
+		rev2 := fmt.Sprintf("%s-00002", serviceName)
 
 		test.ServiceCreate(r, serviceName)
 
-		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1")
+		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1", "--traffic", "40")
-		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2")
+		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2", "--traffic", fmt.Sprintf("%s=%d,%s=%d", rev1, 10, rev2, 20))
 		test.ServiceUpdateWithError(r, serviceName, "--traffic", fmt.Sprintf("%s=%d", rev1, 50))
 
 		test.ServiceDelete(r, serviceName)
@@ -209,7 +210,7 @@ func TestTrafficSplit(t *testing.T) {
 
 		test.ServiceCreate(r, serviceName)
 
-		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1")
+		test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1", "--traffic", "20")
 		test.ServiceUpdateWithError(r, serviceName, "--env", "TARGET=v2", "--traffic", "@latest=50")
 
 		test.ServiceDelete(r, serviceName)