Repository: knative/client (go)

Issue:
e2e test flakes: revision tests
## In what area(s)?
e2e

## What version of Knative Client?
HEAD
```
$ kn version
Version:      v20200229-local-034a9b38-dirty
Build Date:   2020-02-29 09:28:28
Git Revision: 034a9b38
```
Note: the flake is unrelated to https://github.com/knative/client/pull/688 

## What version of Knative Serving running on your cluster?
HEAD

## Expected Behavior
revision tests pass

## Actual Behavior
revision tests flake

## Steps to Reproduce the Problem
1 - ` kn service create hello --image gcr.io/knative-samples/helloworld-go`
2 - Repeat 
```
kn revision delete `kn service describe hello -o=jsonpath={.status.latestReadyRevisionName}` && \
kn service update hello --env TARGET=next
```
3 - Most of the time the service reconciles without errors but found this at 13th update. This is the flake we see in our e2e tests.
```
➜  client git:(master)  kn revision delete `kn service describe hello -o=jsonpath={.status.latestReadyRevisionName}` && \
kn service update hello --env TARGET=next
Revision 'hello-lkcvd-12' deleted in namespace 'default'.
Updating Service 'hello' in namespace 'default':

  0.019s Revision "hello-lkcvd-12" referenced in traffic not found.
  1.696s Ingress has not yet been reconciled.
RevisionMissing: Revision "hello-lkcvd-12" referenced in traffic not found.
```

<hr>

When step 2 is executed:
  - latestReadyRevision of the service (N) is deleted, service re-points the traffic to N-1 revision and Nth revision is being reconciled.
 - Right after revision delete, service is updated too, which also deploys new revision (N+1). 

![image](https://user-images.githubusercontent.com/2510184/75605925-9d4d1b00-5b0d-11ea-9dbd-9709ccf79cd0.png)

 - If all goes well, service re-points the traffic from (N-1) revision to (N+1) revision.
 - If there is a race, error is thrown `RevisionMissing: Revision "hello-lkcvd-12" referenced in traffic not found.`.
```
➜  client git:(master)  kn revision delete `kn service describe hello -o=jsonpath={.status.latestReadyRevisionName}` && \
kn service update hello --env TARGET=next
Revision 'hello-lkcvd-12' deleted in namespace 'default'.
Updating Service 'hello' in namespace 'default':

  0.019s Revision "hello-lkcvd-12" referenced in traffic not found.
  1.696s Ingress has not yet been reconciled.
RevisionMissing: Revision "hello-lkcvd-12" referenced in traffic not found.
```
same as we see in e2e logs https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/688/pull-knative-client-integration-tests/1233406364882046976#1:build-log.txt%3A580

<hr>
I think the way we've sequenced the operations in revision tests can be improved. We want to test,
revision create and delete, but this sequence appears to put pressure on reconciliation and queues in multiple requests.

Pull request:
fix(e2e): Update revision e2e tests
 Fixes #695

## Proposed Changes
 - Re-order revision tests operations to avoid any race condition that
   may occur.

Patch:
diff --git a/docs/cmd/kn_revision_delete.md b/docs/cmd/kn_revision_delete.md
index 037e941a..5d6f4e2c 100644
--- a/docs/cmd/kn_revision_delete.md
+++ b/docs/cmd/kn_revision_delete.md
@@ -21,8 +21,11 @@ kn revision delete NAME [flags]
 ### Options
 
 ```
+      --async              DEPRECATED: please use --no-wait instead. Delete revision and don't wait for it to be deleted.
   -h, --help               help for delete
   -n, --namespace string   Specify the namespace to operate in.
+      --no-wait            Delete revision and don't wait for it to be deleted.
+      --wait-timeout int   Seconds to wait before giving up on waiting for revision to be deleted. (default 600)
 ```
 
 ### Options inherited from parent commands
diff --git a/pkg/kn/commands/revision/delete.go b/pkg/kn/commands/revision/delete.go
index 3a856004..109448e8 100644
--- a/pkg/kn/commands/revision/delete.go
+++ b/pkg/kn/commands/revision/delete.go
@@ -17,6 +17,7 @@ package revision
 import (
 	"errors"
 	"fmt"
+	"time"
 
 	"github.com/spf13/cobra"
 
@@ -25,6 +26,8 @@ import (
 
 // NewRevisionDeleteCommand represent 'revision delete' command
 func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
+	var waitFlags commands.WaitFlags
+
 	RevisionDeleteCommand := &cobra.Command{
 		Use:   "delete NAME",
 		Short: "Delete a revision.",
@@ -45,7 +48,11 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
 			}
 
 			for _, name := range args {
-				err = client.DeleteRevision(name)
+				timeout := time.Duration(0)
+				if !waitFlags.NoWait {
+					timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second
+				}
+				err = client.DeleteRevision(name, timeout)
 				if err != nil {
 					fmt.Fprintf(cmd.OutOrStdout(), "%s.\n", err)
 				} else {
@@ -56,5 +63,6 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command {
 		},
 	}
 	commands.AddNamespaceFlags(RevisionDeleteCommand.Flags(), false)
+	waitFlags.AddConditionWaitFlags(RevisionDeleteCommand, commands.WaitDefaultTimeout, "Delete", "revision", "deleted")
 	return RevisionDeleteCommand
 }
diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go
index 1a6c1def..003199be 100644
--- a/pkg/serving/v1/client.go
+++ b/pkg/serving/v1/client.go
@@ -89,7 +89,7 @@ type KnServingClient interface {
 	ListRevisions(opts ...ListConfig) (*servingv1.RevisionList, error)
 
 	// Delete a revision
-	DeleteRevision(name string) error
+	DeleteRevision(name string, timeout time.Duration) error
 
 	// Get a route by its unique name
 	GetRoute(name string) (*servingv1.Route, error)
@@ -177,6 +177,11 @@ func (cl *knServingClient) WatchService(name string, timeout time.Duration) (wat
 		cl.client.RESTClient(), cl.namespace, "services", name, timeout)
 }
 
+func (cl *knServingClient) WatchRevision(name string, timeout time.Duration) (watch.Interface, error) {
+	return wait.NewWatcher(cl.client.Revisions(cl.namespace).Watch,
+		cl.client.RESTClient(), cl.namespace, "revision", name, timeout)
+}
+
 // List services
 func (cl *knServingClient) ListServices(config ...ListConfig) (*servingv1.ServiceList, error) {
 	serviceList, err := cl.client.Services(cl.namespace).List(ListConfigs(config).toListOptions())
@@ -370,7 +375,25 @@ func getBaseRevision(cl KnServingClient, service *servingv1.Service) (*servingv1
 }
 
 // Delete a revision by name
-func (cl *knServingClient) DeleteRevision(name string) error {
+func (cl *knServingClient) DeleteRevision(name string, timeout time.Duration) error {
+	if timeout == 0 {
+		return cl.deleteRevision(name)
+	}
+	waitC := make(chan error)
+	go func() {
+		waitForEvent := wait.NewWaitForEvent("revision", cl.WatchRevision, func(evt *watch.Event) bool { return evt.Type == watch.Deleted })
+		err, _ := waitForEvent.Wait(name, timeout, wait.NoopMessageCallback())
+		waitC <- err
+	}()
+	err := cl.deleteRevision(name)
+	if err != nil {
+		return clienterrors.GetError(err)
+	}
+
+	return <-waitC
+}
+
+func (cl *knServingClient) deleteRevision(name string) error {
 	err := cl.client.Revisions(cl.namespace).Delete(name, &v1.DeleteOptions{})
 	if err != nil {
 		return clienterrors.GetError(err)
diff --git a/pkg/serving/v1/client_mock.go b/pkg/serving/v1/client_mock.go
index 9381ed34..c60583e7 100644
--- a/pkg/serving/v1/client_mock.go
+++ b/pkg/serving/v1/client_mock.go
@@ -146,12 +146,12 @@ func (c *MockKnServingClient) ListRevisions(opts ...ListConfig) (*servingv1.Revi
 }
 
 // Delete a revision
-func (sr *ServingRecorder) DeleteRevision(name interface{}, err error) {
-	sr.r.Add("DeleteRevision", []interface{}{name}, []interface{}{err})
+func (sr *ServingRecorder) DeleteRevision(name, timeout interface{}, err error) {
+	sr.r.Add("DeleteRevision", []interface{}{name, timeout}, []interface{}{err})
 }
 
-func (c *MockKnServingClient) DeleteRevision(name string) error {
-	call := c.recorder.r.VerifyCall("DeleteRevision", name)
+func (c *MockKnServingClient) DeleteRevision(name string, timeout time.Duration) error {
+	call := c.recorder.r.VerifyCall("DeleteRevision", name, timeout)
 	return mock.ErrorOrNil(call.Result[0])
 }
 


Test patch:
diff --git a/pkg/kn/commands/revision/delete_test.go b/pkg/kn/commands/revision/delete_test.go
index 53ac70dd..66d5d9fb 100644
--- a/pkg/kn/commands/revision/delete_test.go
+++ b/pkg/kn/commands/revision/delete_test.go
@@ -15,14 +15,19 @@
 package revision
 
 import (
+	"errors"
 	"testing"
 
 	"gotest.tools/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/watch"
 	clienttesting "k8s.io/client-go/testing"
 
 	"knative.dev/client/pkg/kn/commands"
 	"knative.dev/client/pkg/util"
+	"knative.dev/client/pkg/wait"
+	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
 func fakeRevisionDelete(args []string) (action clienttesting.Action, name string, output string, err error) {
@@ -35,6 +40,17 @@ func fakeRevisionDelete(args []string) (action clienttesting.Action, name string
 			name = deleteAction.GetName()
 			return true, nil, nil
 		})
+	fakeServing.AddWatchReactor("revisions",
+		func(a clienttesting.Action) (bool, watch.Interface, error) {
+			watchAction := a.(clienttesting.WatchAction)
+			_, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name")
+			if !found {
+				return true, nil, errors.New("no field selector on metadata.name found")
+			}
+			w := wait.NewFakeWatch(getRevisionDeleteEvents("test-revision"))
+			w.Start()
+			return true, w, nil
+		})
 	cmd.SetArgs(args)
 	err = cmd.Execute()
 	if err != nil {
@@ -79,3 +95,10 @@ func TestMultipleRevisionDelete(t *testing.T) {
 	assert.Check(t, util.ContainsAll(output, "Revision", revName2, "deleted", "namespace", commands.FakeNamespace))
 	assert.Check(t, util.ContainsAll(output, "Revision", revName3, "deleted", "namespace", commands.FakeNamespace))
 }
+
+func getRevisionDeleteEvents(name string) []watch.Event {
+	return []watch.Event{
+		{watch.Added, &servingv1.Revision{ObjectMeta: metav1.ObjectMeta{Name: name}}},
+		{watch.Deleted, &servingv1.Revision{ObjectMeta: metav1.ObjectMeta{Name: name}}},
+	}
+}
diff --git a/pkg/serving/v1/client_mock_test.go b/pkg/serving/v1/client_mock_test.go
index 4bf80e52..ec85dd76 100644
--- a/pkg/serving/v1/client_mock_test.go
+++ b/pkg/serving/v1/client_mock_test.go
@@ -40,7 +40,7 @@ func TestMockKnClient(t *testing.T) {
 	recorder.WaitForService("hello", time.Duration(10)*time.Second, wait.NoopMessageCallback(), nil, 10*time.Second)
 	recorder.GetRevision("hello", nil, nil)
 	recorder.ListRevisions(mock.Any(), nil, nil)
-	recorder.DeleteRevision("hello", nil)
+	recorder.DeleteRevision("hello", time.Duration(10)*time.Second, nil)
 	recorder.GetRoute("hello", nil, nil)
 	recorder.ListRoutes(mock.Any(), nil, nil)
 	recorder.GetConfiguration("hello", nil, nil)
@@ -54,7 +54,7 @@ func TestMockKnClient(t *testing.T) {
 	client.WaitForService("hello", time.Duration(10)*time.Second, wait.NoopMessageCallback())
 	client.GetRevision("hello")
 	client.ListRevisions(WithName("blub"))
-	client.DeleteRevision("hello")
+	client.DeleteRevision("hello", time.Duration(10)*time.Second)
 	client.GetRoute("hello")
 	client.ListRoutes(WithName("blub"))
 	client.GetConfiguration("hello")
diff --git a/test/e2e/revision_test.go b/test/e2e/revision_test.go
index 45a7a9ee..bd0290c7 100644
--- a/test/e2e/revision_test.go
+++ b/test/e2e/revision_test.go
@@ -45,27 +45,25 @@ func TestRevision(t *testing.T) {
 	revName := test.findRevision(t, r, "hello")
 	test.revisionDescribeWithPrintFlags(t, r, revName)
 
-	t.Log("update hello service and increase the count of configuration generation")
+	t.Log("update hello service and increase revision count to 2")
 	test.serviceUpdate(t, r, "hello", "--env", "TARGET=kn", "--port", "8888")
 
 	t.Log("show a list of revisions sorted by the count of configuration generation")
 	test.revisionListWithService(t, r, "hello")
 
-	t.Log("delete latest revision from hello service and return no error")
-	revName = test.findRevision(t, r, "hello")
-	test.revisionDelete(t, r, revName)
-
-	t.Log("delete three revisions with one revision a nonexistent")
-
-	// increase count to 2 revisions
+	t.Log("update hello service and increase revision count to 3")
 	test.serviceUpdate(t, r, "hello", "--env", "TARGET=kn", "--port", "8888")
 
+	t.Log("delete three revisions with one revision a nonexistent")
 	existRevision1 := test.findRevisionByGeneration(t, r, "hello", 1)
 	existRevision2 := test.findRevisionByGeneration(t, r, "hello", 2)
 	nonexistRevision := "hello-nonexist"
-
 	test.revisionMultipleDelete(t, r, existRevision1, existRevision2, nonexistRevision)
 
+	t.Log("delete latest revision from hello service and return no error")
+	revName = test.findRevision(t, r, "hello")
+	test.revisionDelete(t, r, revName)
+
 	t.Log("delete hello service and return no error")
 	test.serviceDelete(t, r, "hello")
 }
diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go
index 9e013916..6d26efd4 100644
--- a/test/e2e/service_options_test.go
+++ b/test/e2e/service_options_test.go
@@ -91,6 +91,12 @@ func TestServiceOptions(t *testing.T) {
 	test.validateContainerField(t, r, "svc5", "args", "[myArg1 --myArg2]")
 	test.serviceUpdate(t, r, "svc5", "--arg", "myArg1")
 	test.validateContainerField(t, r, "svc5", "args", "[myArg1]")
+
+	t.Log("create, update and validate service with user defined")
+	test.serviceCreateWithOptions(t, r, "svc6", "--user", "1001")
+	test.validateContainerField(t, r, "svc6", "securityContext.runAsUser", "1001")
+	test.serviceUpdate(t, r, "svc6", "--user", "1002")
+	test.validateContainerField(t, r, "svc6", "securityContext.runAsUser", "1002")
 }
 
 func (test *e2eTest) serviceCreateWithOptions(t *testing.T, r *KnRunResultCollector, serviceName string, options ...string) {