Skip to content

Commit

Permalink
fix: Fix autoscaling annotations in Service metadata (#1021)
Browse files Browse the repository at this point in the history 
* fix: Fix autoscaling annotations in Service metadata

* chore: Add test cases

* fix: Rerun codegen

* chore: Split UpdateAnnotations to dedicated functions
  • Loading branch information
@dsimansk
dsimansk authored Sep 22, 2020
1 parent 1607bf8 commit 719269e
Show file tree
Hide file tree
Showing 6 changed files with 154 additions and 84 deletions.
22 changes: 16 additions & 6 deletions pkg/kn/commands/service/configuration_edit_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down Expand Up @@ -446,11 +459,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{})
// 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
}
Expand Down
34 changes: 34 additions & 0 deletions pkg/kn/commands/service/create_mock_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
26 changes: 13 additions & 13 deletions pkg/kn/commands/service/service_update_mock_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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()
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -1480,9 +1486,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",
Expand All @@ -1491,9 +1494,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",
Expand Down
75 changes: 29 additions & 46 deletions pkg/serving/config_changes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -452,33 +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.
// 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 {

if service.ObjectMeta.Annotations == nil {
service.ObjectMeta.Annotations = make(map[string]string)
}

if template.ObjectMeta.Annotations == nil {
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 toUpdate {
service.ObjectMeta.Annotations[key] = value
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
Expand Down Expand Up @@ -543,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 {
Expand Down
74 changes: 55 additions & 19 deletions pkg/serving/config_changes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,38 +579,81 @@ func TestUpdateImagePullSecrets(t *testing.T) {
assert.Check(t, template.Spec.ImagePullSecrets == nil)
}

func TestUpdateRevisionTemplateAnnotationsNew(t *testing.T) {
_, template, _ := getService()

annotations := map[string]string{
autoscaling.InitialScaleAnnotationKey: "1",
autoscaling.MaxScaleAnnotationKey: "2",
}
err := UpdateRevisionTemplateAnnotations(template, annotations, []string{})
assert.NilError(t, err)

actual := template.ObjectMeta.Annotations
assert.DeepEqual(t, annotations, actual)
}

func TestUpdateRevisionTemplateAnnotationsExisting(t *testing.T) {
_, 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 := UpdateRevisionTemplateAnnotations(template, annotations, []string{})
assert.NilError(t, err)

actual := template.ObjectMeta.Annotations
assert.DeepEqual(t, annotations, actual)
}

func TestUpdateRevisionTemplateAnnotationsRemoveExisting(t *testing.T) {
_, 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 := UpdateRevisionTemplateAnnotations(template, map[string]string{}, remove)
assert.NilError(t, err)

actual := template.ObjectMeta.Annotations
assert.DeepEqual(t, expectedAnnotations, actual)
}

func TestUpdateAnnotationsNew(t *testing.T) {
service, template, _ := getService()
service, _, _ := getService()

annotations := map[string]string{
"a": "foo",
"b": "bar",
}
err := UpdateAnnotations(service, template, annotations, []string{})
err := UpdateServiceAnnotations(service, 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 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",
Expand All @@ -621,28 +664,21 @@ 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",
}

actual := service.ObjectMeta.Annotations
assert.DeepEqual(t, expected, actual)

actual = template.ObjectMeta.Annotations
assert.DeepEqual(t, expected, actual)
}

func TestGenerateVolumeName(t *testing.T) {
Expand Down
Loading

0 comments on commit 719269e

Please sign in to comment.