From 5f68d61295f64b7d4ba37f6611a293c53822945a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Wed, 24 Feb 2021 08:16:46 +0100 Subject: [PATCH] update: Change default to server-side generated revision names (extended) (#1240) * update: Change default to server-side generated revision names Fixes #1144 * update: Fix wait loop for synthetic events for which the generations are already in sync * fix assertion as a service label change does not create a new revision * fix: --cluster-local does not create a new revision but just changes a service's label * chore: Fixed formatting * update: Check generation in update already before doing a watch * fixed lint issue * fix test assertions --- CHANGELOG.adoc | 4 +++ docs/cmd/kn_service_apply.md | 2 +- docs/cmd/kn_service_create.md | 2 +- docs/cmd/kn_service_update.md | 2 +- .../service/configuration_edit_flags.go | 35 +++++++++---------- pkg/kn/commands/service/create.go | 16 +++++---- pkg/kn/commands/service/create_test.go | 3 ++ .../service/service_update_mock_test.go | 14 ++++---- pkg/kn/commands/service/update.go | 11 ++++-- pkg/kn/commands/service/update_test.go | 24 +++++++++++-- pkg/serving/v1/client.go | 31 ++++++++-------- pkg/serving/v1/client_mock.go | 10 +++--- pkg/serving/v1/client_mock_test.go | 2 +- pkg/serving/v1/client_test.go | 12 ++++--- pkg/serving/v1/gitops.go | 8 ++--- pkg/serving/v1/gitops_test.go | 15 +++++--- pkg/wait/wait_for_ready.go | 21 ++++++----- pkg/wait/wait_for_ready_test.go | 8 ++--- test/e2e/revision_test.go | 2 +- test/e2e/service_test.go | 2 +- test/e2e/traffic_split_test.go | 2 +- 21 files changed, 137 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 08bb9fe97..5e36c203c 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -37,6 +37,10 @@ | 🐣 | Do not print `serviceUID` and `configUID` labels in service export results | https://github.com/knative/client/pull/1194[#1194] + +| ✨ +| Use server-side generated revision names by default now. For BYO revision names use `--revision-name` for service commands +| https://github.com/knative/client/issues/1240[#1240] |=== ## v0.20.0 (2021-01-14) diff --git a/docs/cmd/kn_service_apply.md b/docs/cmd/kn_service_apply.md index b8456f3b3..e6c52bad5 100644 --- a/docs/cmd/kn_service_apply.md +++ b/docs/cmd/kn_service_apply.md @@ -58,7 +58,7 @@ kn service apply s0 --filename my-svc.yml -p, --port string The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'. --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. - --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}}") + --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 (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}}) --scale string Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = unchanged) or --scale ..5 (scale-min = unchanged, scale-max = 5) --scale-init int Initial number of replicas with which a service starts. Can be 0 or a positive integer. --scale-max int Maximum number of replicas. diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 7dd6f659b..365f35f0d 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -83,7 +83,7 @@ kn service create NAME --image IMAGE -p, --port string The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'. --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. - --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}}") + --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 (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}}) --scale string Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = unchanged) or --scale ..5 (scale-min = unchanged, scale-max = 5) --scale-init int Initial number of replicas with which a service starts. Can be 0 or a positive integer. --scale-max int Maximum number of replicas. diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 8eeeae7c3..8a44fa30b 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -66,7 +66,7 @@ kn service update NAME -p, --port string The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'. --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. - --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}}") + --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 (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}}) --scale string Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = unchanged) or --scale ..5 (scale-min = unchanged, scale-max = 5) --scale-init int Initial number of replicas with which a service starts. Can be 0 or a positive integer. --scale-max int Maximum number of replicas. diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 19afcc78c..f714d9166 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -104,9 +104,6 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.ClusterLocal, "cluster-local", "", false, "Specify that the service be private. (--no-cluster-local will make the service publicly available)") - //TODO: Need to also not change revision when already set (solution to issue #646) - p.markFlagMakesRevision("cluster-local") - p.markFlagMakesRevision("no-cluster-local") command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. "+ @@ -140,11 +137,12 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { "precedence over the \"label\" flag.") p.markFlagMakesRevision("label-revision") - command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}", + command.Flags().StringVar(&p.RevisionName, "revision-name", "", "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.") + "{{.Generation}} for the generation, and {{.Random [n]}} for n random consonants "+ + "(e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})") p.markFlagMakesRevision("revision-name") knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.LockToDigest, "lock-to-digest", "", true, @@ -206,30 +204,31 @@ func (p *ConfigurationEditFlags) Apply( return err } - name, err := servinglib.GenerateRevisionName(p.RevisionName, service) - if err != nil { - return err + name := "" + if p.RevisionName != "" { + name, err = servinglib.GenerateRevisionName(p.RevisionName, service) + if err != nil { + return err + } + + if p.AnyMutation(cmd) { + template.Name = name + } } - if p.AnyMutation(cmd) { - template.Name = name - } - - imageSet := false - if cmd.Flags().Changed("image") { - imageSet = true - } _, userImagePresent := template.Annotations[servinglib.UserImageAnnotationKey] freezeMode := userImagePresent || cmd.Flags().Changed("lock-to-digest") if p.LockToDigest && p.AnyMutation(cmd) && freezeMode { servinglib.SetUserImageAnnot(template) - if !imageSet { + if !cmd.Flags().Changed("image") { err = servinglib.FreezeImageToDigest(template, baseRevision) if err != nil { return err } } - } else if !p.LockToDigest { + } + + if !p.LockToDigest { servinglib.UnsetUserImageAnnot(template) } diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go index f4bcd77ff..395db100c 100644 --- a/pkg/kn/commands/service/create.go +++ b/pkg/kn/commands/service/create.go @@ -155,10 +155,14 @@ func createService(client clientservingv1.KnServingClient, service *servingv1.Se } func replaceService(client clientservingv1.KnServingClient, service *servingv1.Service, waitFlags commands.WaitFlags, out io.Writer, targetFlag string) error { - err := prepareAndUpdateService(client, service) + changed, err := prepareAndUpdateService(client, service) if err != nil { return err } + if !changed { + fmt.Fprintf(out, "Service '%s' replaced in namespace '%s' (unchanged).\n", service.Name, client.Namespace()) + return nil + } return waitIfRequested(client, waitFlags, service.Name, "Replacing", "replaced", targetFlag, out) } @@ -171,12 +175,12 @@ func waitIfRequested(client clientservingv1.KnServingClient, waitFlags commands. return waitForServiceToGetReady(client, serviceName, waitFlags.TimeoutInSeconds, verbDone, out) } -func prepareAndUpdateService(client clientservingv1.KnServingClient, service *servingv1.Service) error { +func prepareAndUpdateService(client clientservingv1.KnServingClient, service *servingv1.Service) (bool, error) { var retries = 0 for { existingService, err := client.GetService(service.Name) if err != nil { - return err + return false, err } // Copy over some annotations that we want to keep around. Erase others @@ -200,16 +204,16 @@ func prepareAndUpdateService(client clientservingv1.KnServingClient, service *se } service.ResourceVersion = existingService.ResourceVersion - err = client.UpdateService(service) + changed, err := client.UpdateService(service) if err != nil { // Retry to update when a resource version conflict exists if apierrors.IsConflict(err) && retries < MaxUpdateRetries { retries++ continue } - return err + return changed, err } - return nil + return changed, nil } } diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index d43ac93ae..dbf4888bb 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -137,6 +137,9 @@ func TestServiceCreateImage(t *testing.T) { !strings.Contains(output, commands.FakeNamespace) { t.Fatalf("wrong stdout message: %v", output) } + + // No revision name set by default + assert.Assert(t, template.Name == "") } func TestServiceCreateWithMultipleImages(t *testing.T) { diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go index 7ef7a6020..4bfa10e93 100644 --- a/pkg/kn/commands/service/service_update_mock_test.go +++ b/pkg/kn/commands/service/service_update_mock_test.go @@ -131,7 +131,7 @@ func recordServiceUpdateWithSuccess(r *clientservingv1.ServingRecorder, svcName r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName)) r.CreateService(newService, nil) r.GetService(svcName, newService, nil) - r.UpdateService(updatedService, nil) + r.UpdateService(updatedService, true, nil) } func TestServiceUpdateEnvFromAddingWithConfigMap(t *testing.T) { @@ -283,7 +283,7 @@ func TestServiceUpdateEnvFromRemovalWithConfigMap(t *testing.T) { r.GetService(svcName, updatedService1, nil) //r.UpdateService(updatedService2, nil) // since an error happens, update is not triggered here r.GetService(svcName, updatedService2, nil) - r.UpdateService(updatedService3, nil) + r.UpdateService(updatedService3, true, nil) output, err := executeServiceCommand(client, "create", svcName, "--image", "gcr.io/foo/bar:baz", @@ -372,7 +372,7 @@ func TestServiceUpdateEnvFromRemovalWithEmptyName(t *testing.T) { r.CreateService(newService, nil) r.GetService(svcName, newService, nil) r.GetService(svcName, newService, nil) - r.UpdateService(updatedService1, nil) + r.UpdateService(updatedService1, true, nil) output, err := executeServiceCommand(client, "create", svcName, "--image", "gcr.io/foo/bar:baz", @@ -625,11 +625,11 @@ func TestServiceUpdateEnvFromRemovalWithSecret(t *testing.T) { r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName)) r.CreateService(newService, nil) r.GetService(svcName, newService, nil) - r.UpdateService(updatedService1, nil) + r.UpdateService(updatedService1, true, nil) r.GetService(svcName, updatedService1, nil) //r.UpdateService(updatedService2, nil) // since an error happens, update is not triggered here r.GetService(svcName, updatedService2, nil) - r.UpdateService(updatedService3, nil) + r.UpdateService(updatedService3, true, nil) output, err := executeServiceCommand(client, "create", svcName, "--image", "gcr.io/foo/bar:baz", @@ -1383,9 +1383,9 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName)) r.CreateService(newService, nil) r.GetService(svcName, newService, nil) - r.UpdateService(updatedService1, nil) + r.UpdateService(updatedService1, true, nil) r.GetService(svcName, updatedService1, nil) - r.UpdateService(updatedService2, nil) + r.UpdateService(updatedService2, true, nil) output, err := executeServiceCommand(client, "create", svcName, "--image", "gcr.io/foo/bar:baz", diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go index 008170448..6343824b9 100644 --- a/pkg/kn/commands/service/update.go +++ b/pkg/kn/commands/service/update.go @@ -109,12 +109,19 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command { } // Do the actual update with retry in case of conflicts - err = client.UpdateServiceWithRetry(name, updateFunc, MaxUpdateRetries) + changed, err := client.UpdateServiceWithRetry(name, updateFunc, MaxUpdateRetries) if err != nil { return err } - out := cmd.OutOrStdout() + + // No need to wait if not changed + if !changed { + fmt.Fprintf(out, "Service '%s' updated in namespace '%s'.\n", args[0], namespace) + fmt.Fprintln(out, "No new revision has been created.") + return nil + } + if waitFlags.Wait && targetFlag == "" { fmt.Fprintf(out, "Updating Service '%s' in namespace '%s':\n", args[0], namespace) fmt.Fprintln(out, "") diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index da8f22012..cd6334135 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -62,10 +62,12 @@ func fakeServiceUpdate(original *servingv1.Service, args []string) ( if !ok { return true, nil, fmt.Errorf("wrong kind of action %v", action) } - updated, ok = updateAction.GetObject().(*servingv1.Service) + given, ok := updateAction.GetObject().(*servingv1.Service) if !ok { return true, nil, errors.New("was passed the wrong object") } + updated = given.DeepCopy() + updated.Generation = given.Generation + 1 return true, updated, nil }) fakeServing.AddReactor("get", "services", @@ -271,7 +273,7 @@ func TestServiceUpdateRevisionNameGenerated(t *testing.T) { // Test prefix added by command action, updated, _, err := fakeServiceUpdate(orig, []string{ - "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait"}) + "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait", "--revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}"}) assert.NilError(t, err) if !action.Matches("update", "services") { t.Fatalf("Bad action %v", action) @@ -282,6 +284,24 @@ func TestServiceUpdateRevisionNameGenerated(t *testing.T) { assert.Assert(t, !(template.Name == "foo-asdf")) } +func TestServiceUpdateRevisionNameDefault(t *testing.T) { + orig := newEmptyService() + + template := orig.Spec.Template + template.Name = "foo-asdf" + + // Test prefix added by command + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy"}) + assert.NilError(t, err) + if !action.Matches("update", "services") { + t.Fatalf("Bad action %v", action) + } + + template = updated.Spec.Template + assert.Assert(t, cmp.Equal(template.Name, "")) +} + func TestServiceUpdateRevisionNameCleared(t *testing.T) { orig := newEmptyService() diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index c5aa30d27..68c5d53c9 100644 --- a/pkg/serving/v1/client.go +++ b/pkg/serving/v1/client.go @@ -62,12 +62,12 @@ type KnServingClient interface { // UpdateService updates the given service. For a more robust variant with automatic // conflict resolution see UpdateServiceWithRetry - UpdateService(service *servingv1.Service) error + UpdateService(service *servingv1.Service) (bool, error) // UpdateServiceWithRetry updates service and retries if there is a version conflict. // The updateFunc receives a deep copy of the existing service and can add update it in - // place. - UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) error + // place. Return if the service creates a new generation or not + UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error) // Apply a service's definition to the cluster. The full service declaration needs to be provided, // which is different to UpdateService which can also do a partial update. If the given @@ -241,36 +241,37 @@ func (cl *knServingClient) CreateService(service *servingv1.Service) error { } // Update the given service -func (cl *knServingClient) UpdateService(service *servingv1.Service) error { - _, err := cl.client.Services(cl.namespace).Update(context.TODO(), service, v1.UpdateOptions{}) +func (cl *knServingClient) UpdateService(service *servingv1.Service) (bool, error) { + updated, err := cl.client.Services(cl.namespace).Update(context.TODO(), service, v1.UpdateOptions{}) if err != nil { - return err + return false, err } - return updateServingGvk(service) + changed := service.ObjectMeta.Generation != updated.ObjectMeta.Generation + return changed, updateServingGvk(service) } // Update the given service with a retry in case of a conflict -func (cl *knServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) error { +func (cl *knServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error) { return updateServiceWithRetry(cl, name, updateFunc, nrRetries) } // Extracted to be usable with the Mocking client -func updateServiceWithRetry(cl KnServingClient, name string, updateFunc ServiceUpdateFunc, nrRetries int) error { +func updateServiceWithRetry(cl KnServingClient, name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error) { var retries = 0 for { service, err := cl.GetService(name) if err != nil { - return err + return false, err } if service.GetDeletionTimestamp() != nil { - return fmt.Errorf("can't update service %s because it has been marked for deletion", name) + return false, fmt.Errorf("can't update service %s because it has been marked for deletion", name) } updatedService, err := updateFunc(service.DeepCopy()) if err != nil { - return err + return false, err } - err = cl.UpdateService(updatedService) + changed, err := cl.UpdateService(updatedService) if err != nil { // Retry to update when a resource version conflict exists if apierrors.IsConflict(err) && retries < nrRetries { @@ -279,9 +280,9 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc ServiceU time.Sleep(time.Second) continue } - return fmt.Errorf("giving up after %d retries: %w", nrRetries, err) + return false, fmt.Errorf("giving up after %d retries: %w", nrRetries, err) } - return nil + return changed, nil } } diff --git a/pkg/serving/v1/client_mock.go b/pkg/serving/v1/client_mock.go index 4d1f89512..c2256743e 100644 --- a/pkg/serving/v1/client_mock.go +++ b/pkg/serving/v1/client_mock.go @@ -90,17 +90,17 @@ func (c *MockKnServingClient) CreateService(service *servingv1.Service) error { } // Update the given service -func (sr *ServingRecorder) UpdateService(service interface{}, err error) { - sr.r.Add("UpdateService", []interface{}{service}, []interface{}{err}) +func (sr *ServingRecorder) UpdateService(service interface{}, hasChanged bool, err error) { + sr.r.Add("UpdateService", []interface{}{service}, []interface{}{hasChanged, err}) } -func (c *MockKnServingClient) UpdateService(service *servingv1.Service) error { +func (c *MockKnServingClient) UpdateService(service *servingv1.Service) (bool, error) { call := c.recorder.r.VerifyCall("UpdateService", service) - return mock.ErrorOrNil(call.Result[0]) + return call.Result[0].(bool), mock.ErrorOrNil(call.Result[1]) } // Delegate to shared retry method -func (c *MockKnServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, maxRetry int) error { +func (c *MockKnServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, maxRetry int) (bool, error) { return updateServiceWithRetry(c, name, updateFunc, maxRetry) } diff --git a/pkg/serving/v1/client_mock_test.go b/pkg/serving/v1/client_mock_test.go index a44f355cc..3b3f9ec34 100644 --- a/pkg/serving/v1/client_mock_test.go +++ b/pkg/serving/v1/client_mock_test.go @@ -36,7 +36,7 @@ func TestMockKnClient(t *testing.T) { recorder.ListServices(mock.Any(), nil, nil) recorder.ListServices(mock.Any(), nil, nil) recorder.CreateService(&servingv1.Service{}, nil) - recorder.UpdateService(&servingv1.Service{}, nil) + recorder.UpdateService(&servingv1.Service{}, false, nil) recorder.ApplyService(&servingv1.Service{}, true, nil) recorder.DeleteService("hello", time.Duration(10)*time.Second, nil) recorder.WaitForService("hello", time.Duration(10)*time.Second, wait.NoopMessageCallback(), nil, 10*time.Second) diff --git a/pkg/serving/v1/client_test.go b/pkg/serving/v1/client_test.go index 9fa8d518f..a5a2bf6e4 100644 --- a/pkg/serving/v1/client_test.go +++ b/pkg/serving/v1/client_test.go @@ -157,27 +157,29 @@ func TestCreateService(t *testing.T) { func TestUpdateService(t *testing.T) { serving, client := setup() serviceUpdate := newService("update-service") + serviceUpdate.ObjectMeta.Generation = 2 serving.AddReactor("update", "services", func(a clienttesting.Action) (bool, runtime.Object, error) { assert.Equal(t, testNamespace, a.GetNamespace()) name := a.(clienttesting.UpdateAction).GetObject().(metav1.Object).GetName() if name == serviceUpdate.Name { - serviceUpdate.Generation = 3 - return true, serviceUpdate, nil + serviceReturn := newService("update-service") + serviceReturn.Generation = 3 + return true, serviceReturn, nil } return true, nil, fmt.Errorf("error while updating service %s", name) }) t.Run("updating a service without error", func(t *testing.T) { - err := client.UpdateService(serviceUpdate) + changed, err := client.UpdateService(serviceUpdate) assert.NilError(t, err) - assert.Equal(t, int64(3), serviceUpdate.Generation) + assert.Assert(t, changed) validateGroupVersionKind(t, serviceUpdate) }) t.Run("updating a service with error", func(t *testing.T) { - err := client.UpdateService(newService("unknown")) + _, err := client.UpdateService(newService("unknown")) assert.ErrorContains(t, err, "unknown") }) } diff --git a/pkg/serving/v1/gitops.go b/pkg/serving/v1/gitops.go index 5e342045d..d27ad8d4a 100644 --- a/pkg/serving/v1/gitops.go +++ b/pkg/serving/v1/gitops.go @@ -178,17 +178,17 @@ func writeFile(obj runtime.Object, fp, format string) error { // UpdateService updates the service in // the local directory -func (cl *knServingGitOpsClient) UpdateService(service *servingv1.Service) error { +func (cl *knServingGitOpsClient) UpdateService(service *servingv1.Service) (bool, error) { // check if file exist if _, err := cl.GetService(service.ObjectMeta.Name); err != nil { - return err + return false, err } // replace file - return cl.CreateService(service) + return true, cl.CreateService(service) } // UpdateServiceWithRetry updates the service in the local directory -func (cl *knServingGitOpsClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) error { +func (cl *knServingGitOpsClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error) { return updateServiceWithRetry(cl, name, updateFunc, nrRetries) } diff --git a/pkg/serving/v1/gitops_test.go b/pkg/serving/v1/gitops_test.go index 009f419e2..0c097fd43 100644 --- a/pkg/serving/v1/gitops_test.go +++ b/pkg/serving/v1/gitops_test.go @@ -105,13 +105,15 @@ func TestGitOpsOperations(t *testing.T) { assert.DeepEqual(t, allServices, result) }) t.Run("update service with retry foo", func(t *testing.T) { - err := fooclient.UpdateServiceWithRetry("foo", func(svc *servingv1.Service) (*servingv1.Service, error) { + changed, err := fooclient.UpdateServiceWithRetry("foo", func(svc *servingv1.Service) (*servingv1.Service, error) { return svc, nil }, 1) + assert.Assert(t, changed) assert.NilError(t, err) }) t.Run("update service foo", func(t *testing.T) { - err := fooclient.UpdateService(fooUpdateSvc) + changed, err := fooclient.UpdateService(fooUpdateSvc) + assert.Assert(t, changed) assert.NilError(t, err) }) t.Run("check updated service foo", func(t *testing.T) { @@ -176,14 +178,17 @@ func TestGitOpsSingleFile(t *testing.T) { assert.DeepEqual(t, testSvc, result) }) t.Run("update service foo", func(t *testing.T) { - err := fooclient.UpdateService(updateSvc) + changed, err := fooclient.UpdateService(updateSvc) assert.NilError(t, err) + assert.Assert(t, changed) - err = barclient.UpdateService(updateSvc) + changed, err = barclient.UpdateService(updateSvc) assert.NilError(t, err) + assert.Assert(t, changed) - err = bazclient.UpdateService(updateSvc) + changed, err = bazclient.UpdateService(updateSvc) assert.NilError(t, err) + assert.Assert(t, changed) }) t.Run("list services", func(t *testing.T) { result, err := fooclient.ListServices() diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go index 0d0ddb534..00b1b3d3e 100644 --- a/pkg/wait/wait_for_ready.go +++ b/pkg/wait/wait_for_ready.go @@ -166,6 +166,18 @@ func (w *waitForReadyConfig) waitForReadyCondition(watcher watch.Interface, star return true, false, nil } + // Check whether resource is in sync already (meta.generation == status.observedGeneration) + inSync, err := generationCheck(event.Object) + if err != nil { + return false, false, err + } + + // Skip events if generations has not yet been consolidated, regardless of type. + // Wait for the next event to come in until the generations align + if !inSync { + continue + } + // Skip event if its not a MODIFIED event, as only MODIFIED events update the condition // we are looking for. // This will filter out all synthetic ADDED events that created bt the API server for @@ -180,15 +192,6 @@ func (w *waitForReadyConfig) waitForReadyCondition(watcher watch.Interface, star continue } - // Skip event if generations has not yet been consolidated - inSync, err := generationCheck(event.Object) - if err != nil { - return false, false, err - } - if !inSync { - continue - } - conditions, err := w.conditionsExtractor(event.Object) if err != nil { return false, false, err diff --git a/pkg/wait/wait_for_ready_test.go b/pkg/wait/wait_for_ready_test.go index a8ffc93ce..b909efc0d 100644 --- a/pkg/wait/wait_for_ready_test.go +++ b/pkg/wait/wait_for_ready_test.go @@ -156,10 +156,10 @@ func pMessages(max int) []string { func peNormal(name string) ([]watch.Event, int) { messages := pMessages(2) return []watch.Event{ - {Type: watch.Added, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0])}, - {Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0])}, - {Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", messages[1])}, - {Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")}, + {Type: watch.Added, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0], 1, 2)}, + {Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0], 2, 2)}, + {Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", messages[1], 2, 2)}, + {Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "", 2, 2)}, }, len(messages) } diff --git a/test/e2e/revision_test.go b/test/e2e/revision_test.go index 4ccdcf3f6..c3d5a817d 100644 --- a/test/e2e/revision_test.go +++ b/test/e2e/revision_test.go @@ -51,7 +51,7 @@ func TestRevision(t *testing.T) { test.RevisionListWithService(r, "hello") t.Log("update hello service and increase revision count to 3") - test.ServiceUpdate(r, "hello", "--env", "TARGET=kn", "--port", "8888") + test.ServiceUpdate(r, "hello", "--env", "TARGET=kn", "--port", "9000") t.Log("delete three revisions with one revision a nonexistent") existRevision1 := test.FindRevisionByGeneration(r, "hello", 1) diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index 9875026d0..6b483c2ff 100644 --- a/test/e2e/service_test.go +++ b/test/e2e/service_test.go @@ -96,7 +96,7 @@ func serviceCreatePrivateUpdatePublic(r *test.KnRunResultCollector, serviceName out = r.KnTest().Kn().Run("service", "update", serviceName, "--image", pkgtest.ImagePath("helloworld"), "--no-cluster-local") r.AssertNoError(out) - assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "updated", "namespace", r.KnTest().Kn().Namespace(), "ready")) + assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "no new revision", "namespace", r.KnTest().Kn().Namespace())) out = r.KnTest().Kn().Run("service", "describe", serviceName, "--verbose") r.AssertNoError(out) diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go index 178c48edc..0b7857c20 100644 --- a/test/e2e/traffic_split_test.go +++ b/test/e2e/traffic_split_test.go @@ -292,7 +292,7 @@ func TestTrafficSplit(t *testing.T) { serviceCreateWithOptions(r, serviceName, "--revision-name", rev1) // existing state: traffic block having two targets - test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2") + test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2", "--revision-name", serviceName+"-rev-2") // desired state: tag non-@latest revision with two tags and 50-50% traffic each test.ServiceUpdate(r, serviceName,