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.

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

Terminate custom resource watches when storage is destroyed #78029

Merged
merged 2 commits into from May 18, 2019

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented May 17, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
When a watch cache is stopped, drop all active watchers.

For normal resources, this only happens on apiserver shutdown. For custom resources, this happens whenever a CRD spec is changed, which currently strands active watchers.

Which issue(s) this PR fixes:
Fixes #74105
Fixes #71138

Does this PR introduce a user-facing change?:

Active watches of custom resources now terminate properly if the CRD is modified.

/sig api-machinery
/cc @jpbetz @sttts

@k8s-ci-robot k8s-ci-robot requested review from jpbetz and sttts May 17, 2019 14:05
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 17, 2019
@liggitt
Copy link
Member Author

liggitt commented May 17, 2019

/priority critical-urgent

this results in hung watch clients with O(hour) timeouts when CRDs change

@k8s-ci-robot k8s-ci-robot added area/apiserver priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 17, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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 May 17, 2019
@liggitt liggitt changed the title Terminate CRD watches when storage is destroyed Terminate custom resource watches when storage is destroyed May 17, 2019
@liggitt liggitt added this to Required for GA, in progress in Custom Resource Definitions May 17, 2019
@sttts
Copy link
Contributor

sttts commented May 17, 2019

one nit.

Otherwise lgtm.

@sttts
Copy link
Contributor

sttts commented May 17, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2019
@smarterclayton
Copy link
Contributor

Impacted how far back?

@liggitt
Copy link
Member Author

liggitt commented May 17, 2019

Impacted how far back?

Watch cache at least back to 1.12 (I'd expect even further). Custom resources as far back as they used the watch cache (not sure how far that is)

@liggitt liggitt added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels May 17, 2019
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2019
@liggitt
Copy link
Member Author

liggitt commented May 17, 2019

fixed gofmt issue

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2019
@jpbetz
Copy link
Contributor

jpbetz commented May 17, 2019

Looks right. Thanks for finding and fixing.

/lgtm

@yliaog
Copy link
Contributor

yliaog commented May 17, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 0f8009b into kubernetes:master May 18, 2019
@liggitt liggitt moved this from Required for GA, in progress to Complete in Custom Resource Definitions May 20, 2019
@liggitt liggitt deleted the crd-watch branch May 20, 2019 13:41
k8s-ci-robot added a commit that referenced this pull request May 23, 2019
…9-upstream-release-1.12

Automated cherry pick of #78029: Terminate watchers when watch cache is destroyed
k8s-ci-robot added a commit that referenced this pull request May 31, 2019
…9-upstream-release-1.14

Automated cherry pick of #78029: Terminate watchers when watch cache is destroyed
k8s-ci-robot added a commit that referenced this pull request May 31, 2019
…9-upstream-release-1.13

Automated cherry pick of #78029: Terminate watchers when watch cache is destroyed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/custom-resources 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 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.
Development

Successfully merging this pull request may close these issues.

Watch not terminated on CRD deletion Changing CRD validation causes existing WATCH requests to silently hang
6 participants