kubectl wait must handle errors returned by watch #69389

smarterclayton · 2018-10-03T18:19:46Z

Watch can return type "ERROR" and a metav1.Status object. We need to
handle that during wait.

@deads2k i think we should probably have the dynamic watch handle error below
us and return a metav1.Status. I need to investigate more.

The kubectl wait command must handle when a watch returns an error vs closing by printing out the error and retrying the watch.

smarterclayton · 2018-10-03T20:45:39Z

pkg/kubectl/cmd/wait/wait.go

+				if err := apierrors.FromObject(watchEvent.Object); err != nil {
+					return gottenObj, false, err
+				}
+				// TODO: this is terrible because I should be getting metav1.Status from the watch scheme - we could


I think I'm going to move this into apierrors.FromObject, as much as it pains me, because that's at least guaranteed of having a chance to catch it. I don't like hardcoding apiVersion, but that's the api...

jennybuckley · 2018-10-04T20:55:02Z

/cc @wenjiaswe @jpbetz

k8s-ci-robot · 2018-10-04T20:55:03Z

@jennybuckley: GitHub didn't allow me to request PR reviews from the following users: wenjiaswe.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @wenjiaswe @jpbetz

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.

k8s-ci-robot · 2018-10-05T20:30:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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:

~~pkg/kubectl/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apiserver/OWNERS~~ [smarterclayton]

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

smarterclayton · 2018-10-10T19:22:41Z

/retest

soltysh

Two naming nits, overall this lgtm. I'll let @deads2k have final saying, it's his baby still ;)

soltysh · 2018-10-11T10:14:15Z

pkg/kubectl/cmd/wait/wait.go

@@ -300,7 +305,12 @@ func IsDeleted(info *resource.Info, o *WaitOptions) (runtime.Object, bool, error
 }

 func isDeleted(event watch.Event) (bool, error) {
-	return event.Type == watch.Deleted, nil
+	switch event.Type {


Can you rename the function to isDeletedOrError currently the name and the code don't express the same thing.

This only returns true on deleted.

soltysh · 2018-10-11T10:15:56Z

pkg/kubectl/cmd/wait/wait.go

@@ -389,6 +404,9 @@ func (w ConditionalWait) checkCondition(obj *unstructured.Unstructured) (bool, e
 }

 func (w ConditionalWait) isConditionMet(event watch.Event) (bool, error) {
+	if event.Type == watch.Error {


deads2k · 2018-10-15T18:46:32Z

pkg/kubectl/cmd/wait/wait.go

@@ -389,6 +404,9 @@ func (w ConditionalWait) checkCondition(obj *unstructured.Unstructured) (bool, e
 }

 func (w ConditionalWait) isConditionMet(event watch.Event) (bool, error) {
+	if event.Type == watch.Error {
+		return true, nil


This could be

if event.Type == watch.Error { err = apierrors.FromObject(event.Object) fmt.Fprintf(o.ErrOut, "error: An error occurred while waiting for the condition to be satisfied: %v", err) // this will chain back out, result in another get and an return false back up the chain return false, nil }

And the condition check would appear more normal.

Same for above

conditional wait doesn't have ErrOut, I was considering whether to do this or not. I'm fine with it

deads2k · 2018-10-15T18:46:49Z

I'd like a minor tweak to the factorization and then lgtm.

smarterclayton · 2018-10-16T16:05:43Z

/retest

Watch can return type "ERROR" and a metav1.Status object. We need to handle that during wait, and make it easy to handle the status object.

soltysh · 2018-10-17T11:59:11Z

pkg/kubectl/cmd/wait/wait.go

+// IsDeleted returns true if the object is deleted. It prints any errors it encounters.
+func (w Wait) IsDeleted(event watch.Event) (bool, error) {
+	switch event.Type {
+	case watch.Error:


David also meant this condition to look similar to the other one in isConditionMet.

How is this different than the other if not?

The other one calls checkCondition.

smarterclayton · 2018-10-25T00:01:28Z

@soltysh not sure what you're asking on the last comment - hopefully that answers the obvious question (but not sure that there isn't something deeper you're asking)

soltysh

Sorry, I was looking at the switch and didn't notice this is working as expected.
/lgtm

k8s-ci-robot requested review from deads2k and soltysh October 3, 2018 18:19

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 3, 2018

smarterclayton force-pushed the wait_error branch from 022bede to 14a2497 Compare October 3, 2018 19:34

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2018

smarterclayton commented Oct 3, 2018

View reviewed changes

k8s-ci-robot requested a review from jpbetz October 4, 2018 20:55

smarterclayton force-pushed the wait_error branch from 14a2497 to 638100a Compare October 5, 2018 20:29

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 8, 2018

smarterclayton force-pushed the wait_error branch from 638100a to e2ec695 Compare October 8, 2018 14:46

smarterclayton changed the title ~~WIP - Sketch fix for watch errors~~ kubectl wait must handle errors returned by watch Oct 8, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2018

smarterclayton added the kind/bug Categorizes issue or PR as related to a bug. label Oct 8, 2018

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Oct 8, 2018

smarterclayton force-pushed the wait_error branch from e2ec695 to 70d51b3 Compare October 8, 2018 16:16

soltysh approved these changes Oct 11, 2018

View reviewed changes

deads2k reviewed Oct 15, 2018

View reviewed changes

smarterclayton force-pushed the wait_error branch from 70d51b3 to 50ee2e1 Compare October 16, 2018 15:37

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2018

smarterclayton force-pushed the wait_error branch from 50ee2e1 to 2aed55c Compare October 16, 2018 15:37

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2018

Dynamic client watchers should be able to handle ERROR gracefully

5a8afa0

Watch can return type "ERROR" and a metav1.Status object. We need to handle that during wait, and make it easy to handle the status object.

smarterclayton force-pushed the wait_error branch from 2aed55c to 5a8afa0 Compare October 16, 2018 16:55

soltysh requested changes Oct 17, 2018

View reviewed changes

soltysh approved these changes Oct 25, 2018

View reviewed changes

k8s-ci-robot assigned soltysh Oct 25, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2018

k8s-ci-robot merged commit b6fd5d9 into kubernetes:master Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl wait must handle errors returned by watch #69389

kubectl wait must handle errors returned by watch #69389

smarterclayton commented Oct 3, 2018 •

edited

smarterclayton Oct 3, 2018

jennybuckley commented Oct 4, 2018

k8s-ci-robot commented Oct 4, 2018

k8s-ci-robot commented Oct 5, 2018

smarterclayton commented Oct 10, 2018

soltysh left a comment

soltysh Oct 11, 2018

smarterclayton Oct 24, 2018

soltysh Oct 11, 2018

deads2k Oct 15, 2018

smarterclayton Oct 15, 2018

deads2k commented Oct 15, 2018

smarterclayton commented Oct 16, 2018

soltysh Oct 17, 2018

smarterclayton Oct 17, 2018

soltysh Oct 18, 2018

smarterclayton Oct 24, 2018

smarterclayton commented Oct 25, 2018

soltysh left a comment

kubectl wait must handle errors returned by watch #69389

kubectl wait must handle errors returned by watch #69389

Conversation

smarterclayton commented Oct 3, 2018 • edited

Choose a reason for hiding this comment

jennybuckley commented Oct 4, 2018

k8s-ci-robot commented Oct 4, 2018

k8s-ci-robot commented Oct 5, 2018

smarterclayton commented Oct 10, 2018

soltysh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 15, 2018

smarterclayton commented Oct 16, 2018

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

smarterclayton commented Oct 25, 2018

soltysh left a comment

Choose a reason for hiding this comment

smarterclayton commented Oct 3, 2018 •

edited