feat(wait): add wait for delete operation #682

dsimansk · 2020-02-19T13:51:23Z

Fixes #671

Proposed Changes

Add waitForEvent logic to wait package
Refactor client.DeleteService()
- timeout == 0 asyn delete without wait
- otherwise wait a given time for delete op to complete
Change service delete to wait by default
Add --no-wait flag to delete op

Generated doc for --no-wait flag seems a bit off, as it was meant for ready use-case primarly. I'll take a closer after actual waiting logic in reviewed.

knative-prow-robot

@dsimansk: 5 warnings.

In response to this:

Fixes #671

Proposed Changes

Add client.WaitForEvent() to handle waiting for event types

Change service delete to wait for deletion by default

Add --no-wait flag to delete op

Generated doc for --no-wait flag seems a bit off, as it was meant for ready use-case primarly. I'll take a closer after actual waiting logic in reviewed.

/wip

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2020-02-19T13:51:33Z

pkg/wait/wait_for_ready.go

+	kind       string
+}
+
+type EventDone func(ev *watch.Event) bool


Golint comments: exported type EventDone should have comment or be unexported. More info.

knative-prow-robot · 2020-02-19T13:51:33Z

pkg/wait/wait_for_ready.go

 	return &waitForReadyConfig{
 		kind:                kind,
 		watchMaker:          watchMaker,
 		conditionsExtractor: extractor,
 	}
 }

+func NewWaitForEvent(kind string, watchMaker WatchMaker, eventDone EventDone) Wait {


Golint comments: exported function NewWaitForEvent should have comment or be unexported. More info.

knative-prow-robot · 2020-02-19T13:51:33Z

pkg/wait/wait_for_ready.go

@@ -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) {


Golint arg-order: error should be the last type when returning multiple items.

knative-prow-robot · 2020-02-19T13:51:33Z

pkg/serving/v1/client_mock.go

+	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 {


Golint comments: exported method MockKnServingClient.WaitForEvent should have comment or be unexported. More info.

knative-prow-robot · 2020-02-19T13:51:33Z

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


Golint comments: comment on exported method ServingRecorder.WaitForEvent should be of the form "WaitForEvent ...". More info.

dsimansk · 2020-02-19T13:52:52Z

/assign @rhuss

dsimansk · 2020-02-19T13:53:29Z

/wip

rhuss · 2020-02-19T15:49:34Z

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.


don't add that, not deeded to add a deprecated option.

It's still part of AddConditionWaitFlags() and I wish I could create overloaded method...:) But of course, I agress it shouldn't be created with deprecated flag.

yep, I've seen. its ok.

dsimansk · 2020-02-21T13:41:36Z

/lint

knative-prow-robot

@dsimansk: 2 unresolved warnings and 3 new warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2020-02-21T13:41:46Z

pkg/serving/v1/client_mock.go

 }

-func (c *MockKnServingClient) DeleteService(name string) error {
-	call := c.recorder.r.VerifyCall("DeleteService", name)
+func (c *MockKnServingClient) DeleteService(name string, timeout time.Duration) error {


Golint comments: exported method MockKnServingClient.DeleteService should have comment or be unexported. More info.

knative-prow-robot · 2020-02-21T13:41:46Z

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)


Golint naming: var fakeWatchApi should be fakeWatchAPI. More info.

knative-prow-robot · 2020-02-21T13:41:47Z

pkg/wait/wait_for_ready.go

+	kind       string
+}
+
+// Done marker to stop actual waiting on given event state


Golint comments: comment on exported type EventDone should be of the form "EventDone ..." (with optional leading article). More info.

rhuss

Looks good, with some comment :)

pkg/serving/v1/client.go

rhuss · 2020-02-21T14:13:44Z

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.


yep, I've seen. its ok.

pkg/wait/wait_for_ready.go

dsimansk · 2020-02-21T16:23:12Z

@rhuss pls check if the following change is acceptable b97e2ca as part of this PR. :)

dsimansk · 2020-02-21T18:18:25Z

/retest

rhuss · 2020-02-21T18:20:43Z

@dsimansk staring at this latest integration error at https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/682/pull-knative-client-integration-tests-latest-release/1230890397631254528 I think it's not only good enough to wait for the service to be deleted, but also on all revisions attached to this service. As you can see, in the log, the service was deleted sync (per this PR), but the next create still references a revision from this old service in its traffic split. This indicates to me, that although the service has been removed, that it's not the case for its owned revision (therefore somehow the subsequent service create could still select this old revision which is just about to delete (and that doesn't how up anymore in the dumps which happens some time later).

rhuss · 2020-02-21T18:21:10Z

Do you think its hard to implement to wait for the service and all objects which have this service as ownerReference ?

rhuss · 2020-02-21T18:22:18Z

btw, yesterday I stumbled upon: https://github.com/k14s/kapp/blob/master/docs/apply-waiting.md which is more of a generic "wait" on resources. Not sure about the implementation, but we could have a look, too.

navidshaikh · 2020-02-24T07:49:49Z

Nice observation on the logs! 👍

A service is created with BYO revision name and deleted.
Another service with same "name" in the same namespace is created with BYO revision name, why client should generate the same revision name (as generated in earlier service create) ?

dsimansk · 2020-02-24T11:16:48Z

/retest

navidshaikh · 2020-02-24T12:48:05Z

/retest

dsimansk · 2020-02-24T15:24:09Z

/retest

dsimansk · 2020-02-25T08:01:28Z

/retest

This reverts commit 9bb60ac.

knative-metrics-robot · 2020-02-25T09:00:21Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/delete.go	82.4%	86.4%	4.0
pkg/serving/v1/client.go	73.3%	74.7%	1.4
pkg/wait/wait_for_ready.go	74.3%	77.6%	3.4

dsimansk · 2020-02-25T09:33:54Z

@navidshaikh I've reverted sync revision delete commit 9bb60ac due to several side-effects that popped up in E2E tests.
I'd say this PR is good for a review in current state and I'll open a follow-up with sync revision delete, once it's a bit more stable.

navidshaikh

/lgtm
/approve

knative-prow-robot · 2020-02-25T10:38:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

In it's current state it now takes me about 25 seconds for the `kn delete` to complete. Before knative#682 it used to be almost immediate. This is because we now pass in the `DeletePropagationBackground` flag. I believe this is a mistake, not only because of the 20+ seconds of additional time to delete things, but IMO the CLI should talk to the server in the same way regardless of the --wait flag. That flag should just be a CLI thing to indicate if the user wants the CLI to wait for the server to complete but not HOW the server should do the delete. Signed-off-by: Doug Davis <[email protected]>

* Remove the delete propagation flag In it's current state it now takes me about 25 seconds for the `kn delete` to complete. Before #682 it used to be almost immediate. This is because we now pass in the `DeletePropagationBackground` flag. I believe this is a mistake, not only because of the 20+ seconds of additional time to delete things, but IMO the CLI should talk to the server in the same way regardless of the --wait flag. That flag should just be a CLI thing to indicate if the user wants the CLI to wait for the server to complete but not HOW the server should do the delete. Signed-off-by: Doug Davis <[email protected]> * try just tweaking the --no-wait flag Signed-off-by: Doug Davis <[email protected]>

* Remove the delete propagation flag In it's current state it now takes me about 25 seconds for the `kn delete` to complete. Before knative#682 it used to be almost immediate. This is because we now pass in the `DeletePropagationBackground` flag. I believe this is a mistake, not only because of the 20+ seconds of additional time to delete things, but IMO the CLI should talk to the server in the same way regardless of the --wait flag. That flag should just be a CLI thing to indicate if the user wants the CLI to wait for the server to complete but not HOW the server should do the delete. Signed-off-by: Doug Davis <[email protected]> * try just tweaking the --no-wait flag Signed-off-by: Doug Davis <[email protected]>

* (refactor) address the e2e extract / refactor of issue #763 (#765) * (refactor) address the e2e extract / refactor of issue #763 * various updates to address reviewers feedback * renamed lib/test/integration to lib/test and package to test Signed-off-by: Roland Huß <[email protected]> # Conflicts: # CHANGELOG.adoc # test/e2e/service_export_import_apply_test.go # test/e2e/trigger_test.go * fix(plugin): Fix plugin lookup with file ext on Windows (#774) * fix(plugin): Fix plugin lookup with file ext on Windows * chore: Update changelog * fix: Reflect review feedback * fix: Reflect review feedback and add future todo Signed-off-by: Roland Huß <[email protected]> # Conflicts: # CHANGELOG.adoc * fix(issue #762): correct error message when updating service (#778) * fix(issue #762): correct error message when updating service * correct message when updating service and passing many names * fix issue with TestServiceUpdateWithMultipleImages running create vs update * * added TestServiceDescribeWithMultipleNames * added TestServiceCreateWithMultipleNames * fix error message for service delete since many names can be passed * Use vendored deps while running e2e locally (#783) Also set GO111MODULE=on unconditionally * Update sink binding create usage string (#785) * Add "--target-utilization" to manage "autoscaling.knative.dev/targetUtilizationPercentage" annotation (#788) * Support setting "autoscaling.knative.dev/targetUtilizationPercentage" annotation. Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/service_options_test.go * Remove the delete propagation flag (#770) * Remove the delete propagation flag In it's current state it now takes me about 25 seconds for the `kn delete` to complete. Before #682 it used to be almost immediate. This is because we now pass in the `DeletePropagationBackground` flag. I believe this is a mistake, not only because of the 20+ seconds of additional time to delete things, but IMO the CLI should talk to the server in the same way regardless of the --wait flag. That flag should just be a CLI thing to indicate if the user wants the CLI to wait for the server to complete but not HOW the server should do the delete. Signed-off-by: Doug Davis <[email protected]> * try just tweaking the --no-wait flag Signed-off-by: Doug Davis <[email protected]> * Fix error when output is set to name (#775) * fix error when output is set to name * add e2e test * change to flags/listprint.go Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/basic_workflow_test.go * Show all revisions when run `service describe -v` (#790) * The `kn service describe -v` command shows repetitive revisions, because the revision would be covered by next one. * Fix resource listing with -oname flag (#799) * Fix resource listing with -oname flag * add e2e tests Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/ping_test.go # test/e2e/revision_test.go # test/e2e/route_test.go # test/e2e/source_apiserver_test.go # test/e2e/source_binding_test.go # test/e2e/trigger_test.go * Make wait, no-wait and async flags per bool var CLI convention (#802) * Make wait, no-wait and async flags per bool var CLI convention Fixes #800 - Deprecated bool vars can be supported for CLI convention - Bind --async flag value to --no-wait - Only one flag among [wait, no-wait, async] can be provided, else raise an error * Simplify conditionals * Add unit tests for deprecated flag async * Fix a typo * e2e: Foreground delete for revisions and services in e2e (#794) * e2e: Foreground delete for revisions and services in e2e to avoid any race conditions and flakes * Use --wait instead of --no-wait=false Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/basic_workflow_test.go # test/e2e/revision_test.go * e2e: Run tekton e2e against pipeline v0.11.1 (#803) * Use buildah task from master branch and paramterize FORMAT * Configure pipeline v0.11.1 * DNM: Run tekton e2e in this PR * Revert "DNM: Run tekton e2e in this PR" This reverts commit 903f5be. * Update CHANGELOG for v0.13.2 (#804) * Pin serving to v0.13.2 and update version command (#797) * Pin serving v0.13.2 dep to v0.13.2 * Update version command now points to serving v0.13.2 and eventing v0.13.6 * Copy go.sum as generated in CI Signed-off-by: Roland Huß <[email protected]> # Conflicts: # go.mod # go.sum # vendor/modules.txt * add missing vendored files * fixed error reporting for traffics tests * Updated test * fix formatting * e2e for service export (#739) * e2e for service export * e2e for service export * e2e for service export * e2e for service export * e2e for service export Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/service_export_import_apply_test.go Co-authored-by: dr.max <[email protected]> Co-authored-by: David Simansky <[email protected]> Co-authored-by: Navid Shaikh <[email protected]> Co-authored-by: Lv Jiawei <[email protected]> Co-authored-by: Doug Davis <[email protected]> Co-authored-by: Ying Chun Guo <[email protected]> Co-authored-by: Murugappan Chetty <[email protected]>

feat(wait): add wait for delete operation

80d0001

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 19, 2020

knative-prow-robot reviewed Feb 19, 2020

View reviewed changes

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 19, 2020

knative-prow-robot requested review from mattmoor and maximilien February 19, 2020 13:51

knative-prow-robot assigned rhuss Feb 19, 2020

dsimansk changed the title ~~feat(wait): add wait for delete operation~~ WIP: feat(wait): add wait for delete operation Feb 19, 2020

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2020

rhuss reviewed Feb 19, 2020

View reviewed changes

dsimansk added 2 commits February 21, 2020 14:04

feat(wait): refactor initial impl

adf36de

fix(lint): add docs to new functions

01d2b0f

knative-prow-robot reviewed Feb 21, 2020

View reviewed changes

rhuss requested changes Feb 21, 2020

View reviewed changes

dsimansk added 3 commits February 21, 2020 16:00

fix: reflect code review comments

9221ab8

feat(wait): add changelog entry

44e33d7

fix: changelog entry to render correctly

e0cc636

dsimansk changed the title ~~WIP: feat(wait): add wait for delete operation~~ feat(wait): add wait for delete operation Feb 21, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2020

dsimansk mentioned this pull request Feb 21, 2020

Add --wait and --no-wait for all delete operation #671

Closed

fix: usage message of wait flag

b97e2ca

feat(wait): set foreground propagationPolicy in sync service delete

a49748c

feat(wait): add sync revision delete operation

9bb60ac

Revert "feat(wait): add sync revision delete operation"

7ff0fe5

This reverts commit 9bb60ac.

navidshaikh approved these changes Feb 25, 2020

View reviewed changes

knative-prow-robot assigned navidshaikh Feb 25, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2020

knative-prow-robot merged commit 345653b into knative:master Feb 25, 2020

duglin mentioned this pull request Apr 1, 2020

Dont wait for delete operations to be completed by default #770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wait): add wait for delete operation #682

feat(wait): add wait for delete operation #682

dsimansk commented Feb 19, 2020 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot Feb 19, 2020

knative-prow-robot Feb 19, 2020

knative-prow-robot Feb 19, 2020

knative-prow-robot Feb 19, 2020

knative-prow-robot Feb 19, 2020

dsimansk commented Feb 19, 2020

dsimansk commented Feb 19, 2020

rhuss Feb 19, 2020

dsimansk Feb 21, 2020

rhuss Feb 21, 2020

dsimansk commented Feb 21, 2020

knative-prow-robot left a comment

knative-prow-robot Feb 21, 2020

knative-prow-robot Feb 21, 2020

knative-prow-robot Feb 21, 2020

rhuss left a comment

rhuss Feb 21, 2020

dsimansk commented Feb 21, 2020

dsimansk commented Feb 21, 2020

rhuss commented Feb 21, 2020 •

edited

Loading

rhuss commented Feb 21, 2020 •

edited

Loading

rhuss commented Feb 21, 2020

navidshaikh commented Feb 24, 2020

dsimansk commented Feb 24, 2020

navidshaikh commented Feb 24, 2020

dsimansk commented Feb 24, 2020

dsimansk commented Feb 25, 2020

knative-metrics-robot commented Feb 25, 2020

dsimansk commented Feb 25, 2020 •

edited

Loading

navidshaikh left a comment

knative-prow-robot commented Feb 25, 2020

feat(wait): add wait for delete operation #682

feat(wait): add wait for delete operation #682

Conversation

dsimansk commented Feb 19, 2020 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Feb 19, 2020

dsimansk commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Feb 21, 2020

knative-prow-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Feb 21, 2020

dsimansk commented Feb 21, 2020

rhuss commented Feb 21, 2020 • edited Loading

rhuss commented Feb 21, 2020 • edited Loading

rhuss commented Feb 21, 2020

navidshaikh commented Feb 24, 2020

dsimansk commented Feb 24, 2020

navidshaikh commented Feb 24, 2020

dsimansk commented Feb 24, 2020

dsimansk commented Feb 25, 2020

knative-metrics-robot commented Feb 25, 2020

dsimansk commented Feb 25, 2020 • edited Loading

navidshaikh left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 25, 2020

dsimansk commented Feb 19, 2020 •

edited

Loading

rhuss commented Feb 21, 2020 •

edited

Loading

rhuss commented Feb 21, 2020 •

edited

Loading

dsimansk commented Feb 25, 2020 •

edited

Loading