chore(service): Improvements for waiting on service readiness (knativ…

…e#431)

* Increased default timeout to 600s. This timeout will be triggered
  only when the Ready condition stays in UNKNOWN for that long. If its
  True or False then the command will return anyway sooner.
  So it makes sense to go for a much longer timeout than 60s
* Enhanced output to indicate the progress

This change needs some updates to the API and introduces a 'MessageCallback'
type which is calle for each intermediate event with the "Ready" condition message.

CHANGELOG.adoc

@@ -73,6 +73,11 @@
 | 🎁
 | Update build.sh -w to add a message when compilation succeeded
 | https://github.com/knative/client/pull/432[#432]
+
+| 🧽
+| Add more progress information during service create/update
+| https://github.com/knative/client/pull/431[#431]
+
 |===
 
 ## v0.2.0 (2019-07-10)

docs/cmd/kn_service_create.md

@@ -63,7 +63,7 @@ kn service create NAME --image IMAGE [flags]
       --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}}")
       --service-account string   Service account name to set. Empty service account name will result to clear the service account.
-      --wait-timeout int         Seconds to wait before giving up on waiting for service to be ready. (default 60)
+      --wait-timeout int         Seconds to wait before giving up on waiting for service to be ready. (default 600)
 ```
 
 ### Options inherited from parent commands

pkg/kn/commands/service/create.go

@@ -26,11 +26,11 @@ import (
 	"knative.dev/serving/pkg/apis/serving"
 
 	"github.com/spf13/cobra"
-	serving_v1alpha1_api "knative.dev/serving/pkg/apis/serving/v1alpha1"
-
 	corev1 "k8s.io/api/core/v1"
 	api_errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	serving_kn_v1alpha1 "knative.dev/client/pkg/serving/v1alpha1"
+	serving_v1alpha1_api "knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
 
 var create_example = `
@@ -95,59 +95,57 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
+			out := cmd.OutOrStdout()
 			if serviceExists {
 				if !editFlags.ForceCreate {
 					return fmt.Errorf(
 						"cannot create service '%s' in namespace '%s' "+
 							"because the service already exists and no --force option was given", name, namespace)
 				}
-				err = replaceService(client, service, namespace, cmd.OutOrStdout())
+				err = replaceService(client, service, waitFlags, out)
 			} else {
-				err = createService(client, service, namespace, cmd.OutOrStdout())
+				err = createService(client, service, waitFlags, out)
 			}
 			if err != nil {
 				return err
 			}
-
-			if !waitFlags.Async {
-				out := cmd.OutOrStdout()
-				err := waitForService(client, name, out, waitFlags.TimeoutInSeconds)
-				if err != nil {
-					return err
-				}
-				return showUrl(client, name, namespace, out)
-			}
-
 			return nil
 		},
 	}
 	commands.AddNamespaceFlags(serviceCreateCommand.Flags(), false)
 	editFlags.AddCreateFlags(serviceCreateCommand)
-	waitFlags.AddConditionWaitFlags(serviceCreateCommand, 60, "Create", "service")
+	waitFlags.AddConditionWaitFlags(serviceCreateCommand, 600, "Create", "service")
 	return serviceCreateCommand
 }
 
-// Duck type for writers having a flush
-type flusher interface {
-	Flush() error
-}
-
-func flush(out io.Writer) {
-	if flusher, ok := out.(flusher); ok {
-		flusher.Flush()
+func createService(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, waitFlags commands.WaitFlags, out io.Writer) error {
+	err := client.CreateService(service)
+	if err != nil {
+		return err
 	}
+
+	return waitIfRequested(client, service, waitFlags, "Creating", "created", out)
 }
 
-func createService(client v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, namespace string, out io.Writer) error {
-	err := client.CreateService(service)
+func replaceService(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, waitFlags commands.WaitFlags, out io.Writer) error {
+	err := prepareAndUpdateService(client, service)
 	if err != nil {
 		return err
 	}
-	fmt.Fprintf(out, "Service '%s' successfully created in namespace '%s'.\n", service.Name, namespace)
-	return nil
+	return waitIfRequested(client, service, waitFlags, "Replacing", "replaced", out)
+}
+
+func waitIfRequested(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, waitFlags commands.WaitFlags, verbDoing string, verbDone string, out io.Writer) error {
+	if waitFlags.Async {
+		fmt.Fprintf(out, "Service '%s' %s in namespace '%s'.\n", service.Name, verbDone, client.Namespace())
+		return nil
+	}
+
+	fmt.Fprintf(out, "%s service '%s' in namespace '%s':\n", verbDoing, service.Name, client.Namespace())
+	return waitForServiceToGetReady(client, service.Name, waitFlags.TimeoutInSeconds, verbDone, out)
 }
 
-func replaceService(client v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service, namespace string, out io.Writer) error {
+func prepareAndUpdateService(client serving_kn_v1alpha1.KnServingClient, service *serving_v1alpha1_api.Service) error {
 	var retries = 0
 	for {
 		existingService, err := client.GetService(service.Name)
@@ -185,11 +183,29 @@ func replaceService(client v1alpha1.KnServingClient, service *serving_v1alpha1_a
 			}
 			return err
 		}
-		fmt.Fprintf(out, "Service '%s' successfully replaced in namespace '%s'.\n", service.Name, namespace)
 		return nil
 	}
 }
 
+func waitForServiceToGetReady(client serving_kn_v1alpha1.KnServingClient, name string, timeout int, verbDone string, out io.Writer) error {
+	err := waitForService(client, name, out, timeout)
+	if err != nil {
+		return err
+	}
+	return showUrl(client, name, "", verbDone, out)
+}
+
+// Duck type for writers having a flush
+type flusher interface {
+	Flush() error
+}
+
+func flush(out io.Writer) {
+	if flusher, ok := out.(flusher); ok {
+		flusher.Flush()
+	}
+}
+
 func serviceExists(client v1alpha1.KnServingClient, name string) (bool, error) {
 	_, err := client.GetService(name)
 	if api_errors.IsNotFound(err) {
@@ -228,17 +244,3 @@ func constructService(cmd *cobra.Command, editFlags ConfigurationEditFlags, name
 	}
 	return &service, nil
 }
-
-func showUrl(client v1alpha1.KnServingClient, serviceName string, namespace string, out io.Writer) error {
-	service, err := client.GetService(serviceName)
-	if err != nil {
-		return fmt.Errorf("cannot fetch service '%s' in namespace '%s' for extracting the URL: %v", serviceName, namespace, err)
-	}
-	url := service.Status.URL.String()
-	if url == "" {
-		url = service.Status.DeprecatedDomain
-	}
-	fmt.Fprintln(out, "\nService URL:")
-	fmt.Fprintf(out, "%s\n", url)
-	return nil
-}

pkg/kn/commands/service/create_mock_test.go

@@ -16,6 +16,7 @@ package service
 
 import (
 	"testing"
+	"time"
 
 	"gotest.tools/assert"
 	corev1 "k8s.io/api/core/v1"
@@ -27,6 +28,7 @@ import (
 
 	servinglib "knative.dev/client/pkg/serving"
 	knclient "knative.dev/client/pkg/serving/v1alpha1"
+	"knative.dev/client/pkg/wait"
 
 	"knative.dev/client/pkg/util"
 )
@@ -43,14 +45,14 @@ func TestServiceCreateImageMock(t *testing.T) {
 	// Create service (don't validate given service --> "Any()" arg is allowed)
 	r.CreateService(knclient.Any(), nil)
 	// Wait for service to become ready
-	r.WaitForService("foo", knclient.Any(), nil)
+	r.WaitForService("foo", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
 	// Get for showing the URL
 	r.GetService("foo", getServiceWithUrl("foo", "http://foo.example.com"), nil)
 
 	// Testing:
 	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz")
 	assert.NilError(t, err)
-	assert.Assert(t, util.ContainsAll(output, "created", "foo", "http://foo.example.com", "Waiting"))
+	assert.Assert(t, util.ContainsAll(output, "Creating", "foo", "http://foo.example.com", "Ready"))
 
 	// Validate that all recorded API methods have been called
 	r.Validate()

pkg/kn/commands/service/create_test.go

@@ -107,9 +107,9 @@ func fakeServiceCreate(args []string, withExistingService bool, sync bool) (
 
 func getServiceEvents(name string) []watch.Event {
 	return []watch.Event{
-		{watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "")},
-		{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "")},
-		{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "")},
+		{watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")},
+		{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", "msg2")},
+		{watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")},
 	}
 }
 
@@ -147,11 +147,11 @@ func TestServiceCreateImageSync(t *testing.T) {
 	if template.Spec.Containers[0].Image != "gcr.io/foo/bar:baz" {
 		t.Fatalf("wrong image set: %v", template.Spec.Containers[0].Image)
 	}
-	if !strings.Contains(output, "foo") || !strings.Contains(output, "created") ||
+	if !strings.Contains(output, "foo") || !strings.Contains(output, "Creating") ||
 		!strings.Contains(output, commands.FakeNamespace) {
 		t.Fatalf("wrong stdout message: %v", output)
 	}
-	if !strings.Contains(output, "OK") || !strings.Contains(output, "Waiting") {
+	if !strings.Contains(output, "Ready") {
 		t.Fatalf("not running in sync mode")
 	}
 }

pkg/kn/commands/service/list_mock_test.go

@@ -17,11 +17,14 @@ package service
 import (
 	"strings"
 	"testing"
+	"time"
 
 	"gotest.tools/assert"
 	"k8s.io/apimachinery/pkg/api/errors"
 	knclient "knative.dev/client/pkg/serving/v1alpha1"
 	"knative.dev/client/pkg/util"
+	"knative.dev/client/pkg/wait"
+
 	"knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
 
@@ -32,17 +35,17 @@ func TestServiceListAllNamespaceMock(t *testing.T) {
 
 	r.GetService("svc1", nil, errors.NewNotFound(v1alpha1.Resource("service"), "svc1"))
 	r.CreateService(knclient.Any(), nil)
-	r.WaitForService("svc1", knclient.Any(), nil)
+	r.WaitForService("svc1", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
 	r.GetService("svc1", getServiceWithNamespace("svc1", "default"), nil)
 
 	r.GetService("svc2", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo"))
 	r.CreateService(knclient.Any(), nil)
-	r.WaitForService("svc2", knclient.Any(), nil)
+	r.WaitForService("svc2", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
 	r.GetService("svc2", getServiceWithNamespace("svc2", "foo"), nil)
 
 	r.GetService("svc3", nil, errors.NewNotFound(v1alpha1.Resource("service"), "svc3"))
 	r.CreateService(knclient.Any(), nil)
-	r.WaitForService("svc3", knclient.Any(), nil)
+	r.WaitForService("svc3", knclient.Any(), wait.NoopMessageCallback(), nil, time.Second)
 	r.GetService("svc3", getServiceWithNamespace("svc3", "bar"), nil)
 
 	r.ListServices(knclient.Any(), &v1alpha1.ServiceList{
@@ -55,15 +58,17 @@ func TestServiceListAllNamespaceMock(t *testing.T) {
 
 	output, err := executeServiceCommand(client, "create", "svc1", "--image", "gcr.io/foo/bar:baz", "--namespace", "default")
 	assert.NilError(t, err)
-	assert.Assert(t, util.ContainsAll(output, "created", "svc1", "default", "Waiting"))
+	assert.Assert(t, util.ContainsAll(output, "Creating", "svc1", "default", "Ready"))
 
+	r.Namespace("foo")
 	output, err = executeServiceCommand(client, "create", "svc2", "--image", "gcr.io/foo/bar:baz", "--namespace", "foo")
 	assert.NilError(t, err)
-	assert.Assert(t, util.ContainsAll(output, "created", "svc2", "foo", "Waiting"))
+	assert.Assert(t, util.ContainsAll(output, "Creating", "svc2", "foo", "Ready"))
 
+	r.Namespace("bar")
 	output, err = executeServiceCommand(client, "create", "svc3", "--image", "gcr.io/foo/bar:baz", "--namespace", "bar")
 	assert.NilError(t, err)
-	assert.Assert(t, util.ContainsAll(output, "created", "svc3", "bar", "Waiting"))
+	assert.Assert(t, util.ContainsAll(output, "Creating", "svc3", "bar", "Ready"))
 
 	output, err = executeServiceCommand(client, "list", "--all-namespaces")
 	assert.NilError(t, err)

pkg/kn/commands/service/service.go

@@ -20,6 +20,7 @@ import (
 
 	"knative.dev/client/pkg/kn/commands"
 	serving_kn_v1alpha1 "knative.dev/client/pkg/serving/v1alpha1"
+	"knative.dev/client/pkg/wait"
 
 	"fmt"
 
@@ -45,14 +46,29 @@ func NewServiceCommand(p *commands.KnParams) *cobra.Command {
 }
 
 func waitForService(client serving_kn_v1alpha1.KnServingClient, serviceName string, out io.Writer, timeout int) error {
-	fmt.Fprintf(out, "Waiting for service '%s' to become ready ... ", serviceName)
-	flush(out)
-
-	err := client.WaitForService(serviceName, time.Duration(timeout)*time.Second)
+	fmt.Println("")
+	err, duration := client.WaitForService(serviceName, time.Duration(timeout)*time.Second, wait.SimpleMessageCallback(out))
 	if err != nil {
-		fmt.Fprintln(out)
 		return err
 	}
-	fmt.Fprintln(out, "OK")
+	fmt.Fprintf(out, "%7.3fs Ready to serve.\n", float64(duration.Round(time.Millisecond))/float64(time.Second))
+	return nil
+}
+
+func showUrl(client serving_kn_v1alpha1.KnServingClient, serviceName string, originalRevision string, what string, out io.Writer) error {
+	service, err := client.GetService(serviceName)
+	if err != nil {
+		return fmt.Errorf("cannot fetch service '%s' in namespace '%s' for extracting the URL: %v", serviceName, client.Namespace(), err)
+	}
+	url := service.Status.URL.String()
+	if url == "" {
+		url = service.Status.DeprecatedDomain
+	}
+	revisionUpdateStatus := ""
+	newRevision := service.Status.LatestReadyRevisionName
+	if originalRevision != "" && originalRevision == newRevision {
+		revisionUpdateStatus = " (unchanged)"
+	}
+	fmt.Fprintf(out, "\nService '%s' %s with latest revision '%s'%s and URL:\n%s\n", serviceName, what, newRevision, revisionUpdateStatus, url)
 	return nil
 }

pkg/kn/commands/service/update.go

@@ -80,12 +80,12 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 					return err
 				}
 				service = service.DeepCopy()
-
+				latestRevisionBeforeUpdate := service.Status.LatestReadyRevisionName
 				var baseRevision *v1alpha1.Revision
 				if !cmd.Flags().Changed("image") && editFlags.LockToDigest {
 					baseRevision, err = client.GetBaseRevision(service)
 					if _, ok := err.(*serving.NoBaseRevisionError); ok {
-						fmt.Fprintf(cmd.OutOrStdout(), "Warning: No reivision found to update image digest")
+						fmt.Fprintf(cmd.OutOrStdout(), "Warning: No revision found to update image digest")
 					}
 				}
 				err = editFlags.Apply(service, baseRevision, cmd)
@@ -112,15 +112,19 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 					return err
 				}
 
+				out := cmd.OutOrStdout()
 				if !waitFlags.Async {
-					out := cmd.OutOrStdout()
+					fmt.Fprintf(cmd.OutOrStdout(), "Updating Service '%s' in namespace '%s':\n", args[0], namespace)
 					err := waitForService(client, name, out, waitFlags.TimeoutInSeconds)
 					if err != nil {
 						return err
 					}
+					return showUrl(client, name, latestRevisionBeforeUpdate, "updated", out)
+				} else {
+					fmt.Fprintf(out, "Service '%s' updated in namespace '%s'.\n", args[0], namespace)
+
 				}
 
-				fmt.Fprintf(cmd.OutOrStdout(), "Service '%s' updated in namespace '%s'.\n", args[0], namespace)
 				return nil
 			}
 		},

pkg/kn/commands/service/update_test.go

@@ -158,7 +158,7 @@ func TestServiceUpdateImageSync(t *testing.T) {
 	assert.NilError(t, err)
 
 	assert.Equal(t, template.Spec.Containers[0].Image, "gcr.io/foo/quux:xyzzy")
-	assert.Assert(t, util.ContainsAll(strings.ToLower(output), "update", "foo", "service", "namespace", "bar", "ok", "waiting"))
+	assert.Assert(t, util.ContainsAll(strings.ToLower(output), "updating", "foo", "service", "namespace", "bar", "ready"))
 }
 
 func TestServiceUpdateImage(t *testing.T) {

pkg/kn/commands/testing_helper.go

@@ -47,7 +47,7 @@ func CreateTestKnCommand(cmd *cobra.Command, knParams *KnParams) (*cobra.Command
 	fakeServing := &fake.FakeServingV1alpha1{&client_testing.Fake{}}
 	knParams.Output = buf
 	knParams.NewClient = func(namespace string) (v1alpha1.KnServingClient, error) {
-		return v1alpha1.NewKnServingClient(fakeServing, namespace), nil
+		return v1alpha1.NewKnServingClient(fakeServing, FakeNamespace), nil
 	}
 	knParams.fixedCurrentNamespace = FakeNamespace
 	knCommand := NewKnTestCommand(cmd, knParams)