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

apiserver: forward panic in WithTimeout filter #68001

Merged
merged 1 commit into from
Aug 29, 2018
Merged

apiserver: forward panic in WithTimeout filter #68001

merged 1 commit into from
Aug 29, 2018
+36 −8

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Aug 29, 2018 •
edited
Loading 

Return apiserver panics as 500 errors instead terminating the apiserver process.

Without this PR a panic in a HTTP handler will not be caught in the Go routine started by the timeout filter. Uncaught panics terminate the process.

This is a strong condidate to be backported to 1.11, 1.10 and 1.9.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 29, 2018
@k8s-ci-robot k8s-ci-robot requested review from dims and jbeda August 29, 2018 11:47
@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 29, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 29, 2018
@sttts sttts added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. area/security and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 29, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 29, 2018
@sttts sttts added this to the v1.12 milestone Aug 29, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 29, 2018

/retest

deads2k
deads2k reviewed Aug 29, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go
tw := newTimeoutWriter(w)
go func() {
defer func() {
result <- recover()
Copy link
Contributor

Choose a reason for hiding this comment

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

so easy to start a gofunc, so easy to forget to recover panics in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now go through all go funcs in Kube and prepare for new grey hair.

@deads2k
Copy link
Contributor

deads2k commented Aug 29, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sttts

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

This was referenced Aug 29, 2018
Merged
Merged
Merged
@sttts
Copy link
Contributor Author

sttts commented Aug 29, 2018

/retest

@sttts
Copy link
Contributor Author

sttts commented Aug 29, 2018

/hold

Checking whether the test failures are real.

@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 Aug 29, 2018
@sttts
Copy link
Contributor Author

sttts commented Aug 29, 2018

Seeing the same flakes in #64149, totally unrelated PR.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 66577, 67948, 68001, 67982). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit ca8f267 into kubernetes:master Aug 29, 2018
@fedebongio
Copy link
Contributor

/assign @caesarxuchao

This was referenced Sep 3, 2018
Closed
Closed
Merged
Closed
Closed
k8s-github-robot pushed a commit that referenced this pull request Sep 3, 2018
Merge pull request #68010 from sttts/automated-cherry-pick-of-#68001-…
c93d90e 
…release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #68001: apiserver: forward panic in WithTimeout filter

Cherry pick of #68001 on release-1.9.

#68001: apiserver: forward panic in WithTimeout filter
This was referenced Sep 4, 2018
Merged
Merged
k8s-github-robot pushed a commit that referenced this pull request Sep 6, 2018
Merge pull request #68009 from sttts/automated-cherry-pick-of-#68001-…
cb1d872 
…release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #68001: apiserver: forward panic in WithTimeout filter

Cherry pick of #68001 on release-1.10.

#68001: apiserver: forward panic in WithTimeout filter
k8s-ci-robot added a commit that referenced this pull request Oct 4, 2018
@k8s-ci-robot
Merge pull request #68008 from sttts/automated-cherry-pick-of-#68001-…
40b39fc 
…release-1.11

Automated cherry pick of #68001: apiserver: forward panic in WithTimeout filter
@liggitt
Copy link
Member

liggitt commented Nov 14, 2018 •
edited
Loading

while stopping the apiserver crashes (\o/) this also loses stack information from the propagated panic (/o). Will open a follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@dims dims Awaiting requested review from dims

@jbeda jbeda Awaiting requested review from jbeda

Assignees

@mfojtik mfojtik

@caesarxuchao caesarxuchao

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/security cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.
10 participants
@sttts @deads2k @k8s-ci-robot @fejta-bot @k8s-github-robot @fedebongio @liggitt @lavalamp @mfojtik @caesarxuchao