Skip to content

Commit

Permalink
Validate scale and container concurrency options when updating config…
Browse files Browse the repository at this point in the history 
…uration resource (#279)

* Validate invalid container concurrency options

* Use assert package

* Use FlagSet.Changed and don't care about default values

* Update dependency

* Follow e2e test changes

* Return error if invalid value is specified by users

* Fix broken e2e test

* Add more unit tests

* Fix error message

* Fix comment statement

* Revert back unrelated changes

* Fix typo
  • Loading branch information
@toVersus @knative-prow-robot
toVersus authored and knative-prow-robot committed Jul 25, 2019
1 parent 9513ded commit b7808b0
Show file tree
Hide file tree
Showing 6 changed files with 215 additions and 62 deletions.
28 changes: 27 additions & 1 deletion pkg/kn/commands/service/configuration_edit_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,33 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
}
}

servinglib.UpdateConcurrencyConfiguration(template, p.MinScale, p.MaxScale, p.ConcurrencyTarget, p.ConcurrencyLimit)
if cmd.Flags().Changed("min-scale") {
err = servinglib.UpdateMinScale(template, p.MinScale)
if err != nil {
return err
}
}

if cmd.Flags().Changed("max-scale") {
err = servinglib.UpdateMaxScale(template, p.MaxScale)
if err != nil {
return err
}
}

if cmd.Flags().Changed("concurrency-target") {
err = servinglib.UpdateConcurrencyTarget(template, p.ConcurrencyTarget)
if err != nil {
return err
}
}

if cmd.Flags().Changed("concurrency-limit") {
err = servinglib.UpdateConcurrencyLimit(template, p.ConcurrencyLimit)
if err != nil {
return err
}
}

return nil
}
Expand Down
64 changes: 47 additions & 17 deletions pkg/serving/config_changes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@
package serving

import (
"context"
"fmt"
"strconv"

"github.com/knative/serving/pkg/apis/autoscaling"
servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
servingv1beta1 "github.com/knative/serving/pkg/apis/serving/v1beta1"
corev1 "k8s.io/api/core/v1"
)

// Give the configuration all the env var values listed in the given map of
// UpdateEnvVars gives the configuration all the env var values listed in the given map of
// vars. Does not touch any environment variables not mentioned, but it can add
// new env vars and change the values of existing ones.
func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error {
Expand All @@ -35,34 +37,61 @@ func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[stri
return nil
}

// Update min and max scale annotation if larger than 0
func UpdateConcurrencyConfiguration(template *servingv1alpha1.RevisionTemplateSpec, minScale int, maxScale int, target int, limit int) {
if minScale != 0 {
UpdateAnnotation(template, "autoscaling.knative.dev/minScale", strconv.Itoa(minScale))
}
if maxScale != 0 {
UpdateAnnotation(template, "autoscaling.knative.dev/maxScale", strconv.Itoa(maxScale))
}
if target != 0 {
UpdateAnnotation(template, "autoscaling.knative.dev/target", strconv.Itoa(target))
// UpdateMinScale updates min scale annotation
func UpdateMinScale(template *servingv1alpha1.RevisionTemplateSpec, min int) error {
return UpdateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min))
}

// UpdatMaxScale updates max scale annotation
func UpdateMaxScale(template *servingv1alpha1.RevisionTemplateSpec, max int) error {
return UpdateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max))
}

// UpdateConcurrencyTarget updates container concurrency annotation
func UpdateConcurrencyTarget(template *servingv1alpha1.RevisionTemplateSpec, target int) error {
// TODO(toVersus): Remove the following validation once serving library is updated to v0.8.0
// and just rely on ValidateAnnotations method.
if target < autoscaling.TargetMin {
return fmt.Errorf("Invalid 'concurrency-target' value: must be an integer greater than 0: %s",
autoscaling.TargetAnnotationKey)
}

if limit != 0 {
template.Spec.ContainerConcurrency = servingv1beta1.RevisionContainerConcurrencyType(limit)
return UpdateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))
}

// UpdateConcurrencyLimit updates container concurrency limit
func UpdateConcurrencyLimit(template *servingv1alpha1.RevisionTemplateSpec, limit int) error {
cc := servingv1beta1.RevisionContainerConcurrencyType(limit)
// Validate input limit
ctx := context.Background()
if err := cc.Validate(ctx).ViaField("spec.containerConcurrency"); err != nil {
return fmt.Errorf("Invalid 'concurrency-limit' value: %s", err)
}
template.Spec.ContainerConcurrency = cc
return nil
}

// Updater (or add) an annotation to the given service
func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) {
// UpdateAnnotation updates (or adds) an annotation to the given service
func UpdateAnnotation(template *servingv1alpha1.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
if err := autoscaling.ValidateAnnotations(in); err != nil {
return err
}

annoMap[annotation] = value
return nil
}

// Utility function to translate between the API list form of env vars, and the
// 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) {
result := map[string]string{}
Expand All @@ -76,7 +105,7 @@ func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) {
return result, nil
}

// Update a given image
// UpdateImage a given image
func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) error {
container, err := ContainerOfRevisionTemplate(template)
if err != nil {
Expand All @@ -98,6 +127,7 @@ func UpdateContainerPort(template *servingv1alpha1.RevisionTemplateSpec, port in
return nil
}

// UpdateResources updates resources as requested
func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error {
container, err := ContainerOfRevisionTemplate(template)
if err != nil {
Expand Down
158 changes: 123 additions & 35 deletions pkg/serving/config_changes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@ package serving

import (
"reflect"
"strconv"
"testing"

"gotest.tools/assert"

"github.com/knative/serving/pkg/apis/autoscaling"
"github.com/knative/serving/pkg/apis/serving/v1beta1"

servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
Expand All @@ -26,15 +30,35 @@ import (

func TestUpdateAutoscalingAnnotations(t *testing.T) {
template := &servingv1alpha1.RevisionTemplateSpec{}
UpdateConcurrencyConfiguration(template, 10, 100, 1000, 1000)
updateConcurrencyConfiguration(template, 10, 100, 1000, 1000)
annos := template.Annotations
if annos["autoscaling.knative.dev/minScale"] != "10" {
if annos[autoscaling.MinScaleAnnotationKey] != "10" {
t.Error("minScale failed")
}
if annos["autoscaling.knative.dev/maxScale"] != "100" {
if annos[autoscaling.MaxScaleAnnotationKey] != "100" {
t.Error("maxScale failed")
}
if annos["autoscaling.knative.dev/target"] != "1000" {
if annos[autoscaling.TargetAnnotationKey] != "1000" {
t.Error("target failed")
}
if template.Spec.ContainerConcurrency != 1000 {
t.Error("limit failed")
}
}

func TestUpdateInvalidAutoscalingAnnotations(t *testing.T) {
template := &servingv1alpha1.RevisionTemplateSpec{}
updateConcurrencyConfiguration(template, 10, 100, 1000, 1000)
// Update with invalid concurrency options
updateConcurrencyConfiguration(template, -1, -1, 0, -1)
annos := template.Annotations
if annos[autoscaling.MinScaleAnnotationKey] != "10" {
t.Error("minScale failed")
}
if annos[autoscaling.MaxScaleAnnotationKey] != "100" {
t.Error("maxScale failed")
}
if annos[autoscaling.TargetAnnotationKey] != "1000" {
t.Error("target failed")
}
if template.Spec.ContainerConcurrency != 1000 {
Expand All @@ -58,13 +82,9 @@ func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTempla
"b": "bar",
}
err := UpdateEnvVars(template, env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)
found, err := EnvToMap(container.Env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)
if !reflect.DeepEqual(env, found) {
t.Fatalf("Env did not match expected %v found %v", env, found)
}
Expand All @@ -88,19 +108,15 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision
"b": "bar",
}
err := UpdateEnvVars(template, env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)

expected := map[string]string{
"a": "foo",
"b": "bar",
}

found, err := EnvToMap(container.Env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)
if !reflect.DeepEqual(expected, found) {
t.Fatalf("Env did not match expected %v, found %v", env, found)
}
Expand All @@ -123,43 +139,99 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem
"a": "fancy",
}
err := UpdateEnvVars(revision, env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)

expected := map[string]string{
"a": "fancy",
}

found, err := EnvToMap(container.Env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)
if !reflect.DeepEqual(expected, found) {
t.Fatalf("Env did not match expected %v, found %v", env, found)
}
}

func TestUpdateMinScale(t *testing.T) {
template, _ := getV1alpha1RevisionTemplateWithOldFields()
err := UpdateMinScale(template, 10)
assert.NilError(t, err)
// Verify update is successful or not
checkAnnotationValue(t, template, autoscaling.MinScaleAnnotationKey, 10)
// Update with invalid value
err = UpdateMinScale(template, -1)
assert.ErrorContains(t, err, "Invalid")
}

func TestUpdateMaxScale(t *testing.T) {
template, _ := getV1alpha1RevisionTemplateWithOldFields()
err := UpdateMaxScale(template, 10)
assert.NilError(t, err)
// Verify update is successful or not
checkAnnotationValue(t, template, autoscaling.MaxScaleAnnotationKey, 10)
// Update with invalid value
err = UpdateMaxScale(template, -1)
assert.ErrorContains(t, err, "Invalid")
}

func TestUpdateConcurrencyTarget(t *testing.T) {
template, _ := getV1alpha1RevisionTemplateWithOldFields()
err := UpdateConcurrencyTarget(template, 10)
assert.NilError(t, err)
// Verify update is successful or not
checkAnnotationValue(t, template, autoscaling.TargetAnnotationKey, 10)
// Update with invalid value
err = UpdateConcurrencyTarget(template, -1)
assert.ErrorContains(t, err, "Invalid")
}

func TestUpdateConcurrencyLimit(t *testing.T) {
template, _ := getV1alpha1RevisionTemplateWithOldFields()
err := UpdateConcurrencyLimit(template, 10)
assert.NilError(t, err)
// Verify update is successful or not
checkContainerConcurrency(t, template, 10)
// Update with invalid value
err = UpdateConcurrencyLimit(template, -1)
assert.ErrorContains(t, err, "Invalid")
}

func TestUpdateContainerImage(t *testing.T) {
template, _ := getV1alpha1RevisionTemplateWithOldFields()
err := UpdateImage(template, "gcr.io/foo/bar:baz")
assert.NilError(t, err)
// Verify update is successful or not
checkContainerImage(t, template, "gcr.io/foo/bar:baz")
// Update template with container image info
template.Spec.GetContainer().Image = "docker.io/foo/bar:baz"
err = UpdateImage(template, "query.io/foo/bar:baz")
assert.NilError(t, err)
// Verify that given image overrides the existing container image
checkContainerImage(t, template, "query.io/foo/bar:baz")
}

func checkContainerImage(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, image string) {
if got, want := template.Spec.GetContainer().Image, image; got != want {
t.Errorf("Failed to update the container image: got=%s, want=%s", got, want)
}
}

func TestUpdateContainerPort(t *testing.T) {
template, _ := getV1alpha1Config()
err := UpdateContainerPort(template, 8888)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)
// Verify update is successful or not
checkPortUpdate(t, template, 8888)
// update template with container port info
template.Spec.Containers[0].Ports[0].ContainerPort = 9090
err = UpdateContainerPort(template, 80)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)
// Verify that given port overrides the existing container port
checkPortUpdate(t, template, 80)
}

func checkPortUpdate(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, port int32) {
if len(template.Spec.Containers) != 1 || template.Spec.Containers[0].Ports[0].ContainerPort != port {
if template.Spec.GetContainer().Ports[0].ContainerPort != port {
t.Error("Failed to update the container port")
}
}
Expand All @@ -183,9 +255,7 @@ func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTempl
"b": "boo",
}
err := UpdateEnvVars(template, env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)

expected := map[string]string{
"a": "fancy",
Expand All @@ -194,9 +264,7 @@ func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTempl
}

found, err := EnvToMap(container.Env)
if err != nil {
t.Fatal(err)
}
assert.NilError(t, err)
if !reflect.DeepEqual(expected, found) {
t.Fatalf("Env did not match expected %v, found %v", env, found)
}
Expand Down Expand Up @@ -239,3 +307,23 @@ func assertNoV1alpha1(t *testing.T, template *servingv1alpha1.RevisionTemplateSp
t.Error("Assuming only old v1alpha1 fields but found spec.template")
}
}

func checkAnnotationValue(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, key string, value int) {
anno := template.GetAnnotations()
if v, ok := anno[key]; !ok && v != strconv.Itoa(value) {
t.Errorf("Failed to update %s annotation key: got=%s, want=%d", key, v, value)
}
}

func checkContainerConcurrency(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, value int) {
if got, want := template.Spec.ContainerConcurrency, value; got != v1beta1.RevisionContainerConcurrencyType(want) {
t.Errorf("Failed to update containerConcurrency value: got=%d, want=%d", got, want)
}
}

func updateConcurrencyConfiguration(template *servingv1alpha1.RevisionTemplateSpec, minScale int, maxScale int, target int, limit int) {
UpdateMinScale(template, minScale)
UpdateMaxScale(template, maxScale)
UpdateConcurrencyTarget(template, target)
UpdateConcurrencyLimit(template, limit)
}
Loading

0 comments on commit b7808b0

Please sign in to comment.