Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Report a watch error instead of eating it when we can't decode #73937

Merged
merged 1 commit into from
Mar 28, 2019
Merged

Report a watch error instead of eating it when we can't decode #73937

merged 1 commit into from
Mar 28, 2019

Conversation

smarterclayton
Copy link
Contributor

Clients are required to handle watch events of type ERROR, so instead
of eating the decoding error we should pass it on to the client. Use
NewGenericServerError with isUnexpectedResponse to indicate that we
didn't get the bytes from the server we were expecting. For watch, the
415 error code is roughly correct and we will return an error to the
client that makes debugging a failure in either server watch or client
machinery much easier.

We do not alter the behavior when it appears the response is an EOF
or other disconnection.

/kind bug

This was discovered while writing an e2e test to verify #62175

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 11, 2019
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2019
@smarterclayton smarterclayton mentioned this pull request Feb 11, 2019
Closed
@smarterclayton
Copy link
Contributor Author

I may change this to an internal error instead of a 418, to be consistent with when we fail to renegotiate.

logicalhan
logicalhan approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

staging/src/k8s.io/apimachinery/pkg/watch/streamwatcher_test.go Outdated Show resolved Hide resolved
staging/src/k8s.io/apimachinery/pkg/watch/streamwatcher_test.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2019
@smarterclayton
Copy link
Contributor Author

/retest

@smarterclayton
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2019
@smarterclayton
Copy link
Contributor Author

/retest

1 similar comment
@smarterclayton
Copy link
Contributor Author

/retest

@smarterclayton
Copy link
Contributor Author

Updated to return a 500 and cleaned up test, PTAL

@smarterclayton smarterclayton removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2019
@yliaog
Copy link
Contributor

yliaog commented Feb 14, 2019

/cc

@logicalhan
Copy link
Member

/cc @mbohlool

@dims
Copy link
Member

dims commented Feb 18, 2019

/assign @deads2k

@smarterclayton smarterclayton added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 18, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 18, 2019
@smarterclayton smarterclayton mentioned this pull request Mar 7, 2019
Merged
4 tasks
@discordianfish
Copy link
Contributor

Oh great, looks good to me (fwiw).
I only think maybe it shouldn't be trying to be smart about the error (e.g checking, then guessing if it's EOF) but just report it and leave it to the caller to handle it as seen fit?

@smarterclayton
Report a watch error instead of eating it when we can't decode
89620d5 
Clients are required to handle watch events of type ERROR, so instead
of eating the decoding error we should pass it on to the client. Use
NewGenericServerError with isUnexpectedResponse to indicate that we
didn't get the bytes from the server we were expecting. For watch, the
415 error code is roughly correct and we will return an error to the
client that makes debugging a failure in either server watch or client
machinery much easier.

We do not alter the behavior when it appears the response is an EOF
or other disconnection.
@smarterclayton
Copy link
Contributor Author

I only think maybe it shouldn't be trying to be smart about the error (e.g checking, then guessing if it's EOF) but just report it and leave it to the caller to handle it as seen fit?

I don't want to regress that code path here - we're very careful about removing error handling in client-go so it would be better to deal with that in a separate issue.

@smarterclayton
Copy link
Contributor Author

/retest

@smarterclayton
Copy link
Contributor Author

@caesarxuchao PTAL

@caesarxuchao
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2019
@k8s-ci-robot k8s-ci-robot merged commit c28b3b1 into kubernetes:master Mar 28, 2019
@discordianfish discordianfish mentioned this pull request Apr 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims dims left review comments

@logicalhan logicalhan logicalhan approved these changes

@caesarxuchao caesarxuchao caesarxuchao left review comments

@asalkeld asalkeld Awaiting requested review from asalkeld

@brendandburns brendandburns Awaiting requested review from brendandburns

@yliaog yliaog Awaiting requested review from yliaog

@mbohlool mbohlool Awaiting requested review from mbohlool

Assignees

@logicalhan logicalhan

@caesarxuchao caesarxuchao

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@smarterclayton @k8s-ci-robot @yliaog @logicalhan @dims @discordianfish @caesarxuchao @deads2k