Added flag to configure wait-window between intermediate errors durin… (#1645)

* Added flag to configure wait-window between intermediate errors during service create

* Added waitconfig struct

* Moved --wait-window flag to shared flags

docs/cmd/kn_broker_delete.md

@@ -25,6 +25,7 @@ kn broker delete NAME
       --no-wait            Do not wait for 'broker delete' operation to be completed. (default true)
       --wait               Wait for 'broker delete' operation to be completed.
       --wait-timeout int   Seconds to wait before giving up on waiting for broker to be deleted. (default 600)
+      --wait-window int    Seconds to wait for broker to be deleted after a false ready condition is returned (default 2)
 ```
 
 ### Options inherited from parent commands

docs/cmd/kn_revision_delete.md

@@ -30,6 +30,7 @@ kn revision delete NAME [NAME ...]
       --prune-all          Remove all unreferenced revisions in a namespace.
       --wait               Wait for 'revision delete' operation to be completed.
       --wait-timeout int   Seconds to wait before giving up on waiting for revision to be deleted. (default 600)
+      --wait-window int    Seconds to wait for revision to be deleted after a false ready condition is returned (default 2)
 ```
 
 ### Options inherited from parent commands

docs/cmd/kn_service_apply.md

@@ -73,6 +73,7 @@ kn service apply s0 --filename my-svc.yml
       --volume stringArray                Add a volume from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret: or sc:). Example: --volume myvolume=cm:myconfigmap or --volume myvolume=secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --volume myvolume-.
       --wait                              Wait for 'service apply' operation to be completed. (default true)
       --wait-timeout int                  Seconds to wait before giving up on waiting for service to be ready. (default 600)
+      --wait-window int                   Seconds to wait for service to be ready after a false ready condition is returned (default 2)
 ```
 
 ### Options inherited from parent commands

docs/cmd/kn_service_create.md

@@ -100,6 +100,7 @@ kn service create NAME --image IMAGE
       --volume stringArray                Add a volume from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret: or sc:). Example: --volume myvolume=cm:myconfigmap or --volume myvolume=secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --volume myvolume-.
       --wait                              Wait for 'service create' operation to be completed. (default true)
       --wait-timeout int                  Seconds to wait before giving up on waiting for service to be ready. (default 600)
+      --wait-window int                   Seconds to wait for service to be ready after a false ready condition is returned (default 2)
 ```
 
 ### Options inherited from parent commands

docs/cmd/kn_service_delete.md

@@ -35,6 +35,7 @@ kn service delete NAME [NAME ...]
       --target string      Work on local directory instead of a remote cluster (experimental)
       --wait               Wait for 'service delete' operation to be completed.
       --wait-timeout int   Seconds to wait before giving up on waiting for service to be deleted. (default 600)
+      --wait-window int    Seconds to wait for service to be deleted after a false ready condition is returned (default 2)
 ```
 
 ### Options inherited from parent commands

docs/cmd/kn_service_import.md

@@ -25,6 +25,7 @@ kn service import FILENAME
       --no-wait            Do not wait for 'service import' operation to be completed.
       --wait               Wait for 'service import' operation to be completed. (default true)
       --wait-timeout int   Seconds to wait before giving up on waiting for service to be ready. (default 600)
+      --wait-window int    Seconds to wait for service to be ready after a false ready condition is returned (default 2)
 ```
 
 ### Options inherited from parent commands

docs/cmd/kn_service_update.md

@@ -89,6 +89,7 @@ kn service update NAME
       --volume stringArray                Add a volume from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret: or sc:). Example: --volume myvolume=cm:myconfigmap or --volume myvolume=secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --volume myvolume-.
       --wait                              Wait for 'service update' operation to be completed. (default true)
       --wait-timeout int                  Seconds to wait before giving up on waiting for service to be ready. (default 600)
+      --wait-window int                   Seconds to wait for service to be ready after a false ready condition is returned (default 2)
 ```
 
 ### Options inherited from parent commands

pkg/kn/commands/service/create.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"time"
 
 	"knative.dev/client/pkg/config"
 	"knative.dev/client/pkg/kn/commands"
@@ -187,7 +188,11 @@ func waitIfRequested(ctx context.Context, client clientservingv1.KnServingClient
 		return nil
 	}
 	fmt.Fprintf(out, "%s service '%s' in namespace '%s':\n", verbDoing, serviceName, client.Namespace())
-	return waitForServiceToGetReady(ctx, client, serviceName, waitFlags.TimeoutInSeconds, verbDone, out)
+	wconfig := clientservingv1.WaitConfig{
+		Timeout:     time.Duration(waitFlags.TimeoutInSeconds) * time.Second,
+		ErrorWindow: time.Duration(waitFlags.ErrorWindowInSeconds) * time.Second,
+	}
+	return waitForServiceToGetReady(ctx, client, serviceName, wconfig, verbDone, out)
 }
 
 func prepareAndUpdateService(ctx context.Context, client clientservingv1.KnServingClient, service *servingv1.Service) (bool, error) {
@@ -220,9 +225,9 @@ func prepareAndUpdateService(ctx context.Context, client clientservingv1.KnServi
 
 }
 
-func waitForServiceToGetReady(ctx context.Context, client clientservingv1.KnServingClient, name string, timeout int, verbDone string, out io.Writer) error {
+func waitForServiceToGetReady(ctx context.Context, client clientservingv1.KnServingClient, name string, wconfig clientservingv1.WaitConfig, verbDone string, out io.Writer) error {
 	fmt.Fprintln(out, "")
-	err := waitForService(ctx, client, name, out, timeout)
+	err := waitForService(ctx, client, name, out, wconfig)
 	if err != nil {
 		return err
 	}

pkg/kn/commands/service/service.go

@@ -44,8 +44,8 @@ func NewServiceCommand(p *commands.KnParams) *cobra.Command {
 	return serviceCmd
 }
 
-func waitForService(ctx context.Context, client clientservingv1.KnServingClient, serviceName string, out io.Writer, timeout int) error {
-	err, duration := client.WaitForService(ctx, serviceName, time.Duration(timeout)*time.Second, wait.SimpleMessageCallback(out))
+func waitForService(ctx context.Context, client clientservingv1.KnServingClient, serviceName string, out io.Writer, wconfig clientservingv1.WaitConfig) error {
+	err, duration := client.WaitForService(ctx, serviceName, wconfig, wait.SimpleMessageCallback(out))
 	if err != nil {
 		return err
 	}

pkg/kn/commands/service/update.go

@@ -17,6 +17,7 @@ package service
 import (
 	"errors"
 	"fmt"
+	"time"
 
 	"github.com/spf13/cobra"
 	"knative.dev/client/pkg/config"
@@ -135,7 +136,11 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 			if waitFlags.Wait && targetFlag == "" {
 				fmt.Fprintf(out, "Updating Service '%s' in namespace '%s':\n", args[0], namespace)
 				fmt.Fprintln(out, "")
-				err := waitForService(cmd.Context(), client, name, out, waitFlags.TimeoutInSeconds)
+				wconfig := clientservingv1.WaitConfig{
+					Timeout:     time.Duration(waitFlags.TimeoutInSeconds) * time.Second,
+					ErrorWindow: time.Duration(waitFlags.ErrorWindowInSeconds) * time.Second,
+				}
+				err := waitForService(cmd.Context(), client, name, out, wconfig)
 				if err != nil {
 					return err
 				}

pkg/kn/commands/wait_flags.go

@@ -33,6 +33,8 @@ type WaitFlags struct {
 	TimeoutInSeconds int
 	// If set then apply resources and wait for completion
 	Wait bool
+	// Duration in seconds for waiting between intermediate false ready conditions
+	ErrorWindowInSeconds int
 }
 
 // Add flags which influence the wait/no-wait behaviour when creating or updating
@@ -49,4 +51,7 @@ func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDef
 	knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.Wait, "wait", "", waitDefault, waitUsage)
 	timeoutUsage := fmt.Sprintf("Seconds to wait before giving up on waiting for %s to be %s.", what, until)
 	command.Flags().IntVar(&p.TimeoutInSeconds, "wait-timeout", waitTimeoutDefault, timeoutUsage)
+
+	windowUsage := fmt.Sprintf("Seconds to wait for %s to be %s after a false ready condition is returned", what, until)
+	command.Flags().IntVar(&p.ErrorWindowInSeconds, "wait-window", 2, windowUsage)
 }

pkg/serving/v1/client.go

@@ -48,6 +48,11 @@ import (
 // or an error
 type ServiceUpdateFunc func(origService *servingv1.Service) (*servingv1.Service, error)
 
+type WaitConfig struct {
+	Timeout     time.Duration
+	ErrorWindow time.Duration
+}
+
 // Kn interface to serving. All methods are relative to the
 // namespace specified during construction
 type KnServingClient interface {
@@ -89,7 +94,7 @@ type KnServingClient interface {
 
 	// Wait for a service to become ready, but not longer than provided timeout.
 	// Return error and how long has been waited
-	WaitForService(ctx context.Context, name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration)
+	WaitForService(ctx context.Context, name string, wconfig WaitConfig, msgCallback wait.MessageCallback) (error, time.Duration)
 
 	// Get a configuration by name
 	GetConfiguration(ctx context.Context, name string) (*servingv1.Configuration, error)
@@ -351,17 +356,17 @@ func (cl *knServingClient) deleteService(ctx context.Context, serviceName string
 }
 
 // Wait for a service to become ready, but not longer than provided timeout
-func (cl *knServingClient) WaitForService(ctx context.Context, name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration) {
+func (cl *knServingClient) WaitForService(ctx context.Context, name string, wconfig WaitConfig, msgCallback wait.MessageCallback) (error, time.Duration) {
 	waitForReady := wait.NewWaitForReady("service", cl.WatchServiceWithVersion, serviceConditionExtractor)
 
 	service, err := cl.GetService(ctx, name)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			return waitForReady.Wait(ctx, name, "", wait.Options{Timeout: &timeout}, msgCallback)
+			return waitForReady.Wait(ctx, name, "", wait.Options{Timeout: &wconfig.Timeout, ErrorWindow: &wconfig.ErrorWindow}, msgCallback)
 		}
 		return err, 0
 	}
-	return waitForReady.Wait(ctx, name, service.ResourceVersion, wait.Options{Timeout: &timeout}, msgCallback)
+	return waitForReady.Wait(ctx, name, service.ResourceVersion, wait.Options{Timeout: &wconfig.Timeout, ErrorWindow: &wconfig.ErrorWindow}, msgCallback)
 }
 
 // Get the configuration for a service

pkg/serving/v1/client_mock.go

@@ -126,12 +126,12 @@ func (c *MockKnServingClient) DeleteService(ctx context.Context, name string, ti
 }
 
 // Wait for a service to become ready, but not longer than provided timeout
-func (sr *ServingRecorder) WaitForService(name interface{}, timeout interface{}, callback interface{}, err error, duration time.Duration) {
-	sr.r.Add("WaitForService", []interface{}{name, timeout, callback}, []interface{}{err, duration})
+func (sr *ServingRecorder) WaitForService(name interface{}, wconfig interface{}, callback interface{}, err error, duration time.Duration) {
+	sr.r.Add("WaitForService", []interface{}{name, wconfig, callback}, []interface{}{err, duration})
 }
 
-func (c *MockKnServingClient) WaitForService(ctx context.Context, name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration) {
-	call := c.recorder.r.VerifyCall("WaitForService", name, timeout, msgCallback)
+func (c *MockKnServingClient) WaitForService(ctx context.Context, name string, wconfig WaitConfig, msgCallback wait.MessageCallback) (error, time.Duration) {
+	call := c.recorder.r.VerifyCall("WaitForService", name, wconfig, msgCallback)
 	return mock.ErrorOrNil(call.Result[0]), call.Result[1].(time.Duration)
 }
 

pkg/serving/v1/client_mock_test.go

@@ -40,7 +40,10 @@ func TestMockKnClient(t *testing.T) {
 	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)
+	recorder.WaitForService("hello", WaitConfig{
+		Timeout:     time.Duration(10) * time.Second,
+		ErrorWindow: time.Duration(2) * time.Second,
+	}, wait.NoopMessageCallback(), nil, 10*time.Second)
 	recorder.GetRevision("hello", nil, nil)
 	recorder.ListRevisions(mock.Any(), nil, nil)
 	recorder.CreateRevision(&servingv1.Revision{}, nil)
@@ -60,7 +63,10 @@ func TestMockKnClient(t *testing.T) {
 	client.UpdateService(ctx, &servingv1.Service{})
 	client.ApplyService(ctx, &servingv1.Service{})
 	client.DeleteService(ctx, "hello", time.Duration(10)*time.Second)
-	client.WaitForService(ctx, "hello", time.Duration(10)*time.Second, wait.NoopMessageCallback())
+	client.WaitForService(ctx, "hello", WaitConfig{
+		time.Duration(10) * time.Second,
+		time.Duration(2) * time.Second,
+	}, wait.NoopMessageCallback())
 	client.GetRevision(ctx, "hello")
 	client.ListRevisions(ctx, WithName("blub"))
 	client.CreateRevision(ctx, &servingv1.Revision{})

pkg/serving/v1/client_test.go

@@ -783,17 +783,26 @@ func TestWaitForService(t *testing.T) {
 		})
 
 	t.Run("wait on a service to become ready with success", func(t *testing.T) {
-		err, duration := client.WaitForService(context.Background(), serviceName, 60*time.Second, wait.NoopMessageCallback())
+		err, duration := client.WaitForService(context.Background(), serviceName, WaitConfig{
+			Timeout:     time.Duration(10) * time.Second,
+			ErrorWindow: time.Duration(2) * time.Second,
+		}, wait.NoopMessageCallback())
 		assert.NilError(t, err)
 		assert.Assert(t, duration > 0)
 	})
 	t.Run("wait on a service to become ready with not found error", func(t *testing.T) {
-		err, duration := client.WaitForService(context.Background(), notFoundServiceName, 60*time.Second, wait.NoopMessageCallback())
+		err, duration := client.WaitForService(context.Background(), notFoundServiceName, WaitConfig{
+			Timeout:     time.Duration(10) * time.Second,
+			ErrorWindow: time.Duration(2) * time.Second,
+		}, wait.NoopMessageCallback())
 		assert.NilError(t, err)
 		assert.Assert(t, duration > 0)
 	})
 	t.Run("wait on a service to become ready with internal error", func(t *testing.T) {
-		err, duration := client.WaitForService(context.Background(), internalErrorServiceName, 60*time.Second, wait.NoopMessageCallback())
+		err, duration := client.WaitForService(context.Background(), internalErrorServiceName, WaitConfig{
+			Timeout:     time.Duration(10) * time.Second,
+			ErrorWindow: time.Duration(2) * time.Second,
+		}, wait.NoopMessageCallback())
 		assert.ErrorType(t, err, apierrors.IsInternalError)
 		assert.Assert(t, duration == 0)
 	})

pkg/serving/v1/gitops.go

@@ -199,7 +199,7 @@ func (cl *knServingGitOpsClient) DeleteService(ctx context.Context, serviceName
 }
 
 // WaitForService always returns success for this client
-func (cl *knServingGitOpsClient) WaitForService(ctx context.Context, name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration) {
+func (cl *knServingGitOpsClient) WaitForService(ctx context.Context, name string, wconfig WaitConfig, msgCallback wait.MessageCallback) (error, time.Duration) {
 	return nil, 1 * time.Second
 }
 

pkg/serving/v1/gitops_test.go

@@ -69,7 +69,10 @@ func TestGitOpsOperations(t *testing.T) {
 		assert.NilError(t, err)
 	})
 	t.Run("wait for foo service in foo namespace", func(t *testing.T) {
-		err, d := fooclient.WaitForService(context.Background(), "foo", 5*time.Second, nil)
+		err, d := fooclient.WaitForService(context.Background(), "foo", WaitConfig{
+			Timeout:     time.Duration(5) * time.Second,
+			ErrorWindow: time.Duration(2) * time.Second,
+		}, nil)
 		assert.NilError(t, err)
 		assert.Equal(t, 1*time.Second, d)
 	})