From ffe80b9f87fad90cd3627c380a8298f3788ad1d3 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Wed, 14 Aug 2019 15:31:07 -0700 Subject: [PATCH] Specify names on service update and generate names client-side. (#282) * Groundwork for naming revisions automatically and deliberately * Tests for name updates * Add godoc comments * Changelog entry for naming flags * Error when trying to BYO revision name to an old-format service * fix tests again * Template the names * Polsh, remove unused flags, generate helptext, add better helptext * Decapitalize error msg * Respond to Roland comments * Forgot to add test file * Be a good citizen, add more tests, try to get coverage happy. * true dat * Re-add implicit service prefix if one was not added. --- CHANGELOG.adoc | 4 + cmd/kn/main.go | 3 + docs/cmd/kn_service_create.md | 1 + docs/cmd/kn_service_update.md | 1 + .../service/configuration_edit_flags.go | 88 ++++++++++++-- .../commands/service/service_update_test.go | 110 ++++++++++++++++++ pkg/serving/config_changes.go | 12 ++ pkg/serving/config_changes_test.go | 7 ++ pkg/serving/service.go | 50 ++++++++ pkg/serving/service_test.go | 83 +++++++++++++ 10 files changed, 351 insertions(+), 8 deletions(-) create mode 100644 pkg/serving/service_test.go diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index ec6da09de8..3df6cc4c10 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -38,6 +38,10 @@ | `kn service list` lists services sorted by alphabetical order | https://github.com/knative/client/pull/330[#330] +| 🎁 +| Add --revision-name flag +| https://github.com/knative/client/pull/282[#282] + |=== ## v0.2.0 (2019-07-10) diff --git a/cmd/kn/main.go b/cmd/kn/main.go index 878fa7311d..dcb0476921 100644 --- a/cmd/kn/main.go +++ b/cmd/kn/main.go @@ -16,7 +16,9 @@ package main import ( "fmt" + "math/rand" "os" + "time" "github.com/knative/client/pkg/kn/core" "github.com/spf13/viper" @@ -30,6 +32,7 @@ var err error func main() { defer cleanup() + rand.Seed(time.Now().UnixNano()) err = core.NewDefaultKnCommand().Execute() if err != nil { fmt.Fprintln(os.Stderr, err) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 6aa44876eb..7800ca32ce 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -55,6 +55,7 @@ kn service create NAME --image IMAGE [flags] -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). + --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}") --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index b908587a90..b730ead7fe 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -42,6 +42,7 @@ kn service update NAME [flags] -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). + --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}") --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index aefab59823..ed3927a8a0 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -27,16 +27,24 @@ import ( ) type ConfigurationEditFlags struct { + // Direct field manipulation Image string Env []string RequestsFlags, LimitsFlags ResourceFlags - ForceCreate bool MinScale int MaxScale int ConcurrencyTarget int ConcurrencyLimit int Port int32 Labels []string + NamePrefix string + RevisionName string + + // Preferences about how to do the action. + ForceCreate bool + + // Bookkeeping + flags []string } type ResourceFlags struct { @@ -44,34 +52,73 @@ type ResourceFlags struct { Memory string } -func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { +// markFlagMakesRevision indicates that a flag will create a new revision if you +// set it. +func (p *ConfigurationEditFlags) markFlagMakesRevision(f string) { + p.flags = append(p.flags, f) +} + +// addSharedFlags adds the flags common between create & update. +func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { command.Flags().StringVar(&p.Image, "image", "", "Image to run.") + p.markFlagMakesRevision("image") command.Flags().StringArrayVarP(&p.Env, "env", "e", []string{}, "Environment variable to set. NAME=value; you may provide this flag "+ "any number of times to set multiple environment variables. "+ "To unset, specify the environment variable name followed by a \"-\" (e.g., NAME-).") + p.markFlagMakesRevision("env") command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).") + p.markFlagMakesRevision("requests-cpu") command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).") + p.markFlagMakesRevision("requests-memory") command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", "The limits on the requested CPU (e.g., 1000m).") - command.Flags().StringVar(&p.LimitsFlags.Memory, "limits-memory", "", "The limits on the requested memory (e.g., 1024Mi).") + p.markFlagMakesRevision("limits-cpu") + command.Flags().StringVar(&p.LimitsFlags.Memory, "limits-memory", "", + "The limits on the requested memory (e.g., 1024Mi).") + p.markFlagMakesRevision("limits-memory") command.Flags().IntVar(&p.MinScale, "min-scale", 0, "Minimal number of replicas.") + p.markFlagMakesRevision("min-scale") command.Flags().IntVar(&p.MaxScale, "max-scale", 0, "Maximal number of replicas.") - command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.") - command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.") + p.markFlagMakesRevision("max-scale") + command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, + "Recommendation for when to scale up based on the concurrent number of incoming request. "+ + "Defaults to --concurrency-limit when given.") + p.markFlagMakesRevision("concurrency-target") + command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, + "Hard Limit of concurrent requests to be processed by a single replica.") + p.markFlagMakesRevision("concurrency-limit") command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.") + p.markFlagMakesRevision("port") command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, "Service label to set. name=value; you may provide this flag "+ "any number of times to set multiple labels. "+ "To unset, specify the label name followed by a \"-\" (e.g., name-).") + p.markFlagMakesRevision("label") + command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}", + "The revision name to set. Must start with the service name and a dash as a prefix. "+ + "Empty revision name will result in the server generating a name for the revision. "+ + "Accepts golang templates, allowing {{.Service}} for the service name, "+ + "{{.Generation}} for the generation, and {{.Random [n]}} for n random consonants.") + p.markFlagMakesRevision("revision-name") } +// AddUpdateFlags adds the flags specific to update. +func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { + p.addSharedFlags(command) +} + +// AddCreateFlags adds the flags specific to create func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { - p.AddUpdateFlags(command) - command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.") + p.addSharedFlags(command) + command.Flags().BoolVar(&p.ForceCreate, "force", false, + "Create service forcefully, replaces existing service if any.") command.MarkFlagRequired("image") } -func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *cobra.Command) error { +// Apply mutates the given service according to the flags in the command. +func (p *ConfigurationEditFlags) Apply( + service *servingv1alpha1.Service, + cmd *cobra.Command) error { template, err := servinglib.RevisionTemplateOfService(service) if err != nil { @@ -96,6 +143,20 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co } } + name, err := servinglib.GenerateRevisionName(p.RevisionName, service) + if err != nil { + return err + } + + if p.AnyMutation(cmd) { + err = servinglib.UpdateName(template, name) + if err == servinglib.ApiTooOldError && !cmd.Flags().Changed("revision-name") { + // Ignore the error if we don't support revision names and nobody + // explicitly asked for one. + } else if err != nil { + return err + } + } if cmd.Flags().Changed("image") { err = servinglib.UpdateImage(template, p.Image) if err != nil { @@ -194,3 +255,14 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) ( return resourceList, nil } + +// AnyMutation returns true if there are any revision template mutations in the +// command. +func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool { + for _, flag := range p.flags { + if cmd.Flags().Changed(flag) { + return true + } + } + return false +} diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index aa3cb234e4..3f2835fcee 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -22,6 +22,7 @@ import ( "testing" "gotest.tools/assert" + "gotest.tools/assert/cmp" "github.com/knative/client/pkg/kn/commands" servinglib "github.com/knative/client/pkg/serving" @@ -166,6 +167,98 @@ func TestServiceUpdateImage(t *testing.T) { } } +func TestServiceUpdateRevisionNameExplicit(t *testing.T) { + orig := newEmptyServiceBetaAPIStyle() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + template.Name = "foo-asdf" + + // Test user provides prefix + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--revision-name", "foo-dogs", "--namespace", "bar", "--async"}, false) + assert.NilError(t, err) + if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + assert.Equal(t, "foo-dogs", template.Name) + +} + +func TestServiceUpdateRevisionNameGenerated(t *testing.T) { + orig := newEmptyServiceBetaAPIStyle() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + template.Name = "foo-asdf" + + // Test prefix added by command + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--async"}, false) + assert.NilError(t, err) + if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + assert.Assert(t, strings.HasPrefix(template.Name, "foo-")) + assert.Assert(t, !(template.Name == "foo-asdf")) +} + +func TestServiceUpdateRevisionNameCleared(t *testing.T) { + orig := newEmptyServiceBetaAPIStyle() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + template.Name = "foo-asdf" + + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--revision-name=", "--async"}, false) + + assert.NilError(t, err) + if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + assert.Assert(t, cmp.Equal(template.Name, "")) +} + +func TestServiceUpdateRevisionNameNoMutationNoChange(t *testing.T) { + orig := newEmptyServiceBetaAPIStyle() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + template.Name = "foo-asdf" + + // Test prefix added by command + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--namespace", "bar", "--async"}, false) + assert.NilError(t, err) + if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template, err = servinglib.RevisionTemplateOfService(updated) + assert.NilError(t, err) + assert.Equal(t, template.Name, "foo-asdf") +} + func TestServiceUpdateMaxMinScale(t *testing.T) { original := newEmptyService() @@ -456,6 +549,23 @@ func newEmptyService() *v1alpha1.Service { } } +func newEmptyServiceBetaAPIStyle() *v1alpha1.Service { + ret := &v1alpha1.Service{ + TypeMeta: metav1.TypeMeta{ + Kind: "Service", + APIVersion: "knative.dev/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + }, + Spec: v1alpha1.ServiceSpec{}, + } + ret.Spec.Template = &v1alpha1.RevisionTemplateSpec{} + ret.Spec.Template.Spec.Containers = []corev1.Container{{}} + return ret +} + func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, limitsCPU, limitsMemory string) *v1alpha1.Service { service := &v1alpha1.Service{ TypeMeta: metav1.TypeMeta{ diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 6bfdd66475..39eb8485e2 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -16,6 +16,7 @@ package serving import ( "context" + "errors" "fmt" "strconv" @@ -92,6 +93,17 @@ func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation return nil } +var ApiTooOldError = errors.New("the service is using too old of an API format for the operation") + +// UpdateName updates the revision name. +func UpdateName(template *servingv1alpha1.RevisionTemplateSpec, name string) error { + if template.Spec.DeprecatedContainer != nil { + return ApiTooOldError + } + template.Name = name + 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) { diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index a3a3f9c393..ecbbeffef8 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -253,6 +253,13 @@ func TestUpdateContainerPort(t *testing.T) { checkPortUpdate(t, template, 80) } +func TestUpdateName(t *testing.T) { + template, _ := getV1alpha1Config() + err := UpdateName(template, "foo-asdf") + assert.NilError(t, err) + assert.Equal(t, "foo-asdf", template.Name) +} + func checkPortUpdate(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, port int32) { if template.Spec.GetContainer().Ports[0].ContainerPort != port { t.Error("Failed to update the container port") diff --git a/pkg/serving/service.go b/pkg/serving/service.go index 54c7f76d7b..d003f6d271 100644 --- a/pkg/serving/service.go +++ b/pkg/serving/service.go @@ -15,7 +15,11 @@ package serving import ( + "bytes" "errors" + "math/rand" + "strings" + "text/template" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" ) @@ -38,6 +42,52 @@ func RevisionTemplateOfService(service *servingv1alpha1.Service) (*servingv1alph return config.DeprecatedRevisionTemplate, nil } +var charChoices = []string{ + "b", "c", "d", "f", "g", "h", "j", "k", "l", "m", "n", "p", "q", "r", "s", "t", "v", "w", "x", + "y", "z", +} + +type revisionTemplContext struct { + Service string + Generation int64 +} + +func (c *revisionTemplContext) Random(l int) string { + chars := make([]string, 0, l) + for i := 0; i < l; i++ { + chars = append(chars, charChoices[rand.Int()%len(charChoices)]) + } + return strings.Join(chars, "") +} + +// GenerateRevisionName returns an automatically-generated name suitable for the +// next revision of the given service. +func GenerateRevisionName(nameTempl string, service *servingv1alpha1.Service) (string, error) { + templ, err := template.New("revisionName").Parse(nameTempl) + if err != nil { + return "", err + } + context := &revisionTemplContext{ + Service: service.Name, + Generation: service.Generation + 1, + } + buf := new(bytes.Buffer) + err = templ.Execute(buf, context) + if err != nil { + return "", err + } + res := buf.String() + // Empty is ok. + if res == "" { + return res, nil + } + prefix := service.Name + "-" + if !strings.HasPrefix(res, prefix) { + res = prefix + res + } + return res, nil +} + func getConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) { if service.Spec.DeprecatedRunLatest != nil { return &service.Spec.DeprecatedRunLatest.Configuration, nil diff --git a/pkg/serving/service_test.go b/pkg/serving/service_test.go new file mode 100644 index 0000000000..53369e57c7 --- /dev/null +++ b/pkg/serving/service_test.go @@ -0,0 +1,83 @@ +// Copyright © 2018 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package serving + +import ( + "math/rand" + "testing" + + "gotest.tools/assert" + + servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" +) + +type generateNameTest struct { + templ string + result string + err string +} + +func TestGenerateName(t *testing.T) { + rand.Seed(1) + someRandomChars := (&revisionTemplContext{}).Random(20) + service := &servingv1alpha1.Service{} + service.Name = "foo" + service.Generation = 3 + cases := []generateNameTest{ + {"{{.Service}}-v-{{.Generation}}", "foo-v-4", ""}, + {"foo-asdf", "foo-asdf", ""}, + {"{{.Bad}}", "", "can't evaluate field Bad"}, + {"{{.Service}}-{{.Random 5}}", "foo-" + someRandomChars[0:5], ""}, + {"", "", ""}, + {"andrew", "foo-andrew", ""}, + {"{{.Random 5}}", "foo-" + someRandomChars[0:5], ""}, + } + for _, c := range cases { + rand.Seed(1) + name, err := GenerateRevisionName(c.templ, service) + if c.err != "" { + assert.ErrorContains(t, err, c.err) + } else { + assert.Equal(t, name, c.result) + } + } +} + +func TestRevisionTemplateOfServiceNewStyle(t *testing.T) { + service := &servingv1alpha1.Service{} + service.Name = "foo" + template := &servingv1alpha1.RevisionTemplateSpec{} + service.Spec.Template = template + got, err := RevisionTemplateOfService(service) + assert.NilError(t, err) + assert.Equal(t, got, template) +} + +func TestRevisionTemplateOfServiceOldStyle(t *testing.T) { + service := &servingv1alpha1.Service{} + service.Name = "foo" + template := &servingv1alpha1.RevisionTemplateSpec{} + service.Spec.DeprecatedRunLatest = &servingv1alpha1.RunLatestType{} + service.Spec.DeprecatedRunLatest.Configuration.DeprecatedRevisionTemplate = template + got, err := RevisionTemplateOfService(service) + assert.NilError(t, err) + assert.Equal(t, got, template) +} + +func TestRevisionTemplateOfServiceError(t *testing.T) { + service := &servingv1alpha1.Service{} + _, err := RevisionTemplateOfService(service) + assert.ErrorContains(t, err, "does not specify") +}