From 025d51dfd462cc971ed554b9c7204f19b8f132e0 Mon Sep 17 00:00:00 2001 From: Gunjan Vyas Date: Mon, 7 Mar 2022 14:25:13 +0530 Subject: [PATCH 1/4] Traffic split should consider only the active revisions --- pkg/kn/traffic/compute.go | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/pkg/kn/traffic/compute.go b/pkg/kn/traffic/compute.go index fae87ef5b8..2a0caa443b 100644 --- a/pkg/kn/traffic/compute.go +++ b/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 - var existingRevisionCount = len(revisions) + // number of existing targets + 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 { - if !checkRevisionPresent(revisionRefMap, rev) { + for _, target := range targets { + 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 } From 84b582db0afe9e7e6228ea8a3ba06cb6df20ccd5 Mon Sep 17 00:00:00 2001 From: Gunjan Vyas Date: Mon, 7 Mar 2022 15:38:27 +0530 Subject: [PATCH 2/4] Modified unit test for traffic split change --- pkg/kn/traffic/compute_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/kn/traffic/compute_test.go b/pkg/kn/traffic/compute_test.go index 5ffce1ed65..bd0067894c 100644 --- a/pkg/kn/traffic/compute_test.go +++ b/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) }) } } From fc7627706f8f2d50b9dff2d95d509b80785fbb2e Mon Sep 17 00:00:00 2001 From: Gunjan Vyas Date: Tue, 8 Mar 2022 15:59:59 +0530 Subject: [PATCH 3/4] Fix e2e test case --- test/e2e/traffic_split_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go index 42684ee8a6..d9d558ac87 100644 --- a/test/e2e/traffic_split_test.go +++ b/test/e2e/traffic_split_test.go @@ -194,8 +194,8 @@ func TestTrafficSplit(t *testing.T) { test.ServiceCreate(r, serviceName) - test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1") - test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2") + test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1", "--traffic", "40") + test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2", "--traffic", fmt.Sprintf("%s=%d", rev1, 50)) test.ServiceUpdateWithError(r, serviceName, "--traffic", fmt.Sprintf("%s=%d", rev1, 50)) test.ServiceDelete(r, serviceName) From 47109ee27bbb1292efc10a75980dd7869bc8bebd Mon Sep 17 00:00:00 2001 From: Gunjan Vyas Date: Tue, 8 Mar 2022 17:00:07 +0530 Subject: [PATCH 4/4] e2e test fix --- test/e2e/traffic_split_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go index d9d558ac87..46d0204fcd 100644 --- a/test/e2e/traffic_split_test.go +++ b/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", "--traffic", "40") - test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2", "--traffic", fmt.Sprintf("%s=%d", rev1, 50)) + 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)