From 80d0001f73206220f7dee528650b3ce3f1b717ac Mon Sep 17 00:00:00 2001 From: David Simansky Date: Wed, 19 Feb 2020 10:24:40 +0100 Subject: [PATCH 01/10] feat(wait): add wait for delete operation --- docs/cmd/kn_service_delete.md | 3 ++ pkg/kn/commands/service/delete.go | 17 ++++++++ pkg/kn/commands/service/delete_mock_test.go | 7 ++++ pkg/kn/commands/service/delete_test.go | 24 ++++++++++++ pkg/serving/v1/client.go | 8 ++++ pkg/serving/v1/client_mock.go | 10 +++++ pkg/wait/wait_for_ready.go | 43 ++++++++++++++++++++- 7 files changed, 110 insertions(+), 2 deletions(-) diff --git a/docs/cmd/kn_service_delete.md b/docs/cmd/kn_service_delete.md index c778fe4d4f..3a43fdb162 100644 --- a/docs/cmd/kn_service_delete.md +++ b/docs/cmd/kn_service_delete.md @@ -24,8 +24,11 @@ kn service delete NAME [flags] ### Options ``` + --async DEPRECATED: please use --no-wait instead. Delete service and don't wait for it to become ready. -h, --help help for delete -n, --namespace string Specify the namespace to operate in. + --no-wait Delete service and don't wait for it to become ready. + --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 600) ``` ### Options inherited from parent commands diff --git a/pkg/kn/commands/service/delete.go b/pkg/kn/commands/service/delete.go index 7fd0de1366..6e7ffac880 100644 --- a/pkg/kn/commands/service/delete.go +++ b/pkg/kn/commands/service/delete.go @@ -17,14 +17,18 @@ package service import ( "errors" "fmt" + "time" "github.com/spf13/cobra" + "k8s.io/apimachinery/pkg/watch" "knative.dev/client/pkg/kn/commands" ) // NewServiceDeleteCommand represent 'service delete' command func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command { + var waitFlags commands.WaitFlags + serviceDeleteCommand := &cobra.Command{ Use: "delete NAME", Short: "Delete a service.", @@ -50,7 +54,19 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command { } for _, name := range args { + waitC := make(chan error) + defer close(waitC) + if !waitFlags.NoWait { + go func(s string, c chan error) { + err := client.WaitForEvent("service", s, time.Duration(waitFlags.TimeoutInSeconds)*time.Second, + func(evt *watch.Event) bool { return evt.Type == watch.Deleted }) + c <- err + }(name, waitC) + } err = client.DeleteService(name) + if err == nil && !waitFlags.NoWait { + err = <-waitC + } if err != nil { fmt.Fprintf(cmd.OutOrStdout(), "%s.\n", err) } else { @@ -61,5 +77,6 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command { }, } commands.AddNamespaceFlags(serviceDeleteCommand.Flags(), false) + waitFlags.AddConditionWaitFlags(serviceDeleteCommand, commands.WaitDefaultTimeout, "Delete", "service") return serviceDeleteCommand } diff --git a/pkg/kn/commands/service/delete_mock_test.go b/pkg/kn/commands/service/delete_mock_test.go index 4e9ac9d960..87dfe8bee8 100644 --- a/pkg/kn/commands/service/delete_mock_test.go +++ b/pkg/kn/commands/service/delete_mock_test.go @@ -21,6 +21,7 @@ import ( clientservingv1 "knative.dev/client/pkg/serving/v1" "knative.dev/client/pkg/util" + "knative.dev/client/pkg/util/mock" ) func TestServiceDeleteMock(t *testing.T) { @@ -31,6 +32,8 @@ func TestServiceDeleteMock(t *testing.T) { r := client.Recorder() r.DeleteService("foo", nil) + // Wait for delete event + r.WaitForEvent("service", "foo", mock.Any(), mock.Any(), nil) output, err := executeServiceCommand(client, "delete", "foo") assert.NilError(t, err) @@ -46,6 +49,10 @@ func TestMultipleServiceDeleteMock(t *testing.T) { // Recording: r := client.Recorder() + // Wait for delete event + r.WaitForEvent("service", "foo", mock.Any(), mock.Any(), nil) + r.WaitForEvent("service", "bar", mock.Any(), mock.Any(), nil) + r.WaitForEvent("service", "baz", mock.Any(), mock.Any(), nil) r.DeleteService("foo", nil) r.DeleteService("bar", nil) diff --git a/pkg/kn/commands/service/delete_test.go b/pkg/kn/commands/service/delete_test.go index 3668ba41a0..3dba0e0f8b 100644 --- a/pkg/kn/commands/service/delete_test.go +++ b/pkg/kn/commands/service/delete_test.go @@ -17,12 +17,17 @@ package service import ( "testing" + "github.com/pkg/errors" "gotest.tools/assert" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/watch" client_testing "k8s.io/client-go/testing" + clienttesting "k8s.io/client-go/testing" "knative.dev/client/pkg/kn/commands" "knative.dev/client/pkg/util" + "knative.dev/client/pkg/wait" ) func fakeServiceDelete(args []string) (action client_testing.Action, name string, output string, err error) { @@ -35,6 +40,17 @@ func fakeServiceDelete(args []string) (action client_testing.Action, name string name = deleteAction.GetName() return true, nil, nil }) + fakeServing.AddWatchReactor("services", + func(a client_testing.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(getServiceDeleteEvents("test-service")) + w.Start() + return true, w, nil + }) cmd.SetArgs(args) err = cmd.Execute() if err != nil { @@ -79,3 +95,11 @@ func TestMultipleServiceDelete(t *testing.T) { assert.Check(t, util.ContainsAll(output, "Service", sevName2, "deleted", "namespace", commands.FakeNamespace)) assert.Check(t, util.ContainsAll(output, "Service", sevName3, "deleted", "namespace", commands.FakeNamespace)) } + +func getServiceDeleteEvents(name string) []watch.Event { + return []watch.Event{ + {watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")}, + {watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", "msg2")}, + {watch.Deleted, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")}, + } +} diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index 5cf196608a..4b9b41dbbf 100644 --- a/pkg/serving/v1/client.go +++ b/pkg/serving/v1/client.go @@ -75,6 +75,8 @@ type KnServingClient interface { // Return error and how long has been waited WaitForService(name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration) + WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error + // Get a configuration by name GetConfiguration(name string) (*servingv1.Configuration, error) @@ -271,6 +273,12 @@ func (cl *knServingClient) WaitForService(name string, timeout time.Duration, ms return waitForReady.Wait(name, timeout, msgCallback) } +func (cl *knServingClient) WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error { + waitForEvent := wait.NewWaitForEvent(kind, cl.WatchService, done) + err, _ := waitForEvent.Wait(name, timeout, wait.NoopMessageCallback()) + return err +} + // Get the configuration for a service func (cl *knServingClient) GetConfiguration(name string) (*servingv1.Configuration, error) { configuration, err := cl.client.Configurations(cl.namespace).Get(name, v1.GetOptions{}) diff --git a/pkg/serving/v1/client_mock.go b/pkg/serving/v1/client_mock.go index b0000d7665..2dda0891bc 100644 --- a/pkg/serving/v1/client_mock.go +++ b/pkg/serving/v1/client_mock.go @@ -125,6 +125,16 @@ func (c *MockKnServingClient) WaitForService(name string, timeout time.Duration, return mock.ErrorOrNil(call.Result[0]), call.Result[1].(time.Duration) } +// Wait for a service to become ready, but not longer than provided timeout +func (sr *ServingRecorder) WaitForEvent(kind, name interface{}, timeout interface{}, done interface{}, err error) { + sr.r.Add("WaitForEvent", []interface{}{kind, name, timeout, done}, []interface{}{err}) +} + +func (c *MockKnServingClient) WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error { + call := c.recorder.r.VerifyCall("WaitForEvent", kind, name, timeout, done) + return mock.ErrorOrNil(call.Result[0]) +} + // Get a revision by name func (sr *ServingRecorder) GetRevision(name interface{}, revision *servingv1.Revision, err error) { sr.r.Add("GetRevision", []interface{}{name}, []interface{}{revision, err}) diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go index e4c51776ad..7d9c2a15a6 100644 --- a/pkg/wait/wait_for_ready.go +++ b/pkg/wait/wait_for_ready.go @@ -36,9 +36,17 @@ type waitForReadyConfig struct { kind string } +type waitForEvent struct { + watchMaker WatchMaker + eventDone EventDone + kind string +} + +type EventDone func(ev *watch.Event) bool + // Interface used for waiting of a resource of a given name to reach a definitive // state in its "Ready" condition. -type WaitForReady interface { +type Wait interface { // Wait on resource the resource with this name until a given timeout // and write event messages for unknown event to the status writer. @@ -56,7 +64,7 @@ type ConditionsExtractor func(obj runtime.Object) (apis.Conditions, error) type MessageCallback func(durationSinceState time.Duration, message string) // Constructor with resource type specific configuration -func NewWaitForReady(kind string, watchMaker WatchMaker, extractor ConditionsExtractor) WaitForReady { +func NewWaitForReady(kind string, watchMaker WatchMaker, extractor ConditionsExtractor) Wait { return &waitForReadyConfig{ kind: kind, watchMaker: watchMaker, @@ -64,6 +72,14 @@ func NewWaitForReady(kind string, watchMaker WatchMaker, extractor ConditionsExt } } +func NewWaitForEvent(kind string, watchMaker WatchMaker, eventDone EventDone) Wait { + return &waitForEvent{ + kind: kind, + watchMaker: watchMaker, + eventDone: eventDone, + } +} + // A simple message callback which prints out messages line by line func SimpleMessageCallback(out io.Writer) MessageCallback { oldMessage := "" @@ -178,6 +194,29 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string, } } +func (w *waitForEvent) Wait(name string, timeout time.Duration, msgCallback MessageCallback) (error, time.Duration) { + watcher, err := w.watchMaker(name, timeout) + if err != nil { + return err, 0 + } + defer watcher.Stop() + start := time.Now() + // channel used to transport the error + errChan := make(chan error) + for { + select { + case <-time.After(timeout): + return nil, time.Since(start) + case err = <-errChan: + return err, time.Since(start) + case event := <-watcher.ResultChan(): + if w.eventDone(&event) { + return nil, time.Since(start) + } + } + } +} + // Going over Unstructured to keep that function generally applicable. // Alternative implemenentation: Add a func-field to waitForReadyConfig which has to be // provided for every resource (like the conditions extractor) From adf36de2bd5106662e22f84b361fc694c9d69a40 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 21 Feb 2020 14:04:34 +0100 Subject: [PATCH 02/10] feat(wait): refactor initial impl --- pkg/kn/commands/service/delete.go | 16 ++------- pkg/kn/commands/service/delete_mock_test.go | 30 ++++++++++++----- pkg/serving/v1/client.go | 29 +++++++++++----- pkg/serving/v1/client_mock.go | 18 +++------- pkg/serving/v1/client_mock_test.go | 4 +-- pkg/serving/v1/client_test.go | 23 +++++++++++-- pkg/wait/wait_for_ready.go | 2 +- pkg/wait/wait_for_ready_test.go | 37 +++++++++++++++++++++ 8 files changed, 109 insertions(+), 50 deletions(-) diff --git a/pkg/kn/commands/service/delete.go b/pkg/kn/commands/service/delete.go index 6e7ffac880..632c40f759 100644 --- a/pkg/kn/commands/service/delete.go +++ b/pkg/kn/commands/service/delete.go @@ -20,7 +20,6 @@ import ( "time" "github.com/spf13/cobra" - "k8s.io/apimachinery/pkg/watch" "knative.dev/client/pkg/kn/commands" ) @@ -52,21 +51,12 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command { if err != nil { return err } - for _, name := range args { - waitC := make(chan error) - defer close(waitC) + timeout := time.Duration(0) if !waitFlags.NoWait { - go func(s string, c chan error) { - err := client.WaitForEvent("service", s, time.Duration(waitFlags.TimeoutInSeconds)*time.Second, - func(evt *watch.Event) bool { return evt.Type == watch.Deleted }) - c <- err - }(name, waitC) - } - err = client.DeleteService(name) - if err == nil && !waitFlags.NoWait { - err = <-waitC + timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second } + err = client.DeleteService(name, timeout) if err != nil { fmt.Fprintf(cmd.OutOrStdout(), "%s.\n", err) } else { diff --git a/pkg/kn/commands/service/delete_mock_test.go b/pkg/kn/commands/service/delete_mock_test.go index 87dfe8bee8..f901b9ddb1 100644 --- a/pkg/kn/commands/service/delete_mock_test.go +++ b/pkg/kn/commands/service/delete_mock_test.go @@ -31,9 +31,7 @@ func TestServiceDeleteMock(t *testing.T) { // Recording: r := client.Recorder() - r.DeleteService("foo", nil) - // Wait for delete event - r.WaitForEvent("service", "foo", mock.Any(), mock.Any(), nil) + r.DeleteService("foo", mock.Any(), nil) output, err := executeServiceCommand(client, "delete", "foo") assert.NilError(t, err) @@ -43,6 +41,23 @@ func TestServiceDeleteMock(t *testing.T) { } +func TestServiceDeleteMockNoWait(t *testing.T) { + // New mock client + client := clientservingv1.NewMockKnServiceClient(t) + + // Recording: + r := client.Recorder() + + r.DeleteService("foo", mock.Any(), nil) + + output, err := executeServiceCommand(client, "delete", "foo", "--no-wait") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "deleted", "foo", "default")) + + r.Validate() + +} + func TestMultipleServiceDeleteMock(t *testing.T) { // New mock client client := clientservingv1.NewMockKnServiceClient(t) @@ -50,13 +65,10 @@ func TestMultipleServiceDeleteMock(t *testing.T) { // Recording: r := client.Recorder() // Wait for delete event - r.WaitForEvent("service", "foo", mock.Any(), mock.Any(), nil) - r.WaitForEvent("service", "bar", mock.Any(), mock.Any(), nil) - r.WaitForEvent("service", "baz", mock.Any(), mock.Any(), nil) - r.DeleteService("foo", nil) - r.DeleteService("bar", nil) - r.DeleteService("baz", nil) + r.DeleteService("foo", mock.Any(), nil) + r.DeleteService("bar", mock.Any(), nil) + r.DeleteService("baz", mock.Any(), nil) output, err := executeServiceCommand(client, "delete", "foo", "bar", "baz") assert.NilError(t, err) diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index 4b9b41dbbf..ef8a553a48 100644 --- a/pkg/serving/v1/client.go +++ b/pkg/serving/v1/client.go @@ -69,13 +69,13 @@ type KnServingClient interface { UpdateServiceWithRetry(name string, updateFunc serviceUpdateFunc, nrRetries int) error // Delete a service by name - DeleteService(name string) error + DeleteService(name string, timeout time.Duration) error // Wait for a service to become ready, but not longer than provided timeout. // Return error and how long has been waited WaitForService(name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration) - WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error + // WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error // Get a configuration by name GetConfiguration(name string) (*servingv1.Configuration, error) @@ -255,7 +255,24 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU } // Delete a service by name -func (cl *knServingClient) DeleteService(serviceName string) error { +func (cl *knServingClient) DeleteService(serviceName string, timeout time.Duration) error { + if timeout == 0 { + return cl.deleteService(serviceName) + } + waitC := make(chan error) + go func(name string, c chan error) { + waitForEvent := wait.NewWaitForEvent("service", cl.WatchService, func(evt *watch.Event) bool { return evt.Type == watch.Deleted }) + err, _ := waitForEvent.Wait(name, timeout, wait.NoopMessageCallback()) + c <- err + }(serviceName, waitC) + err := cl.deleteService(serviceName) + if err != nil { + return err + } + return <-waitC +} + +func (cl *knServingClient) deleteService(serviceName string) error { err := cl.client.Services(cl.namespace).Delete( serviceName, &v1.DeleteOptions{}, @@ -273,12 +290,6 @@ func (cl *knServingClient) WaitForService(name string, timeout time.Duration, ms return waitForReady.Wait(name, timeout, msgCallback) } -func (cl *knServingClient) WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error { - waitForEvent := wait.NewWaitForEvent(kind, cl.WatchService, done) - err, _ := waitForEvent.Wait(name, timeout, wait.NoopMessageCallback()) - return err -} - // Get the configuration for a service func (cl *knServingClient) GetConfiguration(name string) (*servingv1.Configuration, error) { configuration, err := cl.client.Configurations(cl.namespace).Get(name, v1.GetOptions{}) diff --git a/pkg/serving/v1/client_mock.go b/pkg/serving/v1/client_mock.go index 2dda0891bc..9381ed34ca 100644 --- a/pkg/serving/v1/client_mock.go +++ b/pkg/serving/v1/client_mock.go @@ -106,12 +106,12 @@ func (c *MockKnServingClient) UpdateServiceWithRetry(name string, updateFunc ser } // Delete a service by name -func (sr *ServingRecorder) DeleteService(name interface{}, err error) { - sr.r.Add("DeleteService", []interface{}{name}, []interface{}{err}) +func (sr *ServingRecorder) DeleteService(name, timeout interface{}, err error) { + sr.r.Add("DeleteService", []interface{}{name, timeout}, []interface{}{err}) } -func (c *MockKnServingClient) DeleteService(name string) error { - call := c.recorder.r.VerifyCall("DeleteService", name) +func (c *MockKnServingClient) DeleteService(name string, timeout time.Duration) error { + call := c.recorder.r.VerifyCall("DeleteService", name, timeout) return mock.ErrorOrNil(call.Result[0]) } @@ -125,16 +125,6 @@ func (c *MockKnServingClient) WaitForService(name string, timeout time.Duration, return mock.ErrorOrNil(call.Result[0]), call.Result[1].(time.Duration) } -// Wait for a service to become ready, but not longer than provided timeout -func (sr *ServingRecorder) WaitForEvent(kind, name interface{}, timeout interface{}, done interface{}, err error) { - sr.r.Add("WaitForEvent", []interface{}{kind, name, timeout, done}, []interface{}{err}) -} - -func (c *MockKnServingClient) WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error { - call := c.recorder.r.VerifyCall("WaitForEvent", kind, name, timeout, done) - return mock.ErrorOrNil(call.Result[0]) -} - // Get a revision by name func (sr *ServingRecorder) GetRevision(name interface{}, revision *servingv1.Revision, err error) { sr.r.Add("GetRevision", []interface{}{name}, []interface{}{revision, err}) diff --git a/pkg/serving/v1/client_mock_test.go b/pkg/serving/v1/client_mock_test.go index a2ee9dc0ae..4bf80e5240 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.CreateService(&servingv1.Service{}, nil) recorder.UpdateService(&servingv1.Service{}, nil) - recorder.DeleteService("hello", 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.GetRevision("hello", nil, nil) recorder.ListRevisions(mock.Any(), nil, nil) @@ -50,7 +50,7 @@ func TestMockKnClient(t *testing.T) { client.ListServices(WithName("blub")) client.CreateService(&servingv1.Service{}) client.UpdateService(&servingv1.Service{}) - client.DeleteService("hello") + client.DeleteService("hello", time.Duration(10)*time.Second) client.WaitForService("hello", time.Duration(10)*time.Second, wait.NoopMessageCallback()) client.GetRevision("hello") client.ListRevisions(WithName("blub")) diff --git a/pkg/serving/v1/client_test.go b/pkg/serving/v1/client_test.go index 36821c6630..2b9e0192d2 100644 --- a/pkg/serving/v1/client_test.go +++ b/pkg/serving/v1/client_test.go @@ -179,19 +179,38 @@ func TestDeleteService(t *testing.T) { } return true, nil, errors.NewNotFound(servingv1.Resource("service"), name) }) + serving.AddWatchReactor("services", + func(a clienttesting.Action) (bool, watch.Interface, error) { + watchAction := a.(clienttesting.WatchAction) + name, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name") + if !found { + return true, nil, errors.NewNotFound(servingv1.Resource("service"), name) + } + w := wait.NewFakeWatch(getServiceDeleteEvents("test-service")) + w.Start() + return true, w, nil + }) t.Run("delete existing service returns no error", func(t *testing.T) { - err := client.DeleteService(serviceName) + err := client.DeleteService(serviceName, time.Duration(10)*time.Second) assert.NilError(t, err) }) t.Run("trying to delete non-existing service returns error", func(t *testing.T) { - err := client.DeleteService(nonExistingServiceName) + err := client.DeleteService(nonExistingServiceName, time.Duration(10)*time.Second) assert.ErrorContains(t, err, "not found") assert.ErrorContains(t, err, nonExistingServiceName) }) } +func getServiceDeleteEvents(name string) []watch.Event { + return []watch.Event{ + {watch.Added, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")}, + {watch.Modified, wait.CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", "msg2")}, + {watch.Deleted, wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")}, + } +} + func TestGetRevision(t *testing.T) { serving, client := setup() diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go index 7d9c2a15a6..104a1b3abc 100644 --- a/pkg/wait/wait_for_ready.go +++ b/pkg/wait/wait_for_ready.go @@ -206,7 +206,7 @@ func (w *waitForEvent) Wait(name string, timeout time.Duration, msgCallback Mess for { select { case <-time.After(timeout): - return nil, time.Since(start) + return fmt.Errorf("timeout: %s '%s' not ready after %d seconds", w.kind, name, int(timeout/time.Second)), time.Since(start) case err = <-errChan: return err, time.Since(start) case event := <-watcher.ResultChan(): diff --git a/pkg/wait/wait_for_ready_test.go b/pkg/wait/wait_for_ready_test.go index 6ff89d8cd5..a09ca61c66 100644 --- a/pkg/wait/wait_for_ready_test.go +++ b/pkg/wait/wait_for_ready_test.go @@ -76,6 +76,28 @@ func TestAddWaitForReady(t *testing.T) { } } +func TestAddWaitForDelete(t *testing.T) { + for i, tc := range prepareDeleteTestCases("test-service") { + fakeWatchApi := NewFakeWatch(tc.events) + + waitForEvent := NewWaitForEvent( + "blub", + func(name string, timeout time.Duration) (watch.Interface, error) { + return fakeWatchApi, nil + }, + func(evt *watch.Event) bool { return evt.Type == watch.Deleted }) + fakeWatchApi.Start() + + waitForEvent.Wait("foobar", tc.timeout, NoopMessageCallback()) + close(fakeWatchApi.eventChan) + + if fakeWatchApi.StopCalled != 1 { + t.Errorf("%d: Exactly one 'stop' should be called, but got %d", i, fakeWatchApi.StopCalled) + } + + } +} + // Test cases which consists of a series of events to send and the expected behaviour. func prepareTestCases(name string) []waitForReadyTestCase { return []waitForReadyTestCase{ @@ -87,6 +109,12 @@ func prepareTestCases(name string) []waitForReadyTestCase { } } +func prepareDeleteTestCases(name string) []waitForReadyTestCase { + return []waitForReadyTestCase{ + tc(deNormal, name, time.Second, ""), + } +} + func errorTest(name string) waitForReadyTestCase { events := []watch.Event{ {watch.Added, CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", "msg1")}, @@ -150,3 +178,12 @@ func peReadyFalseWithinErrorWindow(name string) ([]watch.Event, int) { {watch.Modified, CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "Route ready", "")}, }, len(messages) } + +func deNormal(name string) ([]watch.Event, int) { + messages := pMessages(2) + return []watch.Event{ + {watch.Added, CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0])}, + {watch.Modified, CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", messages[1])}, + {watch.Deleted, CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")}, + }, len(messages) +} From 01d2b0f5df1c4f6a8ca30ff1e78de96f07ac12a7 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 21 Feb 2020 14:40:30 +0100 Subject: [PATCH 03/10] fix(lint): add docs to new functions --- pkg/wait/wait_for_ready.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go index 104a1b3abc..8f38b95526 100644 --- a/pkg/wait/wait_for_ready.go +++ b/pkg/wait/wait_for_ready.go @@ -36,12 +36,14 @@ type waitForReadyConfig struct { kind string } +// Callbacks and configuration used while waiting for event type waitForEvent struct { watchMaker WatchMaker eventDone EventDone kind string } +// Done marker to stop actual waiting on given event state type EventDone func(ev *watch.Event) bool // Interface used for waiting of a resource of a given name to reach a definitive @@ -194,6 +196,7 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string, } } +// Wait until the expected EventDone is satisfied func (w *waitForEvent) Wait(name string, timeout time.Duration, msgCallback MessageCallback) (error, time.Duration) { watcher, err := w.watchMaker(name, timeout) if err != nil { From 9221ab85f60519a9bb5289fc2639c99cc03a3c38 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 21 Feb 2020 16:00:17 +0100 Subject: [PATCH 04/10] fix: reflect code review comments --- pkg/serving/v1/client.go | 12 ++++++------ pkg/wait/wait_for_ready.go | 10 +++++----- pkg/wait/wait_for_ready_test.go | 27 ++++++++++++++++++++------- 3 files changed, 31 insertions(+), 18 deletions(-) diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index ef8a553a48..6dc0e4dff7 100644 --- a/pkg/serving/v1/client.go +++ b/pkg/serving/v1/client.go @@ -75,8 +75,6 @@ type KnServingClient interface { // Return error and how long has been waited WaitForService(name string, timeout time.Duration, msgCallback wait.MessageCallback) (error, time.Duration) - // WaitForEvent(kind, name string, timeout time.Duration, done wait.EventDone) error - // Get a configuration by name GetConfiguration(name string) (*servingv1.Configuration, error) @@ -255,16 +253,18 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU } // Delete a service by name +// Param `timeout` represents a duration to wait for a delete op to finish. +// For `timeout == 0` delete is performed async without any wait. func (cl *knServingClient) DeleteService(serviceName string, timeout time.Duration) error { if timeout == 0 { return cl.deleteService(serviceName) } waitC := make(chan error) - go func(name string, c chan error) { + go func() { waitForEvent := wait.NewWaitForEvent("service", cl.WatchService, func(evt *watch.Event) bool { return evt.Type == watch.Deleted }) - err, _ := waitForEvent.Wait(name, timeout, wait.NoopMessageCallback()) - c <- err - }(serviceName, waitC) + err, _ := waitForEvent.Wait(serviceName, timeout, wait.NoopMessageCallback()) + waitC <- err + }() err := cl.deleteService(serviceName) if err != nil { return err diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go index 8f38b95526..479b543903 100644 --- a/pkg/wait/wait_for_ready.go +++ b/pkg/wait/wait_for_ready.go @@ -43,7 +43,7 @@ type waitForEvent struct { kind string } -// Done marker to stop actual waiting on given event state +// EventDone is a marker to stop actual waiting on given event state type EventDone func(ev *watch.Event) bool // Interface used for waiting of a resource of a given name to reach a definitive @@ -137,6 +137,7 @@ func (w *waitForReadyConfig) waitForReadyCondition(start time.Time, name string, // channel used to transport the error errChan := make(chan error) + var errorTimer *time.Timer // Stop error timer if it has been started because of // a ConditionReady has been set to false @@ -206,9 +207,11 @@ func (w *waitForEvent) Wait(name string, timeout time.Duration, msgCallback Mess start := time.Now() // channel used to transport the error errChan := make(chan error) + timer := time.NewTimer(timeout) + defer timer.Stop() for { select { - case <-time.After(timeout): + case <-timer.C: return fmt.Errorf("timeout: %s '%s' not ready after %d seconds", w.kind, name, int(timeout/time.Second)), time.Since(start) case err = <-errChan: return err, time.Since(start) @@ -220,9 +223,6 @@ func (w *waitForEvent) Wait(name string, timeout time.Duration, msgCallback Mess } } -// Going over Unstructured to keep that function generally applicable. -// Alternative implemenentation: Add a func-field to waitForReadyConfig which has to be -// provided for every resource (like the conditions extractor) func isGivenEqualsObservedGeneration(object runtime.Object) (bool, error) { unstructured, err := runtime.DefaultUnstructuredConverter.ToUnstructured(object) if err != nil { diff --git a/pkg/wait/wait_for_ready_test.go b/pkg/wait/wait_for_ready_test.go index a09ca61c66..cf1ebdde31 100644 --- a/pkg/wait/wait_for_ready_test.go +++ b/pkg/wait/wait_for_ready_test.go @@ -78,21 +78,33 @@ func TestAddWaitForReady(t *testing.T) { func TestAddWaitForDelete(t *testing.T) { for i, tc := range prepareDeleteTestCases("test-service") { - fakeWatchApi := NewFakeWatch(tc.events) + fakeWatchAPI := NewFakeWatch(tc.events) waitForEvent := NewWaitForEvent( "blub", func(name string, timeout time.Duration) (watch.Interface, error) { - return fakeWatchApi, nil + return fakeWatchAPI, nil }, func(evt *watch.Event) bool { return evt.Type == watch.Deleted }) - fakeWatchApi.Start() + fakeWatchAPI.Start() - waitForEvent.Wait("foobar", tc.timeout, NoopMessageCallback()) - close(fakeWatchApi.eventChan) + err, _ := waitForEvent.Wait("foobar", tc.timeout, NoopMessageCallback()) + close(fakeWatchAPI.eventChan) - if fakeWatchApi.StopCalled != 1 { - t.Errorf("%d: Exactly one 'stop' should be called, but got %d", i, fakeWatchApi.StopCalled) + if tc.errorText == "" && err != nil { + t.Errorf("%d: Error received %v", i, err) + continue + } + if tc.errorText != "" { + if err == nil { + t.Errorf("%d: No error but expected one", i) + } else { + assert.ErrorContains(t, err, tc.errorText) + } + } + + if fakeWatchAPI.StopCalled != 1 { + t.Errorf("%d: Exactly one 'stop' should be called, but got %d", i, fakeWatchAPI.StopCalled) } } @@ -112,6 +124,7 @@ func prepareTestCases(name string) []waitForReadyTestCase { func prepareDeleteTestCases(name string) []waitForReadyTestCase { return []waitForReadyTestCase{ tc(deNormal, name, time.Second, ""), + tc(peTimeout, name, 10*time.Second, "timeout"), } } From 44e33d707755b353d378c3dfa75183bd878ad009 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 21 Feb 2020 16:15:36 +0100 Subject: [PATCH 05/10] feat(wait): add changelog entry --- CHANGELOG.adoc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index c7d3bf4cdb..718530fa5e 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -52,6 +52,11 @@ | 🐛 | Fix `--image` flag to only allow single occurence | https://github.com/knative/client/pull/647[#647] + +| 🧽 +| Add `--wait` and `--no-wait` to service delete operation. +| Change service delete to wait by default. +| https://github.com/knative/client/pull/682[#682] |=== ## v0.12.0 (2020-01-29) From e0cc636c242bc676dc30910e76933e3cd9d07de3 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 21 Feb 2020 16:22:12 +0100 Subject: [PATCH 06/10] fix: changelog entry to render correctly --- CHANGELOG.adoc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 718530fa5e..037d98a218 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -45,8 +45,7 @@ | https://github.com/knative/client/pull/639[#639] | 🧽 -| Refactor service `create_test.go` and `update_test.go` to -| remove unecessary `sync` parameter in setup call +| Refactor service `create_test.go` and `update_test.go` to remove unecessary `sync` parameter in setup call | https://github.com/knative/client/pull/656[#656] | 🐛 @@ -54,8 +53,7 @@ | https://github.com/knative/client/pull/647[#647] | 🧽 -| Add `--wait` and `--no-wait` to service delete operation. -| Change service delete to wait by default. +| Add `--wait` and `--no-wait` to service delete operation. Change service delete to wait by default. | https://github.com/knative/client/pull/682[#682] |=== From b97e2ca8ffadd478bf1f194cce2379f137370476 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 21 Feb 2020 17:21:32 +0100 Subject: [PATCH 07/10] fix: usage message of wait flag --- docs/cmd/kn_service_create.md | 4 ++-- docs/cmd/kn_service_delete.md | 6 +++--- docs/cmd/kn_service_update.md | 4 ++-- pkg/kn/commands/service/create.go | 2 +- pkg/kn/commands/service/delete.go | 2 +- pkg/kn/commands/service/update.go | 2 +- pkg/kn/commands/wait_flags.go | 6 +++--- pkg/kn/commands/wait_flags_test.go | 9 ++++++--- 8 files changed, 19 insertions(+), 16 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 74f0fd709a..ce4f4a99ff 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -44,7 +44,7 @@ kn service create NAME --image IMAGE [flags] ``` --annotation stringArray Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-). --arg stringArray Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times. - --async DEPRECATED: please use --no-wait instead. Create service and don't wait for it to become ready. + --async DEPRECATED: please use --no-wait instead. Create service and don't wait for it to be ready. --autoscale-window string Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s) --cmd string Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd /app/start --arg myArg to pass aditional arguments. --concurrency-limit int Hard Limit of concurrent requests to be processed by a single replica. @@ -63,7 +63,7 @@ kn service create NAME --image IMAGE [flags] --mount stringArray Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume. -n, --namespace string Specify the namespace to operate in. --no-lock-to-digest Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) - --no-wait Create service and don't wait for it to become ready. + --no-wait Create service and don't wait for it to be ready. -p, --port int32 The port where application listens on. --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. --requests-cpu string The requested CPU (e.g., 250m). diff --git a/docs/cmd/kn_service_delete.md b/docs/cmd/kn_service_delete.md index 3a43fdb162..eef536e552 100644 --- a/docs/cmd/kn_service_delete.md +++ b/docs/cmd/kn_service_delete.md @@ -24,11 +24,11 @@ kn service delete NAME [flags] ### Options ``` - --async DEPRECATED: please use --no-wait instead. Delete service and don't wait for it to become ready. + --async DEPRECATED: please use --no-wait instead. Delete service 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 service and don't wait for it to become ready. - --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 600) + --no-wait Delete service and don't wait for it to be deleted. + --wait-timeout int Seconds to wait before giving up on waiting for service to be deleted. (default 600) ``` ### Options inherited from parent commands diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index d854f9b26d..0792fa9708 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -40,7 +40,7 @@ kn service update NAME [flags] ``` --annotation stringArray Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-). --arg stringArray Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times. - --async DEPRECATED: please use --no-wait instead. Update service and don't wait for it to become ready. + --async DEPRECATED: please use --no-wait instead. Update service and don't wait for it to be ready. --autoscale-window string Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s) --cmd string Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd /app/start --arg myArg to pass aditional arguments. --concurrency-limit int Hard Limit of concurrent requests to be processed by a single replica. @@ -58,7 +58,7 @@ kn service update NAME [flags] --mount stringArray Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume. -n, --namespace string Specify the namespace to operate in. --no-lock-to-digest Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) - --no-wait Update service and don't wait for it to become ready. + --no-wait Update service and don't wait for it to be ready. -p, --port int32 The port where application listens on. --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. --requests-cpu string The requested CPU (e.g., 250m). diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go index 2fd79d6dfa..6667cd95f7 100644 --- a/pkg/kn/commands/service/create.go +++ b/pkg/kn/commands/service/create.go @@ -114,7 +114,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command { } commands.AddNamespaceFlags(serviceCreateCommand.Flags(), false) editFlags.AddCreateFlags(serviceCreateCommand) - waitFlags.AddConditionWaitFlags(serviceCreateCommand, commands.WaitDefaultTimeout, "Create", "service") + waitFlags.AddConditionWaitFlags(serviceCreateCommand, commands.WaitDefaultTimeout, "Create", "service", "ready") return serviceCreateCommand } diff --git a/pkg/kn/commands/service/delete.go b/pkg/kn/commands/service/delete.go index 632c40f759..81dc8f9612 100644 --- a/pkg/kn/commands/service/delete.go +++ b/pkg/kn/commands/service/delete.go @@ -67,6 +67,6 @@ func NewServiceDeleteCommand(p *commands.KnParams) *cobra.Command { }, } commands.AddNamespaceFlags(serviceDeleteCommand.Flags(), false) - waitFlags.AddConditionWaitFlags(serviceDeleteCommand, commands.WaitDefaultTimeout, "Delete", "service") + waitFlags.AddConditionWaitFlags(serviceDeleteCommand, commands.WaitDefaultTimeout, "Delete", "service", "deleted") return serviceDeleteCommand } diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go index c5d419e92f..b0589e0a1d 100644 --- a/pkg/kn/commands/service/update.go +++ b/pkg/kn/commands/service/update.go @@ -136,7 +136,7 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command { commands.AddNamespaceFlags(serviceUpdateCommand.Flags(), false) editFlags.AddUpdateFlags(serviceUpdateCommand) - waitFlags.AddConditionWaitFlags(serviceUpdateCommand, commands.WaitDefaultTimeout, "Update", "service") + waitFlags.AddConditionWaitFlags(serviceUpdateCommand, commands.WaitDefaultTimeout, "Update", "service", "ready") trafficFlags.Add(serviceUpdateCommand) return serviceUpdateCommand } diff --git a/pkg/kn/commands/wait_flags.go b/pkg/kn/commands/wait_flags.go index bb459157d0..ca78368568 100644 --- a/pkg/kn/commands/wait_flags.go +++ b/pkg/kn/commands/wait_flags.go @@ -39,12 +39,12 @@ type WaitFlags struct { // Add flags which influence the sync/async behaviour when creating or updating // resources. Set `waitDefault` argument if the default behaviour is synchronous. // Use `what` for describing what is waited for. -func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDefault int, action string, what string) { - waitUsage := fmt.Sprintf("%s %s and don't wait for it to become ready.", action, what) +func (p *WaitFlags) AddConditionWaitFlags(command *cobra.Command, waitTimeoutDefault int, action, what, until string) { + waitUsage := fmt.Sprintf("%s %s and don't wait for it to be %s.", action, what, until) //TODO: deprecated flag should be removed in next release command.Flags().BoolVar(&p.Async, "async", false, "DEPRECATED: please use --no-wait instead. "+waitUsage) command.Flags().BoolVar(&p.NoWait, "no-wait", false, waitUsage) - timeoutUsage := fmt.Sprintf("Seconds to wait before giving up on waiting for %s to be ready.", what) + 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) } diff --git a/pkg/kn/commands/wait_flags_test.go b/pkg/kn/commands/wait_flags_test.go index a98f45a94d..9c711d2c78 100644 --- a/pkg/kn/commands/wait_flags_test.go +++ b/pkg/kn/commands/wait_flags_test.go @@ -42,7 +42,7 @@ func TestAddWaitForReadyDeprecatedFlags(t *testing.T) { flags := &WaitFlags{} cmd := cobra.Command{} - flags.AddConditionWaitFlags(&cmd, 60, "Create", "service") + flags.AddConditionWaitFlags(&cmd, 60, "Create", "service", "ready") err := cmd.ParseFlags(tc.args) if err != nil && !tc.isParseErrorExpected { @@ -76,7 +76,7 @@ func TestAddWaitForReadyFlags(t *testing.T) { flags := &WaitFlags{} cmd := cobra.Command{} - flags.AddConditionWaitFlags(&cmd, 60, "Create", "service") + flags.AddConditionWaitFlags(&cmd, 60, "Create", "service", "ready") err := cmd.ParseFlags(tc.args) if err != nil && !tc.isParseErrorExpected { @@ -101,7 +101,7 @@ func TestAddWaitUsageMessage(t *testing.T) { flags := &WaitFlags{} cmd := cobra.Command{} - flags.AddConditionWaitFlags(&cmd, 60, "bla", "blub") + flags.AddConditionWaitFlags(&cmd, 60, "bla", "blub", "deleted") if !strings.Contains(cmd.UsageString(), "blub") { t.Error("no type returned in usage") } @@ -111,4 +111,7 @@ func TestAddWaitUsageMessage(t *testing.T) { if !strings.Contains(cmd.UsageString(), "60") { t.Error("default timeout not contained") } + if !strings.Contains(cmd.UsageString(), "deleted") { + t.Error("wrong until message") + } } From a49748c4bfa499049ff5227e13b0c5629719164b Mon Sep 17 00:00:00 2001 From: David Simansky Date: Mon, 24 Feb 2020 10:09:37 +0100 Subject: [PATCH 08/10] feat(wait): set foreground propagationPolicy in sync service delete --- pkg/serving/v1/client.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index 6dc0e4dff7..1a6c1defea 100644 --- a/pkg/serving/v1/client.go +++ b/pkg/serving/v1/client.go @@ -257,7 +257,7 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU // For `timeout == 0` delete is performed async without any wait. func (cl *knServingClient) DeleteService(serviceName string, timeout time.Duration) error { if timeout == 0 { - return cl.deleteService(serviceName) + return cl.deleteService(serviceName, v1.DeletePropagationBackground) } waitC := make(chan error) go func() { @@ -265,17 +265,17 @@ func (cl *knServingClient) DeleteService(serviceName string, timeout time.Durati err, _ := waitForEvent.Wait(serviceName, timeout, wait.NoopMessageCallback()) waitC <- err }() - err := cl.deleteService(serviceName) + err := cl.deleteService(serviceName, v1.DeletePropagationForeground) if err != nil { return err } return <-waitC } -func (cl *knServingClient) deleteService(serviceName string) error { +func (cl *knServingClient) deleteService(serviceName string, propagationPolicy v1.DeletionPropagation) error { err := cl.client.Services(cl.namespace).Delete( serviceName, - &v1.DeleteOptions{}, + &v1.DeleteOptions{PropagationPolicy: &propagationPolicy}, ) if err != nil { return clienterrors.GetError(err) From 9bb60ac25e2f2a30a637a233d851c19d73729628 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Mon, 24 Feb 2020 14:37:27 +0100 Subject: [PATCH 09/10] feat(wait): add sync revision delete operation --- docs/cmd/kn_revision_delete.md | 3 +++ pkg/kn/commands/revision/delete.go | 10 ++++++++- pkg/kn/commands/revision/delete_test.go | 24 ++++++++++++++++++++++ pkg/serving/v1/client.go | 27 +++++++++++++++++++++++-- pkg/serving/v1/client_mock.go | 8 ++++---- pkg/serving/v1/client_mock_test.go | 4 ++-- 6 files changed, 67 insertions(+), 9 deletions(-) diff --git a/docs/cmd/kn_revision_delete.md b/docs/cmd/kn_revision_delete.md index 1a46f65825..0905f7364a 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 3a856004d1..109448e8a5 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/kn/commands/revision/delete_test.go b/pkg/kn/commands/revision/delete_test.go index 53ac70ddc6..bc833e96ee 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,11 @@ 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.Modified, &servingv1.Revision{ObjectMeta: metav1.ObjectMeta{Name: name}}}, + {watch.Deleted, &servingv1.Revision{ObjectMeta: metav1.ObjectMeta{Name: name}}}, + } +} diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index 1a6c1defea..003199be28 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 9381ed34ca..c60583e7ab 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]) } diff --git a/pkg/serving/v1/client_mock_test.go b/pkg/serving/v1/client_mock_test.go index 4bf80e5240..ec85dd76c7 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") From 7ff0fe50004a9f537fbe0a2b02111df97d74eef7 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 25 Feb 2020 09:56:06 +0100 Subject: [PATCH 10/10] Revert "feat(wait): add sync revision delete operation" This reverts commit 9bb60ac25e2f2a30a637a233d851c19d73729628. --- docs/cmd/kn_revision_delete.md | 3 --- pkg/kn/commands/revision/delete.go | 10 +-------- pkg/kn/commands/revision/delete_test.go | 24 ---------------------- pkg/serving/v1/client.go | 27 ++----------------------- pkg/serving/v1/client_mock.go | 8 ++++---- pkg/serving/v1/client_mock_test.go | 4 ++-- 6 files changed, 9 insertions(+), 67 deletions(-) diff --git a/docs/cmd/kn_revision_delete.md b/docs/cmd/kn_revision_delete.md index 0905f7364a..1a46f65825 100644 --- a/docs/cmd/kn_revision_delete.md +++ b/docs/cmd/kn_revision_delete.md @@ -21,11 +21,8 @@ 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 109448e8a5..3a856004d1 100644 --- a/pkg/kn/commands/revision/delete.go +++ b/pkg/kn/commands/revision/delete.go @@ -17,7 +17,6 @@ package revision import ( "errors" "fmt" - "time" "github.com/spf13/cobra" @@ -26,8 +25,6 @@ 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.", @@ -48,11 +45,7 @@ func NewRevisionDeleteCommand(p *commands.KnParams) *cobra.Command { } for _, name := range args { - timeout := time.Duration(0) - if !waitFlags.NoWait { - timeout = time.Duration(waitFlags.TimeoutInSeconds) * time.Second - } - err = client.DeleteRevision(name, timeout) + err = client.DeleteRevision(name) if err != nil { fmt.Fprintf(cmd.OutOrStdout(), "%s.\n", err) } else { @@ -63,6 +56,5 @@ 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/kn/commands/revision/delete_test.go b/pkg/kn/commands/revision/delete_test.go index bc833e96ee..53ac70ddc6 100644 --- a/pkg/kn/commands/revision/delete_test.go +++ b/pkg/kn/commands/revision/delete_test.go @@ -15,19 +15,14 @@ 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) { @@ -40,17 +35,6 @@ 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 { @@ -95,11 +79,3 @@ 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.Modified, &servingv1.Revision{ObjectMeta: metav1.ObjectMeta{Name: name}}}, - {watch.Deleted, &servingv1.Revision{ObjectMeta: metav1.ObjectMeta{Name: name}}}, - } -} diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go index 003199be28..1a6c1defea 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, timeout time.Duration) error + DeleteRevision(name string) error // Get a route by its unique name GetRoute(name string) (*servingv1.Route, error) @@ -177,11 +177,6 @@ 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()) @@ -375,25 +370,7 @@ func getBaseRevision(cl KnServingClient, service *servingv1.Service) (*servingv1 } // Delete a revision by name -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 { +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 c60583e7ab..9381ed34ca 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, timeout interface{}, err error) { - sr.r.Add("DeleteRevision", []interface{}{name, timeout}, []interface{}{err}) +func (sr *ServingRecorder) DeleteRevision(name interface{}, err error) { + sr.r.Add("DeleteRevision", []interface{}{name}, []interface{}{err}) } -func (c *MockKnServingClient) DeleteRevision(name string, timeout time.Duration) error { - call := c.recorder.r.VerifyCall("DeleteRevision", name, timeout) +func (c *MockKnServingClient) DeleteRevision(name string) error { + call := c.recorder.r.VerifyCall("DeleteRevision", name) return mock.ErrorOrNil(call.Result[0]) } diff --git a/pkg/serving/v1/client_mock_test.go b/pkg/serving/v1/client_mock_test.go index ec85dd76c7..4bf80e5240 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", time.Duration(10)*time.Second, nil) + recorder.DeleteRevision("hello", 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", time.Duration(10)*time.Second) + client.DeleteRevision("hello") client.GetRoute("hello") client.ListRoutes(WithName("blub")) client.GetConfiguration("hello")