From d8d0ee1e6e1013a9ed69b9328fc3c5fcf27a5dc6 Mon Sep 17 00:00:00 2001 From: Gunjan Vyas Date: Sat, 30 Oct 2021 01:54:59 +0530 Subject: [PATCH] Calculate traffic split when N-1 revisions are specified (#1483) * Calculate traffic split when N-1 revisions are specified * Added test cases for traffic split * enhanced error messages and added example * added e2e test * refactored verifyInput function * Update docs/cmd/kn_service_update.md Co-authored-by: Navid Shaikh * Update pkg/kn/commands/service/update.go Co-authored-by: Navid Shaikh * added unit test for revision list error Co-authored-by: Navid Shaikh --- docs/cmd/kn_service_update.md | 4 + pkg/kn/commands/service/update.go | 10 +- pkg/kn/commands/service/update_test.go | 32 ++ pkg/kn/traffic/compute.go | 156 +++++++-- pkg/kn/traffic/compute_test.go | 441 ++++++++++++++++--------- test/e2e/traffic_split_test.go | 17 + 6 files changed, 463 insertions(+), 197 deletions(-) diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index ab8c78357..7aef6756c 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -30,6 +30,10 @@ kn service update NAME # Add tag 'test' to echo-v3 revision with 10% traffic and rest to latest ready revision of service kn service update svc --tag echo-v3=test --traffic test=10,@latest=90 + # Split 50% traffic to stable, 40% traffic to staging and the + # rest will automatically be directed to echo-v3 (the remaining revision) + kn service update svc --traffic stable=50,staging=40 + # Update the service in offline mode instead of kubernetes cluster kn service update gitopstest -n test-ns --env KEY1=VALUE1 --target=/user/knfiles kn service update gitopstest --env KEY1=VALUE1 --target=/user/knfiles/test.yaml diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go index 39f0f2238..424e246b4 100644 --- a/pkg/kn/commands/service/update.go +++ b/pkg/kn/commands/service/update.go @@ -51,6 +51,10 @@ var updateExample = ` # Add tag 'test' to echo-v3 revision with 10% traffic and rest to latest ready revision of service kn service update svc --tag echo-v3=test --traffic test=10,@latest=90 + # Split 50% traffic to stable, 40% traffic to staging and the + # rest will automatically be directed to echo-v3 (the remaining revision) + kn service update svc --traffic stable=50,staging=40 + # Update the service in offline mode instead of kubernetes cluster kn service update gitopstest -n test-ns --env KEY1=VALUE1 --target=/user/knfiles kn service update gitopstest --env KEY1=VALUE1 --target=/user/knfiles/test.yaml @@ -99,7 +103,11 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command { } if trafficFlags.Changed(cmd) { - traffic, err := traffic.Compute(cmd, service.Spec.Traffic, &trafficFlags, service.Name) + revisions, err := client.ListRevisions(cmd.Context(), clientservingv1.WithService(service.Name)) + if err != nil { + return nil, err + } + traffic, err := traffic.Compute(cmd, service.Spec.Traffic, &trafficFlags, service.Name, revisions.Items) if err != nil { return nil, err } diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index 067b61805..a944d9a48 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -98,6 +98,30 @@ func fakeServiceUpdate(original *servingv1.Service, args []string) ( } return true, rev, nil }) + fakeServing.AddReactor("list", "revisions", + func(a clienttesting.Action) (bool, runtime.Object, error) { + var err error + listAction, ok := a.(clienttesting.ListAction) + if !ok { + return true, nil, fmt.Errorf("wrong kind of action %v", action) + } + if reqs, _ := listAction.GetListRestrictions().Labels.Requirements(); len(reqs) == 1 { + if strings.Contains(reqs[0].String(), "foo-err") { + err = fmt.Errorf("revision list error") + } + } + rev := &servingv1.Revision{} + rev.Spec = original.Spec.Template.Spec + rev.ObjectMeta = original.Spec.Template.ObjectMeta + rev.Name = original.Status.LatestCreatedRevisionName + rev.Status.ContainerStatuses = []servingv1.ContainerStatus{ + {ImageDigest: exampleImageByDigest, Name: "user-container"}, + } + revList := &servingv1.RevisionList{ + Items: []servingv1.Revision{*rev}, + } + return true, revList, err + }) if sync { fakeServing.AddWatchReactor("services", @@ -1019,6 +1043,14 @@ func TestServiceUpdateTagDoesNotExist(t *testing.T) { assert.Assert(t, util.ContainsAll(err.Error(), "tag(s)", "foo", "not present", "service", "foo")) } +func TestServiceUpdateRevisionListError(t *testing.T) { + orig := newEmptyService() + orig.Name = "foo-err" + _, _, _, err := fakeServiceUpdate(orig, []string{"service", "update", "foo-err", "--traffic", "@latest=100"}) + + assert.Assert(t, util.ContainsAll(err.Error(), "revision list error")) +} + func newEmptyService() *servingv1.Service { ret := &servingv1.Service{ TypeMeta: metav1.TypeMeta{ diff --git a/pkg/kn/traffic/compute.go b/pkg/kn/traffic/compute.go index f924d5049..17c260100 100644 --- a/pkg/kn/traffic/compute.go +++ b/pkg/kn/traffic/compute.go @@ -20,6 +20,7 @@ import ( "strings" "github.com/spf13/cobra" + "knative.dev/client/pkg/kn/commands/revision" "knative.dev/pkg/ptr" servingv1 "knative.dev/serving/pkg/apis/serving/v1" @@ -28,6 +29,12 @@ import ( var latestRevisionRef = "@latest" +const ( + errorDistributionRevisionCount = iota + errorDistributionLatestTag + errorDistributionRevisionNotFound +) + // ServiceTraffic type for operating on service traffic targets type ServiceTraffic []servingv1.TrafficTarget @@ -212,14 +219,116 @@ func errorRepeatingRevision(forFlag string, name string) error { "is not allowed, use only once with %s flag", name, forFlag) } +func errorTrafficDistribution(sum int, reason int) error { + errMsg := "" + switch reason { + case errorDistributionRevisionCount: + errMsg = "incorrect number of revisions specified. Only 1 revision should be missing" + case errorDistributionLatestTag: + errMsg = "cannot determine traffic split when @latest tag is specified" + case errorDistributionRevisionNotFound: + errMsg = "cannot determine the missing revision" + } + return fmt.Errorf("unable to allocate the remaining traffic %d. %s", 100-sum, errMsg) +} + +func errorSumGreaterThan100(sum int) error { + return fmt.Errorf("given traffic percents sum to %d, want <=100", sum) +} + // verifies if user has repeated @latest field in --tag or --traffic flags // verifyInput checks: // - if user has repeated @latest field in --tag or --traffic flags // - if provided traffic portion are integers -func verifyInput(trafficFlags *flags.Traffic) error { - var latestRevisionTag = false - var sum = 0 +func verifyInput(trafficFlags *flags.Traffic, revisions []servingv1.Revision) error { + // check if traffic is being sent to @latest tag + var latestRefTraffic bool + // number of revisions + var revisionCount = len(revisions) + + err := verifyLatestTag(trafficFlags) + if err != nil { + return err + } + + revisionRefMap, sum, err := verifyRevisionSumAndReferences(trafficFlags) + if err != nil { + return err + } + + if _, ok := revisionRefMap[latestRevisionRef]; ok { + // traffic has been routed to @latest tag + latestRefTraffic = true + } + + // number of revisions specified in traffic flags + specRevPercentCount := len(trafficFlags.RevisionsPercentages) + // equivalent check for `cmd.Flags().Changed("traffic")` as we don't have `cmd` in this function + if specRevPercentCount > 0 { + if sum > 100 { + return errorSumGreaterThan100(sum) + } + if sum < 100 && specRevPercentCount != revisionCount-1 { + return errorTrafficDistribution(sum, errorDistributionRevisionCount) + } + if sum < 100 && specRevPercentCount == revisionCount-1 { + if latestRefTraffic { + return errorTrafficDistribution(sum, errorDistributionLatestTag) + } + for _, rev := range revisions { + if !checkRevisionPresent(revisionRefMap, rev) { + trafficFlags.RevisionsPercentages = append(trafficFlags.RevisionsPercentages, fmt.Sprintf("%s=%d", rev.Name, 100-sum)) + return nil + } + } + return errorTrafficDistribution(sum, errorDistributionRevisionNotFound) + } + } + + return nil +} + +func verifyRevisionSumAndReferences(trafficFlags *flags.Traffic) (revisionRefMap map[string]int, sum int, err error) { + + revisionRefMap = make(map[string]int) + for i, each := range trafficFlags.RevisionsPercentages { + var revisionRef, percent string + revisionRef, percent, err = splitByEqualSign(each) + if err != nil { + return + } + // To check if there are duplicate revision names in traffic flags + if _, exist := revisionRefMap[revisionRef]; exist { + err = errorRepeatingRevision("--traffic", revisionRef) + return + } + + revisionRefMap[revisionRef] = i + + var percentInt int + percentInt, err = strconv.Atoi(percent) + if err != nil { + err = errorParsingInteger(percent) + return + } + + if percentInt < 0 || percentInt > 100 { + err = fmt.Errorf("invalid value for traffic percent %d, expected 0 <= percent <= 100", percentInt) + return + } + + sum += percentInt + } + return +} + +func errorParsingInteger(percent string) error { + return fmt.Errorf("error converting given %s to integer value for traffic distribution", percent) +} + +func verifyLatestTag(trafficFlags *flags.Traffic) error { + var latestRevisionTag bool for _, each := range trafficFlags.RevisionsTags { revision, _, err := splitByEqualSign(each) if err != nil { @@ -234,44 +343,19 @@ func verifyInput(trafficFlags *flags.Traffic) error { latestRevisionTag = true } } - - revisionRefMap := make(map[string]int) - for i, each := range trafficFlags.RevisionsPercentages { - revisionRef, percent, err := splitByEqualSign(each) - if err != nil { - return err - } - - // To check if there are duplicate revision names in traffic flags - if _, exist := revisionRefMap[revisionRef]; exist { - return errorRepeatingRevision("--traffic", revisionRef) - } - revisionRefMap[revisionRef] = i - - percentInt, err := strconv.Atoi(percent) - if err != nil { - return fmt.Errorf("error converting given %s to integer value for traffic distribution", percent) - } - - if percentInt < 0 || percentInt > 100 { - return fmt.Errorf("invalid value for traffic percent %d, expected 0 <= percent <= 100", percentInt) - } - - sum += percentInt - } - - // equivalent check for `cmd.Flags().Changed("traffic")` as we don't have `cmd` in this function - if len(trafficFlags.RevisionsPercentages) > 0 && sum != 100 { - return fmt.Errorf("given traffic percents sum to %d, want 100", sum) - } - return nil } +func checkRevisionPresent(refMap map[string]int, rev servingv1.Revision) bool { + _, nameExists := refMap[rev.Name] + _, tagExists := refMap[rev.Annotations[revision.RevisionTagsAnnotation]] + return tagExists || nameExists +} + // Compute takes service traffic targets and updates per given traffic flags func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget, - trafficFlags *flags.Traffic, serviceName string) ([]servingv1.TrafficTarget, error) { - err := verifyInput(trafficFlags) + trafficFlags *flags.Traffic, serviceName string, revisions []servingv1.Revision) ([]servingv1.TrafficTarget, error) { + err := verifyInput(trafficFlags, revisions) if err != nil { return nil, err } diff --git a/pkg/kn/traffic/compute_test.go b/pkg/kn/traffic/compute_test.go index a7af5e421..d0e1a60fb 100644 --- a/pkg/kn/traffic/compute_test.go +++ b/pkg/kn/traffic/compute_test.go @@ -16,6 +16,8 @@ package traffic import ( "gotest.tools/v3/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/client/pkg/kn/commands/revision" servingv1 "knative.dev/serving/pkg/apis/serving/v1" "testing" @@ -26,19 +28,21 @@ import ( ) type trafficTestCase struct { - name string - existingTraffic []servingv1.TrafficTarget - inputFlags []string - desiredRevisions []string - desiredTags []string - desiredPercents []int64 + name string + existingTraffic []servingv1.TrafficTarget + inputFlags []string + desiredRevisions []string + desiredTags []string + desiredPercents []int64 + existingRevisions []servingv1.Revision } type trafficErrorTestCase struct { - name string - existingTraffic []servingv1.TrafficTarget - inputFlags []string - errMsg string + name string + existingTraffic []servingv1.TrafficTarget + inputFlags []string + errMsg string + existingRevisions []servingv1.Revision } func newTestTrafficCommand() (*cobra.Command, *flags.Traffic) { @@ -55,133 +59,163 @@ func newTestTrafficCommand() (*cobra.Command, *flags.Traffic) { func TestCompute(t *testing.T) { for _, testCase := range []trafficTestCase{ { - "assign 'latest' tag to @latest revision", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--tag", "@latest=latest"}, - []string{"@latest"}, - []string{"latest"}, - []int64{100}, + name: "assign 'latest' tag to @latest revision", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--tag", "@latest=latest"}, + desiredRevisions: []string{"@latest"}, + desiredTags: []string{"latest"}, + desiredPercents: []int64{100}, }, { - "assign tag to revision", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "echo-v1", 100, false)), - []string{"--tag", "echo-v1=stable"}, - []string{"echo-v1"}, - []string{"stable"}, - []int64{100}, + name: "assign tag to revision", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "echo-v1", 100, false)), + inputFlags: []string{"--tag", "echo-v1=stable"}, + desiredRevisions: []string{"echo-v1"}, + desiredTags: []string{"stable"}, + desiredPercents: []int64{100}, }, { - "re-assign same tag to same revision (unchanged)", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("current", "", 100, true)), - []string{"--tag", "@latest=current"}, - []string{""}, - []string{"current"}, - []int64{100}, + name: "re-assign same tag to same revision (unchanged)", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("current", "", 100, true)), + inputFlags: []string{"--tag", "@latest=current"}, + desiredRevisions: []string{""}, + desiredTags: []string{"current"}, + desiredPercents: []int64{100}, }, { - "split traffic to tags", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true), newTarget("", "rev-v1", 0, false)), - []string{"--traffic", "@latest=10,rev-v1=90"}, - []string{"@latest", "rev-v1"}, - []string{"", ""}, - []int64{10, 90}, + name: "split traffic to tags", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true), newTarget("", "rev-v1", 0, false)), + inputFlags: []string{"--traffic", "@latest=10,rev-v1=90"}, + desiredRevisions: []string{"@latest", "rev-v1"}, + desiredTags: []string{"", ""}, + desiredPercents: []int64{10, 90}, }, { - "split traffic to tags with '%' suffix", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true), newTarget("", "rev-v1", 0, false)), - []string{"--traffic", "@latest=10%,rev-v1=90%"}, - []string{"@latest", "rev-v1"}, - []string{"", ""}, - []int64{10, 90}, + name: "split traffic to tags with '%' suffix", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true), newTarget("", "rev-v1", 0, false)), + inputFlags: []string{"--traffic", "@latest=10%,rev-v1=90%"}, + desiredRevisions: []string{"@latest", "rev-v1"}, + desiredTags: []string{"", ""}, + desiredPercents: []int64{10, 90}, }, { - "add 2 more tagged revisions without giving them traffic portions", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "", 100, true)), - []string{"--tag", "echo-v0=stale,echo-v1=old"}, - []string{"@latest", "echo-v0", "echo-v1"}, - []string{"latest", "stale", "old"}, - []int64{100, 0, 0}, + name: "add 2 more tagged revisions without giving them traffic portions", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "", 100, true)), + inputFlags: []string{"--tag", "echo-v0=stale,echo-v1=old"}, + desiredRevisions: []string{"@latest", "echo-v0", "echo-v1"}, + desiredTags: []string{"latest", "stale", "old"}, + desiredPercents: []int64{100, 0, 0}, }, { - "re-assign same tag to 'echo-v1' revision", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), - []string{"--tag", "echo-v1=latest"}, - []string{"echo-v1"}, - []string{"latest"}, - []int64{100}, + name: "re-assign same tag to 'echo-v1' revision", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + inputFlags: []string{"--tag", "echo-v1=latest"}, + desiredRevisions: []string{"echo-v1"}, + desiredTags: []string{"latest"}, + desiredPercents: []int64{100}, }, { - "set 2% traffic to latest revision by appending it in traffic block", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), - []string{"--traffic", "@latest=2,echo-v1=98"}, - []string{"echo-v1", "@latest"}, - []string{"latest", ""}, - []int64{98, 2}, + name: "set 2% traffic to latest revision by appending it in traffic block", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + inputFlags: []string{"--traffic", "@latest=2,echo-v1=98"}, + desiredRevisions: []string{"echo-v1", "@latest"}, + desiredTags: []string{"latest", ""}, + desiredPercents: []int64{98, 2}, }, { - "set 2% to @latest with tag (append it in traffic block)", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), - []string{"--traffic", "@latest=2,echo-v1=98", "--tag", "@latest=testing"}, - []string{"echo-v1", "@latest"}, - []string{"latest", "testing"}, - []int64{98, 2}, + name: "set 2% to @latest with tag (append it in traffic block)", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + inputFlags: []string{"--traffic", "@latest=2,echo-v1=98", "--tag", "@latest=testing"}, + desiredRevisions: []string{"echo-v1", "@latest"}, + desiredTags: []string{"latest", "testing"}, + desiredPercents: []int64{98, 2}, }, { - "change traffic percent of an existing revision in traffic block, add new revision with traffic share", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("v1", "echo-v1", 100, false)), - []string{"--tag", "echo-v2=v2", "--traffic", "v1=10,v2=90"}, - []string{"echo-v1", "echo-v2"}, - []string{"v1", "v2"}, - []int64{10, 90}, //default value, + name: "change traffic percent of an existing revision in traffic block, add new revision with traffic share", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("v1", "echo-v1", 100, false)), + inputFlags: []string{"--tag", "echo-v2=v2", "--traffic", "v1=10,v2=90"}, + desiredRevisions: []string{"echo-v1", "echo-v2"}, + desiredTags: []string{"v1", "v2"}, + desiredPercents: []int64{10, 90}, //default value, }, { - "untag 'latest' tag from 'echo-v1' revision", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), - []string{"--untag", "latest"}, - []string{"echo-v1"}, - []string{""}, - []int64{100}, + name: "untag 'latest' tag from 'echo-v1' revision", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + inputFlags: []string{"--untag", "latest"}, + desiredRevisions: []string{"echo-v1"}, + desiredTags: []string{""}, + desiredPercents: []int64{100}, }, { - "replace revision pointing to 'latest' tag from 'echo-v1' to 'echo-v2' revision", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 50, false), newTarget("", "echo-v2", 50, false)), - []string{"--untag", "latest", "--tag", "echo-v1=old,echo-v2=latest"}, - []string{"echo-v1", "echo-v2"}, - []string{"old", "latest"}, - []int64{50, 50}, + name: "replace revision pointing to 'latest' tag from 'echo-v1' to 'echo-v2' revision", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 50, false), newTarget("", "echo-v2", 50, false)), + inputFlags: []string{"--untag", "latest", "--tag", "echo-v1=old,echo-v2=latest"}, + desiredRevisions: []string{"echo-v1", "echo-v2"}, + desiredTags: []string{"old", "latest"}, + desiredPercents: []int64{50, 50}, }, { - "have multiple tags for a revision, revision present in traffic block", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 50, false), newTarget("", "echo-v2", 50, false)), - []string{"--tag", "echo-v1=latest,echo-v1=current"}, - []string{"echo-v1", "echo-v2", "echo-v1"}, // appends a new target - []string{"latest", "", "current"}, // with new tag requested - []int64{50, 50, 0}, // and assign 0% to it + name: "have multiple tags for a revision, revision present in traffic block", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 50, false), newTarget("", "echo-v2", 50, false)), + inputFlags: []string{"--tag", "echo-v1=latest,echo-v1=current"}, + desiredRevisions: []string{"echo-v1", "echo-v2", "echo-v1"}, // appends a new target + desiredTags: []string{"latest", "", "current"}, // with new tag requested + desiredPercents: []int64{50, 50, 0}, // and assign 0% to it }, { - "have multiple tags for a revision, revision absent in traffic block", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "echo-v2", 100, false)), - []string{"--tag", "echo-v1=latest,echo-v1=current"}, - []string{"echo-v2", "echo-v1", "echo-v1"}, // appends two new targets - []string{"", "latest", "current"}, // with new tags requested - []int64{100, 0, 0}, // and assign 0% to each + name: "have multiple tags for a revision, revision absent in traffic block", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "echo-v2", 100, false)), + inputFlags: []string{"--tag", "echo-v1=latest,echo-v1=current"}, + desiredRevisions: []string{"echo-v2", "echo-v1", "echo-v1"}, // appends two new targets + desiredTags: []string{"", "latest", "current"}, // with new tags requested + desiredPercents: []int64{100, 0, 0}, // and assign 0% to each }, { - "re-assign same tag 'current' to @latest", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("current", "", 100, true)), - []string{"--tag", "@latest=current"}, - []string{""}, - []string{"current"}, // since no change, no error - []int64{100}, + name: "re-assign same tag 'current' to @latest", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("current", "", 100, true)), + inputFlags: []string{"--tag", "@latest=current"}, + desiredRevisions: []string{""}, + desiredTags: []string{"current"}, // since no change, no error + desiredPercents: []int64{100}, }, { - "assign echo-v1 10% traffic adjusting rest to @latest, echo-v1 isn't present in existing traffic block", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--traffic", "echo-v1=10,@latest=90"}, - []string{"", "echo-v1"}, - []string{"", ""}, // since no change, no error - []int64{90, 10}, + name: "assign echo-v1 10% traffic adjusting rest to @latest, echo-v1 isn't present in existing traffic block", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--traffic", "echo-v1=10,@latest=90"}, + desiredRevisions: []string{"", "echo-v1"}, + desiredTags: []string{"", ""}, // since no change, no error + desiredPercents: []int64{90, 10}, + }, + { + name: "traffic split with sum < 100 should work for n-1 revisions", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "rev-00001", 0, false), newTarget("", "rev-00002", 0, false), newTarget("", "rev-00003", 100, false)), + inputFlags: []string{"--traffic", "rev-00001=10,rev-00002=10"}, + desiredRevisions: []string{"rev-00001", "rev-00002", "rev-00003"}, + desiredPercents: []int64{10, 10, 80}, + desiredTags: []string{"", "", ""}, + existingRevisions: []servingv1.Revision{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00001", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00002", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00003", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }}, }, } { t.Run(testCase.name, func(t *testing.T) { @@ -193,7 +227,7 @@ func TestCompute(t *testing.T) { testCmd, tFlags := newTestTrafficCommand() testCmd.SetArgs(testCase.inputFlags) testCmd.Execute() - targets, err := Compute(testCmd, testCase.existingTraffic, tFlags, "serviceName") + targets, err := Compute(testCmd, testCase.existingTraffic, tFlags, "serviceName", testCase.existingRevisions) if err != nil { t.Fatal(err) } @@ -213,89 +247,176 @@ func TestCompute(t *testing.T) { func TestComputeErrMsg(t *testing.T) { for _, testCase := range []trafficErrorTestCase{ { - "invalid format for --traffic option", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--traffic", "@latest=100=latest"}, - "expecting the value format in value1=value2, given @latest=100=latest", + name: "invalid format for --traffic option", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--traffic", "@latest=100=latest"}, + errMsg: "expecting the value format in value1=value2, given @latest=100=latest", }, { - "invalid format for --tag option", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--tag", "@latest="}, - "expecting the value format in value1=value2, given @latest=", + name: "invalid format for --tag option", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--tag", "@latest="}, + errMsg: "expecting the value format in value1=value2, given @latest=", }, { - "repeatedly splitting traffic to @latest revision", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--traffic", "@latest=90,@latest=10"}, - "repetition of identifier @latest is not allowed, use only once with --traffic flag", + name: "repeatedly splitting traffic to @latest revision", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--traffic", "@latest=90,@latest=10"}, + errMsg: "repetition of identifier @latest is not allowed, use only once with --traffic flag", }, { - "repeatedly tagging to @latest revision not allowed", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--tag", "@latest=latest,@latest=2"}, - "repetition of identifier @latest is not allowed, use only once with --tag flag", + name: "repeatedly tagging to @latest revision not allowed", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--tag", "@latest=latest,@latest=2"}, + errMsg: "repetition of identifier @latest is not allowed, use only once with --tag flag", }, { - "overwriting tag not allowed, to @latest from other revision", - append(append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "", 2, true)), newTarget("stable", "echo-v2", 98, false)), - []string{"--tag", "@latest=stable"}, - "refusing to overwrite existing tag in service, add flag '--untag stable' in command to untag it", + name: "overwriting tag not allowed, to @latest from other revision", + existingTraffic: append(append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "", 2, true)), newTarget("stable", "echo-v2", 98, false)), + inputFlags: []string{"--tag", "@latest=stable"}, + errMsg: "refusing to overwrite existing tag in service, add flag '--untag stable' in command to untag it", }, { - "overwriting tag not allowed, to a revision from other revision", - append(append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "", 2, true)), newTarget("stable", "echo-v2", 98, false)), - []string{"--tag", "echo-v2=latest"}, - "refusing to overwrite existing tag in service, add flag '--untag latest' in command to untag it", + name: "overwriting tag not allowed, to a revision from other revision", + existingTraffic: append(append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "", 2, true)), newTarget("stable", "echo-v2", 98, false)), + inputFlags: []string{"--tag", "echo-v2=latest"}, + errMsg: "refusing to overwrite existing tag in service, add flag '--untag latest' in command to untag it", }, { - "overwriting tag of @latest not allowed, existing != requested", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("candidate", "", 100, true)), - []string{"--tag", "@latest=current"}, - "tag 'candidate' exists on latest ready revision of service, refusing to overwrite existing tag with 'current', add flag '--untag candidate' in command to untag it", + name: "overwriting tag of @latest not allowed, existing != requested", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("candidate", "", 100, true)), + inputFlags: []string{"--tag", "@latest=current"}, + errMsg: "tag 'candidate' exists on latest ready revision of service, refusing to overwrite existing tag with 'current', add flag '--untag candidate' in command to untag it", }, { - "verify error for non integer values given to percent", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--traffic", "@latest=100p"}, - "error converting given 100p to integer value for traffic distribution", + name: "verify error for non integer values given to percent", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--traffic", "@latest=100p"}, + errMsg: errorParsingInteger("100p").Error(), }, { - "verify error for traffic sum not equal to 100", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--traffic", "@latest=19,echo-v1=71"}, - "given traffic percents sum to 90, want 100", + name: "verify error for traffic sum not equal to 100", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--traffic", "@latest=40,echo-v1=70"}, + errMsg: errorSumGreaterThan100(110).Error(), }, { - "verify error for values out of range given to percent", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--traffic", "@latest=-100"}, - "invalid value for traffic percent -100, expected 0 <= percent <= 100", + name: "verify error for values out of range given to percent", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--traffic", "@latest=-100"}, + errMsg: "invalid value for traffic percent -100, expected 0 <= percent <= 100", }, { - "repeatedly splitting traffic to the same revision", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), - []string{"--traffic", "echo-v1=40", "--traffic", "echo-v1=60"}, - "repetition of revision reference echo-v1 is not allowed, use only once with --traffic flag", + name: "repeatedly splitting traffic to the same revision", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)), + inputFlags: []string{"--traffic", "echo-v1=40", "--traffic", "echo-v1=60"}, + errMsg: "repetition of revision reference echo-v1 is not allowed, use only once with --traffic flag", }, { - "untag single tag that does not exist", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), - []string{"--untag", "foo"}, - "tag(s) foo not present for any revisions of service serviceName", + name: "untag single tag that does not exist", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + inputFlags: []string{"--untag", "foo"}, + errMsg: "tag(s) foo not present for any revisions of service serviceName", }, { - "untag multiple tags that do not exist", - append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), - []string{"--untag", "foo", "--untag", "bar"}, - "tag(s) foo, bar not present for any revisions of service serviceName", + name: "untag multiple tags that do not exist", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)), + inputFlags: []string{"--untag", "foo", "--untag", "bar"}, + errMsg: "tag(s) foo, bar not present for any revisions of service serviceName", + }, + { + name: "traffic split sum < 100 should have N-1 revisions specified", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "rev-00001", 0, false), newTarget("", "rev-00002", 0, false), newTarget("", "rev-00003", 100, false)), + inputFlags: []string{"--traffic", "rev-00001=10"}, + errMsg: errorTrafficDistribution(10, errorDistributionRevisionCount).Error(), + existingRevisions: []servingv1.Revision{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00001", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00002", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00003", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }}, + }, + { + name: "traffic split sum < 100 should not have @latest specified", + existingTraffic: append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "rev-00001", 0, false), newTarget("", "rev-00002", 0, false), newTarget("", "rev-00003", 100, true)), + inputFlags: []string{"--traffic", "rev-00001=10,@latest=20"}, + errMsg: errorTrafficDistribution(30, errorDistributionLatestTag).Error(), + existingRevisions: []servingv1.Revision{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00001", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00002", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00003", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }}, + }, + { + 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"}, + errMsg: errorTrafficDistribution(30, errorDistributionRevisionNotFound).Error(), + existingRevisions: []servingv1.Revision{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rev-00001", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + Annotations: map[string]string{ + 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", + Labels: map[string]string{ + "serving.knative.dev/service": "serviceName", + }, + }, + }}, }, } { t.Run(testCase.name, func(t *testing.T) { testCmd, tFlags := newTestTrafficCommand() testCmd.SetArgs(testCase.inputFlags) testCmd.Execute() - _, err := Compute(testCmd, testCase.existingTraffic, tFlags, "serviceName") + _, err := Compute(testCmd, testCase.existingTraffic, tFlags, "serviceName", testCase.existingRevisions) assert.Error(t, err, testCase.errMsg) }) } diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go index dd9039bf1..e68e6f7c7 100644 --- a/test/e2e/traffic_split_test.go +++ b/test/e2e/traffic_split_test.go @@ -131,6 +131,23 @@ func TestTrafficSplit(t *testing.T) { test.ServiceDelete(r, serviceName) }, ) + t.Run("45:55 automatic traffic split", func(t *testing.T) { + t.Log("direct 45% traffic explicitly to 1st rev and remaining automatically to second") + r := test.NewKnRunResultCollector(t, it) + defer r.DumpIfFailed() + + serviceName := test.GetNextServiceName(serviceBase) + test.ServiceCreate(r, serviceName) + test.ServiceUpdate(r, serviceName, "--env", "TARGET=v1") + + rev1 := fmt.Sprintf("%s-00001", serviceName) + rev2 := fmt.Sprintf("%s-00002", serviceName) + + test.ServiceUpdate(r, serviceName, "--traffic", fmt.Sprintf("%s=45", rev1)) + expectedTargets := []TargetFields{newTargetFields("", rev1, 45, false), newTargetFields("", rev2, 55, false)} + verifyTargets(r, serviceName, expectedTargets, false) + test.ServiceDelete(r, serviceName) + }) t.Run("TagCandidate", func(t *testing.T) { t.Log("tag a revision as candidate, without otherwise changing any traffic split")