From 938d079a11283d8fe8c6d5ddb75a59570bff3f71 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 18 Sep 2020 17:18:54 +0200 Subject: [PATCH 1/4] fix: Fix autoscaling annotations in Service metadata --- .../service/configuration_edit_flags.go | 5 +---- .../service/service_update_mock_test.go | 6 ------ pkg/serving/config_changes.go | 18 ++++++++++++++++-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index d76d2381f4..50c78d9b2e 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -446,11 +446,8 @@ func (p *ConfigurationEditFlags) Apply( if cmd.Flags().Changed("annotation") && containsAnnotation(p.Annotations, autoscaling.InitialScaleAnnotationKey) { return fmt.Errorf("only one of the --scale-init or --annotation %s can be specified", autoscaling.InitialScaleAnnotationKey) } - annotationsMap := map[string]string{ - autoscaling.InitialScaleAnnotationKey: strconv.Itoa(p.ScaleInit), - } - err = servinglib.UpdateAnnotations(service, template, annotationsMap, []string{}) + err = servinglib.UpdateRevisionTemplateAnnotation(template, autoscaling.InitialScaleAnnotationKey, strconv.Itoa(p.ScaleInit)) if err != nil { return err } diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go index 783ac7c817..09ba94fcac 100644 --- a/pkg/kn/commands/service/service_update_mock_test.go +++ b/pkg/kn/commands/service/service_update_mock_test.go @@ -1480,9 +1480,6 @@ func TestServiceUpdateInitialScaleMock(t *testing.T) { newService := getService(svcName) template := &newService.Spec.Template template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" - newService.ObjectMeta.Annotations = map[string]string{ - "autoscaling.knative.dev/initialScale": "1", - } template.ObjectMeta.Annotations = map[string]string{ "autoscaling.knative.dev/initialScale": "1", clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", @@ -1491,9 +1488,6 @@ func TestServiceUpdateInitialScaleMock(t *testing.T) { updatedService := getService(svcName) template = &updatedService.Spec.Template template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" - updatedService.ObjectMeta.Annotations = map[string]string{ - "autoscaling.knative.dev/initialScale": "2", - } template.ObjectMeta.Annotations = map[string]string{ "autoscaling.knative.dev/initialScale": "2", clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 057fc32aca..a49fdcc2d3 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -453,6 +453,7 @@ func UpdateLabels(labelsMap map[string]string, add map[string]string, remove []s } // UpdateAnnotations updates the annotations identically on a service and template. +// Unless it's Autoscaling annotation that's allowed only on template. // Does not overwrite the entire Annotations field, only makes the requested updates. func UpdateAnnotations( service *servingv1.Service, @@ -460,11 +461,20 @@ func UpdateAnnotations( toUpdate map[string]string, toRemove []string) error { - if service.ObjectMeta.Annotations == nil { + // filter Autoscaling annotations + autoscalingAnnotations := make(map[string]string) + for key, value := range toUpdate { + if strings.HasPrefix(key, autoscaling.GroupName) { + autoscalingAnnotations[key] = value + delete(toUpdate, key) + } + } + + if service.ObjectMeta.Annotations == nil && len(toUpdate) > 0 { service.ObjectMeta.Annotations = make(map[string]string) } - if template.ObjectMeta.Annotations == nil { + if template.ObjectMeta.Annotations == nil && (len(toUpdate) > 0 || len(autoscalingAnnotations) > 0) { template.ObjectMeta.Annotations = make(map[string]string) } @@ -472,6 +482,10 @@ func UpdateAnnotations( service.ObjectMeta.Annotations[key] = value template.ObjectMeta.Annotations[key] = value } + // add only to template + for key, value := range autoscalingAnnotations { + template.ObjectMeta.Annotations[key] = value + } for _, key := range toRemove { delete(service.ObjectMeta.Annotations, key) From 5aadeb69d81eb1ea9986b1c5e8a1d4a017eda1b8 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Mon, 21 Sep 2020 11:56:04 +0200 Subject: [PATCH 2/4] chore: Add test cases --- pkg/serving/config_changes.go | 16 ++- pkg/serving/config_changes_test.go | 88 ++++++++++++ test/e2e/service_options_test.go | 210 +++++++++++++++-------------- 3 files changed, 204 insertions(+), 110 deletions(-) diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index a49fdcc2d3..1b60d296bf 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -462,28 +462,30 @@ func UpdateAnnotations( toRemove []string) error { // filter Autoscaling annotations - autoscalingAnnotations := make(map[string]string) + annotations := make(map[string]string) + templateAnnotations := make(map[string]string) for key, value := range toUpdate { if strings.HasPrefix(key, autoscaling.GroupName) { - autoscalingAnnotations[key] = value - delete(toUpdate, key) + templateAnnotations[key] = value + } else { + annotations[key] = value } } - if service.ObjectMeta.Annotations == nil && len(toUpdate) > 0 { + if service.ObjectMeta.Annotations == nil && len(annotations) > 0 { service.ObjectMeta.Annotations = make(map[string]string) } - if template.ObjectMeta.Annotations == nil && (len(toUpdate) > 0 || len(autoscalingAnnotations) > 0) { + if template.ObjectMeta.Annotations == nil && (len(annotations) > 0 || len(templateAnnotations) > 0) { template.ObjectMeta.Annotations = make(map[string]string) } - for key, value := range toUpdate { + for key, value := range annotations { service.ObjectMeta.Annotations[key] = value template.ObjectMeta.Annotations[key] = value } // add only to template - for key, value := range autoscalingAnnotations { + for key, value := range templateAnnotations { template.ObjectMeta.Annotations[key] = value } diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index bd985e0055..b4c02fffea 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -600,6 +600,94 @@ func TestUpdateAnnotationsNew(t *testing.T) { } } +func TestUpdateAnnotationsAutoscalingNew(t *testing.T) { + service, template, _ := getService() + + annotations := map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + err := UpdateAnnotations(service, template, annotations, []string{}) + assert.NilError(t, err) + + actual := service.ObjectMeta.Annotations + assert.Assert(t, actual == nil) + + actual = template.ObjectMeta.Annotations + assert.DeepEqual(t, annotations, actual) +} + +func TestUpdateAnnotationsAutoscalingExisting(t *testing.T) { + service, template, _ := getService() + template.ObjectMeta.Annotations = map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + + annotations := map[string]string{ + autoscaling.InitialScaleAnnotationKey: "5", + autoscaling.MaxScaleAnnotationKey: "10", + } + err := UpdateAnnotations(service, template, annotations, []string{}) + assert.NilError(t, err) + + actual := service.ObjectMeta.Annotations + assert.Assert(t, actual == nil) + + actual = template.ObjectMeta.Annotations + assert.DeepEqual(t, annotations, actual) +} + +func TestUpdateAnnotationsAutoscalingRemoveExisting(t *testing.T) { + service, template, _ := getService() + template.ObjectMeta.Annotations = map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + expectedAnnotations := map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + } + remove := []string{autoscaling.MaxScaleAnnotationKey} + err := UpdateAnnotations(service, template, map[string]string{}, remove) + assert.NilError(t, err) + + actual := service.ObjectMeta.Annotations + assert.Assert(t, actual == nil) + + actual = template.ObjectMeta.Annotations + assert.DeepEqual(t, expectedAnnotations, actual) +} + +func TestUpdateAnnotationsAutoscalingCombo(t *testing.T) { + service, template, _ := getService() + + annotations := map[string]string{ + "foo": "bar", + "bar": "foo", + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + expectedMetaAnnotations := map[string]string{ + "foo": "bar", + "bar": "foo", + } + expectedTemplateAnnotations := map[string]string{ + "foo": "bar", + "bar": "foo", + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + + err := UpdateAnnotations(service, template, annotations, []string{}) + assert.NilError(t, err) + + actual := service.ObjectMeta.Annotations + assert.DeepEqual(t, expectedMetaAnnotations, actual) + + actual = template.ObjectMeta.Annotations + assert.DeepEqual(t, expectedTemplateAnnotations, actual) +} + func TestUpdateAnnotationsExisting(t *testing.T) { service, template, _ := getService() service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go index 586eb43042..24ca02a936 100644 --- a/test/e2e/service_options_test.go +++ b/test/e2e/service_options_test.go @@ -20,8 +20,7 @@ package e2e import ( "encoding/json" "fmt" - "os" - "strconv" + "knative.dev/serving/pkg/apis/autoscaling" "strings" "testing" @@ -46,107 +45,107 @@ func TestServiceOptions(t *testing.T) { t.Log("create and validate service with concurrency options") defer r.DumpIfFailed() - serviceCreateWithOptions(r, "svc1", "--concurrency-limit", "250", "--concurrency-target", "300", "--concurrency-utilization", "50") - validateServiceConcurrencyTarget(r, "svc1", "300") - validateServiceConcurrencyLimit(r, "svc1", "250") - validateServiceConcurrencyUtilization(r, "svc1", "50") - - t.Log("update and validate service with concurrency limit") - test.ServiceUpdate(r, "svc1", "--concurrency-limit", "300") - validateServiceConcurrencyLimit(r, "svc1", "300") - - t.Log("update concurrency options with invalid values for service") - out := r.KnTest().Kn().Run("service", "update", "svc1", "--concurrency-limit", "-1", "--concurrency-target", "0") - r.AssertError(out) - assert.Check(r.T(), util.ContainsAll(out.Stderr, "should be at least 0.01")) - - t.Log("returns steady concurrency options for service") - validateServiceConcurrencyLimit(r, "svc1", "300") - validateServiceConcurrencyTarget(r, "svc1", "300") - validateServiceConcurrencyUtilization(r, "svc1", "50") - - t.Log("delete service") - test.ServiceDelete(r, "svc1") - - t.Log("create and validate service with min/max scale options") - serviceCreateWithOptions(r, "svc2", "--scale-min", "1", "--scale-max", "3") - validateServiceMinScale(r, "svc2", "1") - validateServiceMaxScale(r, "svc2", "3") - - t.Log("update and validate service with max scale option") - test.ServiceUpdate(r, "svc2", "--scale-max", "2") - validateServiceMaxScale(r, "svc2", "2") - - t.Log("create and validate service with scale options") - serviceCreateWithOptions(r, "svc2a", "--scale", "5") - validateServiceMinScale(r, "svc2a", "5") - validateServiceMaxScale(r, "svc2a", "5") - - t.Log("update and validate service with scale option") - test.ServiceUpdate(r, "svc2a", "--scale", "2") - validateServiceMaxScale(r, "svc2a", "2") - validateServiceMinScale(r, "svc2a", "2") - - t.Log("delete service") - test.ServiceDelete(r, "svc2") - - t.Log("create, update and validate service with annotations") - serviceCreateWithOptions(r, "svc3", "--annotation", "alpha=wolf", "--annotation", "brave=horse") - validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "wolf", "brave": "horse"}) - test.ServiceUpdate(r, "svc3", "--annotation", "alpha=direwolf", "--annotation", "brave-") - validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "direwolf", "brave": ""}) - test.ServiceDelete(r, "svc3") - - t.Log("create, update and validate service with annotations but -a") - serviceCreateWithOptions(r, "svc3a", "-a", "alpha=wolf", "-a", "brave=horse") - validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "wolf", "brave": "horse"}) - test.ServiceUpdate(r, "svc3a", "-a", "alpha=direwolf", "-a", "brave-") - validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "direwolf", "brave": ""}) - test.ServiceDelete(r, "svc3a") - - t.Log("create, update and validate service with autoscale window option") - serviceCreateWithOptions(r, "svc4", "--autoscale-window", "1m") - validateAutoscaleWindow(r, "svc4", "1m") - test.ServiceUpdate(r, "svc4", "--autoscale-window", "15s") - validateAutoscaleWindow(r, "svc4", "15s") - test.ServiceDelete(r, "svc4") - - t.Log("create, update and validate service with cmd and arg options") - serviceCreateWithOptions(r, "svc5", "--cmd", "/ko-app/helloworld") - validateContainerField(r, "svc5", "command", "[/ko-app/helloworld]") - test.ServiceUpdate(r, "svc5", "--arg", "myArg1", "--arg", "--myArg2") - validateContainerField(r, "svc5", "args", "[myArg1 --myArg2]") - test.ServiceUpdate(r, "svc5", "--arg", "myArg1") - validateContainerField(r, "svc5", "args", "[myArg1]") - test.ServiceDelete(r, "svc5") - - t.Log("create, update and validate service with user defined") - var uid int64 = 1000 - if uids, ok := os.LookupEnv("TEST_RUN_AS_UID"); ok { - uid, err = strconv.ParseInt(uids, 10, 64) - assert.NilError(t, err) - } - - serviceCreateWithOptions(r, "svc6", "--user", strconv.FormatInt(uid, 10)) - validateUserID(r, "svc6", uid) - test.ServiceUpdate(r, "svc6", "--user", strconv.FormatInt(uid+1, 10)) - validateUserID(r, "svc6", uid+1) - test.ServiceDelete(r, "svc6") - - t.Log("create and validate service and revision labels") - serviceCreateWithOptions(r, "svc7", "--label-service", "svc=helloworld-svc", "--label-revision", "rev=helloworld-rev") - validateLabels(r, "svc7", map[string]string{"svc": "helloworld-svc"}, map[string]string{"rev": "helloworld-rev"}) - test.ServiceDelete(r, "svc7") - - t.Log("create and validate service resource options") - serviceCreateWithOptions(r, "svc8", "--limit", "memory=500Mi,cpu=1000m", "--request", "memory=250Mi,cpu=200m") - test.ValidateServiceResources(r, "svc8", "250Mi", "200m", "500Mi", "1000m") - test.ServiceDelete(r, "svc8") - - t.Log("create a grpc service and validate port name") - serviceCreateWithOptions(r, "svc9", "--image", pkgtest.ImagePath("grpc-ping"), "--port", "h2c:8080") - validatePort(r, "svc9", 8080, "h2c") - test.ServiceDelete(r, "svc9") + //serviceCreateWithOptions(r, "svc1", "--concurrency-limit", "250", "--concurrency-target", "300", "--concurrency-utilization", "50") + //validateServiceConcurrencyTarget(r, "svc1", "300") + //validateServiceConcurrencyLimit(r, "svc1", "250") + //validateServiceConcurrencyUtilization(r, "svc1", "50") + // + //t.Log("update and validate service with concurrency limit") + //test.ServiceUpdate(r, "svc1", "--concurrency-limit", "300") + //validateServiceConcurrencyLimit(r, "svc1", "300") + // + //t.Log("update concurrency options with invalid values for service") + //out := r.KnTest().Kn().Run("service", "update", "svc1", "--concurrency-limit", "-1", "--concurrency-target", "0") + //r.AssertError(out) + //assert.Check(r.T(), util.ContainsAll(out.Stderr, "should be at least 0.01")) + // + //t.Log("returns steady concurrency options for service") + //validateServiceConcurrencyLimit(r, "svc1", "300") + //validateServiceConcurrencyTarget(r, "svc1", "300") + //validateServiceConcurrencyUtilization(r, "svc1", "50") + // + //t.Log("delete service") + //test.ServiceDelete(r, "svc1") + // + //t.Log("create and validate service with min/max scale options") + //serviceCreateWithOptions(r, "svc2", "--scale-min", "1", "--scale-max", "3") + //validateServiceMinScale(r, "svc2", "1") + //validateServiceMaxScale(r, "svc2", "3") + // + //t.Log("update and validate service with max scale option") + //test.ServiceUpdate(r, "svc2", "--scale-max", "2") + //validateServiceMaxScale(r, "svc2", "2") + // + //t.Log("create and validate service with scale options") + //serviceCreateWithOptions(r, "svc2a", "--scale", "5") + //validateServiceMinScale(r, "svc2a", "5") + //validateServiceMaxScale(r, "svc2a", "5") + // + //t.Log("update and validate service with scale option") + //test.ServiceUpdate(r, "svc2a", "--scale", "2") + //validateServiceMaxScale(r, "svc2a", "2") + //validateServiceMinScale(r, "svc2a", "2") + // + //t.Log("delete service") + //test.ServiceDelete(r, "svc2") + // + //t.Log("create, update and validate service with annotations") + //serviceCreateWithOptions(r, "svc3", "--annotation", "alpha=wolf", "--annotation", "brave=horse") + //validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "wolf", "brave": "horse"}) + //test.ServiceUpdate(r, "svc3", "--annotation", "alpha=direwolf", "--annotation", "brave-") + //validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "direwolf", "brave": ""}) + //test.ServiceDelete(r, "svc3") + // + //t.Log("create, update and validate service with annotations but -a") + //serviceCreateWithOptions(r, "svc3a", "-a", "alpha=wolf", "-a", "brave=horse") + //validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "wolf", "brave": "horse"}) + //test.ServiceUpdate(r, "svc3a", "-a", "alpha=direwolf", "-a", "brave-") + //validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "direwolf", "brave": ""}) + //test.ServiceDelete(r, "svc3a") + // + //t.Log("create, update and validate service with autoscale window option") + //serviceCreateWithOptions(r, "svc4", "--autoscale-window", "1m") + //validateAutoscaleWindow(r, "svc4", "1m") + //test.ServiceUpdate(r, "svc4", "--autoscale-window", "15s") + //validateAutoscaleWindow(r, "svc4", "15s") + //test.ServiceDelete(r, "svc4") + // + //t.Log("create, update and validate service with cmd and arg options") + //serviceCreateWithOptions(r, "svc5", "--cmd", "/ko-app/helloworld") + //validateContainerField(r, "svc5", "command", "[/ko-app/helloworld]") + //test.ServiceUpdate(r, "svc5", "--arg", "myArg1", "--arg", "--myArg2") + //validateContainerField(r, "svc5", "args", "[myArg1 --myArg2]") + //test.ServiceUpdate(r, "svc5", "--arg", "myArg1") + //validateContainerField(r, "svc5", "args", "[myArg1]") + //test.ServiceDelete(r, "svc5") + // + //t.Log("create, update and validate service with user defined") + //var uid int64 = 1000 + //if uids, ok := os.LookupEnv("TEST_RUN_AS_UID"); ok { + // uid, err = strconv.ParseInt(uids, 10, 64) + // assert.NilError(t, err) + //} + // + //serviceCreateWithOptions(r, "svc6", "--user", strconv.FormatInt(uid, 10)) + //validateUserID(r, "svc6", uid) + //test.ServiceUpdate(r, "svc6", "--user", strconv.FormatInt(uid+1, 10)) + //validateUserID(r, "svc6", uid+1) + //test.ServiceDelete(r, "svc6") + // + //t.Log("create and validate service and revision labels") + //serviceCreateWithOptions(r, "svc7", "--label-service", "svc=helloworld-svc", "--label-revision", "rev=helloworld-rev") + //validateLabels(r, "svc7", map[string]string{"svc": "helloworld-svc"}, map[string]string{"rev": "helloworld-rev"}) + //test.ServiceDelete(r, "svc7") + // + //t.Log("create and validate service resource options") + //serviceCreateWithOptions(r, "svc8", "--limit", "memory=500Mi,cpu=1000m", "--request", "memory=250Mi,cpu=200m") + //test.ValidateServiceResources(r, "svc8", "250Mi", "200m", "500Mi", "1000m") + //test.ServiceDelete(r, "svc8") + // + //t.Log("create a grpc service and validate port name") + //serviceCreateWithOptions(r, "svc9", "--image", pkgtest.ImagePath("grpc-ping"), "--port", "h2c:8080") + //validatePort(r, "svc9", 8080, "h2c") + //test.ServiceDelete(r, "svc9") t.Log("create and validate service with scale init option") serviceCreateWithOptions(r, "svc10", "--scale-init", "1") @@ -156,6 +155,11 @@ func TestServiceOptions(t *testing.T) { t.Log("delete service") test.ServiceDelete(r, "svc10") + t.Log("create and validate service with scale init option via --annotation flag") + serviceCreateWithOptions(r, "svc11", "--annotation", autoscaling.InitialScaleAnnotationKey+"=2") + validateServiceInitScale(r, "svc11", "2") + t.Log("delete service") + test.ServiceDelete(r, "svc11") } func serviceCreateWithOptions(r *test.KnRunResultCollector, serviceName string, options ...string) { From 068f216912b7886e6714612b0fdccf33eef85888 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Mon, 21 Sep 2020 12:40:31 +0200 Subject: [PATCH 3/4] fix: Rerun codegen --- test/e2e/service_options_test.go | 207 ++++++++++++++++--------------- 1 file changed, 105 insertions(+), 102 deletions(-) diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go index 24ca02a936..c0f5ffdb43 100644 --- a/test/e2e/service_options_test.go +++ b/test/e2e/service_options_test.go @@ -20,10 +20,13 @@ package e2e import ( "encoding/json" "fmt" - "knative.dev/serving/pkg/apis/autoscaling" + "os" + "strconv" "strings" "testing" + "knative.dev/serving/pkg/apis/autoscaling" + "gotest.tools/assert" "knative.dev/client/lib/test" @@ -45,107 +48,107 @@ func TestServiceOptions(t *testing.T) { t.Log("create and validate service with concurrency options") defer r.DumpIfFailed() - //serviceCreateWithOptions(r, "svc1", "--concurrency-limit", "250", "--concurrency-target", "300", "--concurrency-utilization", "50") - //validateServiceConcurrencyTarget(r, "svc1", "300") - //validateServiceConcurrencyLimit(r, "svc1", "250") - //validateServiceConcurrencyUtilization(r, "svc1", "50") - // - //t.Log("update and validate service with concurrency limit") - //test.ServiceUpdate(r, "svc1", "--concurrency-limit", "300") - //validateServiceConcurrencyLimit(r, "svc1", "300") - // - //t.Log("update concurrency options with invalid values for service") - //out := r.KnTest().Kn().Run("service", "update", "svc1", "--concurrency-limit", "-1", "--concurrency-target", "0") - //r.AssertError(out) - //assert.Check(r.T(), util.ContainsAll(out.Stderr, "should be at least 0.01")) - // - //t.Log("returns steady concurrency options for service") - //validateServiceConcurrencyLimit(r, "svc1", "300") - //validateServiceConcurrencyTarget(r, "svc1", "300") - //validateServiceConcurrencyUtilization(r, "svc1", "50") - // - //t.Log("delete service") - //test.ServiceDelete(r, "svc1") - // - //t.Log("create and validate service with min/max scale options") - //serviceCreateWithOptions(r, "svc2", "--scale-min", "1", "--scale-max", "3") - //validateServiceMinScale(r, "svc2", "1") - //validateServiceMaxScale(r, "svc2", "3") - // - //t.Log("update and validate service with max scale option") - //test.ServiceUpdate(r, "svc2", "--scale-max", "2") - //validateServiceMaxScale(r, "svc2", "2") - // - //t.Log("create and validate service with scale options") - //serviceCreateWithOptions(r, "svc2a", "--scale", "5") - //validateServiceMinScale(r, "svc2a", "5") - //validateServiceMaxScale(r, "svc2a", "5") - // - //t.Log("update and validate service with scale option") - //test.ServiceUpdate(r, "svc2a", "--scale", "2") - //validateServiceMaxScale(r, "svc2a", "2") - //validateServiceMinScale(r, "svc2a", "2") - // - //t.Log("delete service") - //test.ServiceDelete(r, "svc2") - // - //t.Log("create, update and validate service with annotations") - //serviceCreateWithOptions(r, "svc3", "--annotation", "alpha=wolf", "--annotation", "brave=horse") - //validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "wolf", "brave": "horse"}) - //test.ServiceUpdate(r, "svc3", "--annotation", "alpha=direwolf", "--annotation", "brave-") - //validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "direwolf", "brave": ""}) - //test.ServiceDelete(r, "svc3") - // - //t.Log("create, update and validate service with annotations but -a") - //serviceCreateWithOptions(r, "svc3a", "-a", "alpha=wolf", "-a", "brave=horse") - //validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "wolf", "brave": "horse"}) - //test.ServiceUpdate(r, "svc3a", "-a", "alpha=direwolf", "-a", "brave-") - //validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "direwolf", "brave": ""}) - //test.ServiceDelete(r, "svc3a") - // - //t.Log("create, update and validate service with autoscale window option") - //serviceCreateWithOptions(r, "svc4", "--autoscale-window", "1m") - //validateAutoscaleWindow(r, "svc4", "1m") - //test.ServiceUpdate(r, "svc4", "--autoscale-window", "15s") - //validateAutoscaleWindow(r, "svc4", "15s") - //test.ServiceDelete(r, "svc4") - // - //t.Log("create, update and validate service with cmd and arg options") - //serviceCreateWithOptions(r, "svc5", "--cmd", "/ko-app/helloworld") - //validateContainerField(r, "svc5", "command", "[/ko-app/helloworld]") - //test.ServiceUpdate(r, "svc5", "--arg", "myArg1", "--arg", "--myArg2") - //validateContainerField(r, "svc5", "args", "[myArg1 --myArg2]") - //test.ServiceUpdate(r, "svc5", "--arg", "myArg1") - //validateContainerField(r, "svc5", "args", "[myArg1]") - //test.ServiceDelete(r, "svc5") - // - //t.Log("create, update and validate service with user defined") - //var uid int64 = 1000 - //if uids, ok := os.LookupEnv("TEST_RUN_AS_UID"); ok { - // uid, err = strconv.ParseInt(uids, 10, 64) - // assert.NilError(t, err) - //} - // - //serviceCreateWithOptions(r, "svc6", "--user", strconv.FormatInt(uid, 10)) - //validateUserID(r, "svc6", uid) - //test.ServiceUpdate(r, "svc6", "--user", strconv.FormatInt(uid+1, 10)) - //validateUserID(r, "svc6", uid+1) - //test.ServiceDelete(r, "svc6") - // - //t.Log("create and validate service and revision labels") - //serviceCreateWithOptions(r, "svc7", "--label-service", "svc=helloworld-svc", "--label-revision", "rev=helloworld-rev") - //validateLabels(r, "svc7", map[string]string{"svc": "helloworld-svc"}, map[string]string{"rev": "helloworld-rev"}) - //test.ServiceDelete(r, "svc7") - // - //t.Log("create and validate service resource options") - //serviceCreateWithOptions(r, "svc8", "--limit", "memory=500Mi,cpu=1000m", "--request", "memory=250Mi,cpu=200m") - //test.ValidateServiceResources(r, "svc8", "250Mi", "200m", "500Mi", "1000m") - //test.ServiceDelete(r, "svc8") - // - //t.Log("create a grpc service and validate port name") - //serviceCreateWithOptions(r, "svc9", "--image", pkgtest.ImagePath("grpc-ping"), "--port", "h2c:8080") - //validatePort(r, "svc9", 8080, "h2c") - //test.ServiceDelete(r, "svc9") + serviceCreateWithOptions(r, "svc1", "--concurrency-limit", "250", "--concurrency-target", "300", "--concurrency-utilization", "50") + validateServiceConcurrencyTarget(r, "svc1", "300") + validateServiceConcurrencyLimit(r, "svc1", "250") + validateServiceConcurrencyUtilization(r, "svc1", "50") + + t.Log("update and validate service with concurrency limit") + test.ServiceUpdate(r, "svc1", "--concurrency-limit", "300") + validateServiceConcurrencyLimit(r, "svc1", "300") + + t.Log("update concurrency options with invalid values for service") + out := r.KnTest().Kn().Run("service", "update", "svc1", "--concurrency-limit", "-1", "--concurrency-target", "0") + r.AssertError(out) + assert.Check(r.T(), util.ContainsAll(out.Stderr, "should be at least 0.01")) + + t.Log("returns steady concurrency options for service") + validateServiceConcurrencyLimit(r, "svc1", "300") + validateServiceConcurrencyTarget(r, "svc1", "300") + validateServiceConcurrencyUtilization(r, "svc1", "50") + + t.Log("delete service") + test.ServiceDelete(r, "svc1") + + t.Log("create and validate service with min/max scale options") + serviceCreateWithOptions(r, "svc2", "--scale-min", "1", "--scale-max", "3") + validateServiceMinScale(r, "svc2", "1") + validateServiceMaxScale(r, "svc2", "3") + + t.Log("update and validate service with max scale option") + test.ServiceUpdate(r, "svc2", "--scale-max", "2") + validateServiceMaxScale(r, "svc2", "2") + + t.Log("create and validate service with scale options") + serviceCreateWithOptions(r, "svc2a", "--scale", "5") + validateServiceMinScale(r, "svc2a", "5") + validateServiceMaxScale(r, "svc2a", "5") + + t.Log("update and validate service with scale option") + test.ServiceUpdate(r, "svc2a", "--scale", "2") + validateServiceMaxScale(r, "svc2a", "2") + validateServiceMinScale(r, "svc2a", "2") + + t.Log("delete service") + test.ServiceDelete(r, "svc2") + + t.Log("create, update and validate service with annotations") + serviceCreateWithOptions(r, "svc3", "--annotation", "alpha=wolf", "--annotation", "brave=horse") + validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "wolf", "brave": "horse"}) + test.ServiceUpdate(r, "svc3", "--annotation", "alpha=direwolf", "--annotation", "brave-") + validateServiceAnnotations(r, "svc3", map[string]string{"alpha": "direwolf", "brave": ""}) + test.ServiceDelete(r, "svc3") + + t.Log("create, update and validate service with annotations but -a") + serviceCreateWithOptions(r, "svc3a", "-a", "alpha=wolf", "-a", "brave=horse") + validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "wolf", "brave": "horse"}) + test.ServiceUpdate(r, "svc3a", "-a", "alpha=direwolf", "-a", "brave-") + validateServiceAnnotations(r, "svc3a", map[string]string{"alpha": "direwolf", "brave": ""}) + test.ServiceDelete(r, "svc3a") + + t.Log("create, update and validate service with autoscale window option") + serviceCreateWithOptions(r, "svc4", "--autoscale-window", "1m") + validateAutoscaleWindow(r, "svc4", "1m") + test.ServiceUpdate(r, "svc4", "--autoscale-window", "15s") + validateAutoscaleWindow(r, "svc4", "15s") + test.ServiceDelete(r, "svc4") + + t.Log("create, update and validate service with cmd and arg options") + serviceCreateWithOptions(r, "svc5", "--cmd", "/ko-app/helloworld") + validateContainerField(r, "svc5", "command", "[/ko-app/helloworld]") + test.ServiceUpdate(r, "svc5", "--arg", "myArg1", "--arg", "--myArg2") + validateContainerField(r, "svc5", "args", "[myArg1 --myArg2]") + test.ServiceUpdate(r, "svc5", "--arg", "myArg1") + validateContainerField(r, "svc5", "args", "[myArg1]") + test.ServiceDelete(r, "svc5") + + t.Log("create, update and validate service with user defined") + var uid int64 = 1000 + if uids, ok := os.LookupEnv("TEST_RUN_AS_UID"); ok { + uid, err = strconv.ParseInt(uids, 10, 64) + assert.NilError(t, err) + } + + serviceCreateWithOptions(r, "svc6", "--user", strconv.FormatInt(uid, 10)) + validateUserID(r, "svc6", uid) + test.ServiceUpdate(r, "svc6", "--user", strconv.FormatInt(uid+1, 10)) + validateUserID(r, "svc6", uid+1) + test.ServiceDelete(r, "svc6") + + t.Log("create and validate service and revision labels") + serviceCreateWithOptions(r, "svc7", "--label-service", "svc=helloworld-svc", "--label-revision", "rev=helloworld-rev") + validateLabels(r, "svc7", map[string]string{"svc": "helloworld-svc"}, map[string]string{"rev": "helloworld-rev"}) + test.ServiceDelete(r, "svc7") + + t.Log("create and validate service resource options") + serviceCreateWithOptions(r, "svc8", "--limit", "memory=500Mi,cpu=1000m", "--request", "memory=250Mi,cpu=200m") + test.ValidateServiceResources(r, "svc8", "250Mi", "200m", "500Mi", "1000m") + test.ServiceDelete(r, "svc8") + + t.Log("create a grpc service and validate port name") + serviceCreateWithOptions(r, "svc9", "--image", pkgtest.ImagePath("grpc-ping"), "--port", "h2c:8080") + validatePort(r, "svc9", 8080, "h2c") + test.ServiceDelete(r, "svc9") t.Log("create and validate service with scale init option") serviceCreateWithOptions(r, "svc10", "--scale-init", "1") From 48cae5bf91676bf1a602bc127d6164512a8758a6 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Mon, 21 Sep 2020 16:14:26 +0200 Subject: [PATCH 4/4] chore: Split UpdateAnnotations to dedicated functions --- .../service/configuration_edit_flags.go | 17 ++- pkg/kn/commands/service/create_mock_test.go | 34 ++++++ .../service/service_update_mock_test.go | 20 ++-- pkg/serving/config_changes.go | 91 +++++----------- pkg/serving/config_changes_test.go | 100 +++++------------- 5 files changed, 115 insertions(+), 147 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 50c78d9b2e..8050e1464e 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -412,10 +412,23 @@ func (p *ConfigurationEditFlags) Apply( } annotationsToRemove := util.ParseMinusSuffix(annotationsMap) - err = servinglib.UpdateAnnotations(service, template, annotationsMap, annotationsToRemove) + // Service Annotations can't contain Autoscaling ones + serviceAnnotations := make(map[string]string) + for key, value := range annotationsMap { + if !strings.HasPrefix(key, autoscaling.GroupName) { + serviceAnnotations[key] = value + } + } + // Add all user provided annotations to RevisionTemplate + err = servinglib.UpdateRevisionTemplateAnnotations(template, annotationsMap, annotationsToRemove) if err != nil { return err } + err = servinglib.UpdateServiceAnnotations(service, serviceAnnotations, annotationsToRemove) + if err != nil { + return err + } + } if cmd.Flags().Changed("service-account") { @@ -446,7 +459,7 @@ func (p *ConfigurationEditFlags) Apply( if cmd.Flags().Changed("annotation") && containsAnnotation(p.Annotations, autoscaling.InitialScaleAnnotationKey) { return fmt.Errorf("only one of the --scale-init or --annotation %s can be specified", autoscaling.InitialScaleAnnotationKey) } - + // Autoscaling annotations are only applicable on Revision Template, not Service err = servinglib.UpdateRevisionTemplateAnnotation(template, autoscaling.InitialScaleAnnotationKey, strconv.Itoa(p.ScaleInit)) if err != nil { return err diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go index 627c86fd6c..0c919ee76b 100644 --- a/pkg/kn/commands/service/create_mock_test.go +++ b/pkg/kn/commands/service/create_mock_test.go @@ -18,6 +18,8 @@ import ( "testing" "time" + "knative.dev/serving/pkg/apis/autoscaling" + "gotest.tools/assert" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -545,6 +547,38 @@ func TestServiceCreateWithBothAnnotationAndInitScaleAsOption(t *testing.T) { r.Validate() } +func TestServiceCreateWithAnnotations(t *testing.T) { + client := knclient.NewMockKnServiceClient(t) + + r := client.Recorder() + r.GetService("foo", nil, errors.NewNotFound(servingv1.Resource("service"), "foo")) + + service := getService("foo") + template := &service.Spec.Template + + service.ObjectMeta.Annotations = map[string]string{ + "foo": "bar", + } + + template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" + template.ObjectMeta.Annotations = map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", // autoscaling in only added Revision Template + "foo": "bar", + servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + } + + r.CreateService(service, nil) + + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--annotation", "foo=bar", + "--annotation", autoscaling.InitialScaleAnnotationKey+"=1", + "--no-wait", "--revision-name=") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) + + r.Validate() +} + func getServiceWithUrl(name string, urlName string) *servingv1.Service { service := servingv1.Service{} url, _ := apis.ParseURL(urlName) diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go index 09ba94fcac..885647f6c4 100644 --- a/pkg/kn/commands/service/service_update_mock_test.go +++ b/pkg/kn/commands/service/service_update_mock_test.go @@ -17,6 +17,8 @@ package service import ( "testing" + "knative.dev/serving/pkg/apis/autoscaling" + "gotest.tools/assert" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -77,10 +79,11 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "an3": "getsRemoved", } template.ObjectMeta.Annotations = map[string]string{ - "an1": "staysConstant", - "an2": "getsUpdated", - "an3": "getsRemoved", - clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + "an1": "staysConstant", + "an2": "getsUpdated", + "an3": "getsRemoved", + clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + autoscaling.InitialScaleAnnotationKey: "1", } updatedService := getService(svcName) @@ -91,9 +94,10 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "an2": "isUpdated", } template.ObjectMeta.Annotations = map[string]string{ - "an1": "staysConstant", - "an2": "isUpdated", - clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + "an1": "staysConstant", + "an2": "isUpdated", + clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + autoscaling.InitialScaleAnnotationKey: "2", } r := client.Recorder() @@ -104,6 +108,7 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "--annotation", "an1=staysConstant", "--annotation", "an2=getsUpdated", "--annotation", "an3=getsRemoved", + "--annotation", autoscaling.InitialScaleAnnotationKey+"=1", "--no-wait", "--revision-name=", ) assert.NilError(t, err) @@ -113,6 +118,7 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "update", svcName, "--annotation", "an2=isUpdated", "--annotation", "an3-", + "--annotation", autoscaling.InitialScaleAnnotationKey+"=2", "--no-wait", "--revision-name=", ) assert.NilError(t, err) diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 1b60d296bf..74fcb32b3f 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -216,30 +216,6 @@ func UpdateConcurrencyLimit(template *servingv1.RevisionTemplateSpec, limit int6 return nil } -// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template. -// Also validates the autoscaling annotation values -func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec, annotation string, value string) error { - annoMap := template.Annotations - if annoMap == nil { - annoMap = make(map[string]string) - template.Annotations = annoMap - } - - // Validate autoscaling annotations and returns error if invalid input provided - // without changing the existing spec - in := make(map[string]string) - in[annotation] = value - // The boolean indicates whether or not the init-scale annotation can be set to 0. - // Since we don't have the config handy, err towards allowing it. The API will - // correctly fail the request if it's forbidden. - if err := autoscaling.ValidateAnnotations(true, in); err != nil { - return err - } - - annoMap[annotation] = value - return nil -} - // EnvToMap is an utility function to translate between the API list form of env vars, and the // more convenient map form. func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) { @@ -452,49 +428,30 @@ func UpdateLabels(labelsMap map[string]string, add map[string]string, remove []s return labelsMap } -// UpdateAnnotations updates the annotations identically on a service and template. -// Unless it's Autoscaling annotation that's allowed only on template. -// Does not overwrite the entire Annotations field, only makes the requested updates. -func UpdateAnnotations( - service *servingv1.Service, - template *servingv1.RevisionTemplateSpec, - toUpdate map[string]string, - toRemove []string) error { - - // filter Autoscaling annotations - annotations := make(map[string]string) - templateAnnotations := make(map[string]string) - for key, value := range toUpdate { - if strings.HasPrefix(key, autoscaling.GroupName) { - templateAnnotations[key] = value - } else { - annotations[key] = value - } - } - - if service.ObjectMeta.Annotations == nil && len(annotations) > 0 { - service.ObjectMeta.Annotations = make(map[string]string) - } - - if template.ObjectMeta.Annotations == nil && (len(annotations) > 0 || len(templateAnnotations) > 0) { - template.ObjectMeta.Annotations = make(map[string]string) +// UpdateServiceAnnotations updates annotations for the given Service Metadata. +func UpdateServiceAnnotations(service *servingv1.Service, toUpdate map[string]string, toRemove []string) error { + if service.Annotations == nil && len(toUpdate) > 0 { + service.Annotations = make(map[string]string) } + return updateAnnotations(service.Annotations, toUpdate, toRemove) +} - for key, value := range annotations { - service.ObjectMeta.Annotations[key] = value - template.ObjectMeta.Annotations[key] = value - } - // add only to template - for key, value := range templateAnnotations { - template.ObjectMeta.Annotations[key] = value +// UpdateRevisionTemplateAnnotations updates annotations for the given Revision Template. +// Also validates the autoscaling annotation values +func UpdateRevisionTemplateAnnotations(template *servingv1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error { + if err := autoscaling.ValidateAnnotations(true, toUpdate); err != nil { + return err } - - for _, key := range toRemove { - delete(service.ObjectMeta.Annotations, key) - delete(template.ObjectMeta.Annotations, key) + if template.Annotations == nil { + template.Annotations = make(map[string]string) } + return updateAnnotations(template.Annotations, toUpdate, toRemove) +} - return nil +// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template. +// Also validates the autoscaling annotation values +func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec, annotation string, value string) error { + return UpdateRevisionTemplateAnnotations(template, map[string]string{annotation: value}, []string{}) } // UpdateServiceAccountName updates the service account name used for the corresponding knative service @@ -559,6 +516,16 @@ func GenerateVolumeName(path string) string { // ======================================================================================= +func updateAnnotations(annotations map[string]string, toUpdate map[string]string, toRemove []string) error { + for key, value := range toUpdate { + annotations[key] = value + } + for _, key := range toRemove { + delete(annotations, key) + } + return nil +} + func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar { set := sets.NewString() for i := range env { diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index b4c02fffea..eed1b75cc0 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -579,46 +579,22 @@ func TestUpdateImagePullSecrets(t *testing.T) { assert.Check(t, template.Spec.ImagePullSecrets == nil) } -func TestUpdateAnnotationsNew(t *testing.T) { - service, template, _ := getService() - - annotations := map[string]string{ - "a": "foo", - "b": "bar", - } - err := UpdateAnnotations(service, template, annotations, []string{}) - assert.NilError(t, err) - - actual := service.ObjectMeta.Annotations - if !reflect.DeepEqual(annotations, actual) { - t.Fatalf("Service annotations did not match expected %v found %v", annotations, actual) - } - - actual = template.ObjectMeta.Annotations - if !reflect.DeepEqual(annotations, actual) { - t.Fatalf("Template annotations did not match expected %v found %v", annotations, actual) - } -} - -func TestUpdateAnnotationsAutoscalingNew(t *testing.T) { - service, template, _ := getService() +func TestUpdateRevisionTemplateAnnotationsNew(t *testing.T) { + _, template, _ := getService() annotations := map[string]string{ autoscaling.InitialScaleAnnotationKey: "1", autoscaling.MaxScaleAnnotationKey: "2", } - err := UpdateAnnotations(service, template, annotations, []string{}) + err := UpdateRevisionTemplateAnnotations(template, annotations, []string{}) assert.NilError(t, err) - actual := service.ObjectMeta.Annotations - assert.Assert(t, actual == nil) - - actual = template.ObjectMeta.Annotations + actual := template.ObjectMeta.Annotations assert.DeepEqual(t, annotations, actual) } -func TestUpdateAnnotationsAutoscalingExisting(t *testing.T) { - service, template, _ := getService() +func TestUpdateRevisionTemplateAnnotationsExisting(t *testing.T) { + _, template, _ := getService() template.ObjectMeta.Annotations = map[string]string{ autoscaling.InitialScaleAnnotationKey: "1", autoscaling.MaxScaleAnnotationKey: "2", @@ -628,18 +604,15 @@ func TestUpdateAnnotationsAutoscalingExisting(t *testing.T) { autoscaling.InitialScaleAnnotationKey: "5", autoscaling.MaxScaleAnnotationKey: "10", } - err := UpdateAnnotations(service, template, annotations, []string{}) + err := UpdateRevisionTemplateAnnotations(template, annotations, []string{}) assert.NilError(t, err) - actual := service.ObjectMeta.Annotations - assert.Assert(t, actual == nil) - - actual = template.ObjectMeta.Annotations + actual := template.ObjectMeta.Annotations assert.DeepEqual(t, annotations, actual) } -func TestUpdateAnnotationsAutoscalingRemoveExisting(t *testing.T) { - service, template, _ := getService() +func TestUpdateRevisionTemplateAnnotationsRemoveExisting(t *testing.T) { + _, template, _ := getService() template.ObjectMeta.Annotations = map[string]string{ autoscaling.InitialScaleAnnotationKey: "1", autoscaling.MaxScaleAnnotationKey: "2", @@ -648,57 +621,39 @@ func TestUpdateAnnotationsAutoscalingRemoveExisting(t *testing.T) { autoscaling.InitialScaleAnnotationKey: "1", } remove := []string{autoscaling.MaxScaleAnnotationKey} - err := UpdateAnnotations(service, template, map[string]string{}, remove) + err := UpdateRevisionTemplateAnnotations(template, map[string]string{}, remove) assert.NilError(t, err) - actual := service.ObjectMeta.Annotations - assert.Assert(t, actual == nil) - - actual = template.ObjectMeta.Annotations + actual := template.ObjectMeta.Annotations assert.DeepEqual(t, expectedAnnotations, actual) } -func TestUpdateAnnotationsAutoscalingCombo(t *testing.T) { - service, template, _ := getService() +func TestUpdateAnnotationsNew(t *testing.T) { + service, _, _ := getService() annotations := map[string]string{ - "foo": "bar", - "bar": "foo", - autoscaling.InitialScaleAnnotationKey: "1", - autoscaling.MaxScaleAnnotationKey: "2", - } - expectedMetaAnnotations := map[string]string{ - "foo": "bar", - "bar": "foo", - } - expectedTemplateAnnotations := map[string]string{ - "foo": "bar", - "bar": "foo", - autoscaling.InitialScaleAnnotationKey: "1", - autoscaling.MaxScaleAnnotationKey: "2", + "a": "foo", + "b": "bar", } - - err := UpdateAnnotations(service, template, annotations, []string{}) + err := UpdateServiceAnnotations(service, annotations, []string{}) assert.NilError(t, err) actual := service.ObjectMeta.Annotations - assert.DeepEqual(t, expectedMetaAnnotations, actual) - - actual = template.ObjectMeta.Annotations - assert.DeepEqual(t, expectedTemplateAnnotations, actual) + if !reflect.DeepEqual(annotations, actual) { + t.Fatalf("Service annotations did not match expected %v found %v", annotations, actual) + } } func TestUpdateAnnotationsExisting(t *testing.T) { - service, template, _ := getService() + service, _, _ := getService() service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} - template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} annotations := map[string]string{ "a": "notfoo", "c": "bat", "d": "", } - err := UpdateAnnotations(service, template, annotations, []string{}) + err := UpdateServiceAnnotations(service, annotations, []string{}) assert.NilError(t, err) expected := map[string]string{ "a": "notfoo", @@ -709,18 +664,14 @@ func TestUpdateAnnotationsExisting(t *testing.T) { actual := service.ObjectMeta.Annotations assert.DeepEqual(t, expected, actual) - - actual = template.ObjectMeta.Annotations - assert.DeepEqual(t, expected, actual) } func TestUpdateAnnotationsRemoveExisting(t *testing.T) { - service, template, _ := getService() + service, _, _ := getService() service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} - template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} remove := []string{"b"} - err := UpdateAnnotations(service, template, map[string]string{}, remove) + err := UpdateServiceAnnotations(service, map[string]string{}, remove) assert.NilError(t, err) expected := map[string]string{ "a": "foo", @@ -728,9 +679,6 @@ func TestUpdateAnnotationsRemoveExisting(t *testing.T) { actual := service.ObjectMeta.Annotations assert.DeepEqual(t, expected, actual) - - actual = template.ObjectMeta.Annotations - assert.DeepEqual(t, expected, actual) } func TestGenerateVolumeName(t *testing.T) {