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

cacher: don't accept requests if stopped #108414

Merged
merged 1 commit into from May 11, 2022
Merged

cacher: don't accept requests if stopped #108414

merged 1 commit into from May 11, 2022

Conversation

aojea
Copy link
Member

@aojea aojea commented Mar 1, 2022

The cacher blocks until it is ready to answer request, however, the
ready variable doesn't differentiate if the cacher was stopped.

Before blocking waiting for the cacher to be ready, check if it
was stopped and return an error notifying the client.

/kind bug

kube-apiserver: Get, GetList and Watch requests that should be served by the apiserver cacher during shutdown will be rejected to avoid a deadlock situation leaving requests hanging.

@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/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 1, 2022
@aojea
Copy link
Member Author

aojea commented Mar 1, 2022

/assign @wojtek-t @sttts @p0lyn0mial @tkashem

@aojea aojea mentioned this pull request Mar 1, 2022
Closed
@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 1, 2022
@aojea
Copy link
Member Author

aojea commented Mar 1, 2022

I set the check before the ready() condition, but I don't know if it should short circuit immediately , not familiar with this code 🤷

@aojea
Copy link
Member Author

aojea commented Mar 1, 2022

this actually reduces the number of goroutines leaked by

s := apitesting.StartTestServerOrDie(t, nil, nil, etcdConfig)
	s.TearDownFn()

from ~200 to 60

grep expected test.log
teardown_test.go:63: expected 4 or fewer, got 59

#108060 (comment)

@jpbetz
Copy link
Contributor

jpbetz commented Mar 1, 2022

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 1, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, wojtek-t

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

@wojtek-t
Copy link
Member

This LGTM modulo the unresolved discussion about error to return.

@sttts @liggitt - can you chime in if this looks ok to you?

@aojea aojea force-pushed the cacher_context branch 4 times, most recently from e5e713a to 0b06078 Compare May 2, 2022 09:15
@wojtek-t
Copy link
Member

wojtek-t commented May 2, 2022

Still LGTM modulo the discussion about error code.
@liggitt - does that 503 look ok to you?

@aojea
Copy link
Member Author

aojea commented May 2, 2022

/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 May 2, 2022
@aojea
apiserver cacher: don't accept requests if stopped
2cb3a56 
The cacher blocks requests until it is ready, however, the
ready variable doesn't differentiate if the cacher was stopped.

The cacher is using a condition variable based on sync.Cond to
handle the readiness, however, this was not taking into account
if it was not ready because it was waiting to be ready or it was
stopped.

Add a new condition to the condition variable to handle the
stop condition, and returning an error to signal the goroutines
that they should stop waiting and bail out.
@wojtek-t
Copy link
Member

Still LGTM - but I guess @liggitt should take another look.

@liggitt
Copy link
Member

liggitt commented May 11, 2022

/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 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 999b1bb into kubernetes:master May 11, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone May 11, 2022
@wojtek-t wojtek-t mentioned this pull request Feb 24, 2023
Merged
This was referenced Mar 20, 2023
Closed
Closed
This was referenced Mar 21, 2023
Merged
Closed
k8s-ci-robot added a commit that referenced this pull request Mar 28, 2023
@k8s-ci-robot
Merge pull request #116822 from natherz97/automated-cherry-pick-of-#1…
9ac1d1a 
…08414-#116024-remotes-upstream-release-1.24

Automated cherry pick of #108414: apiserver cacher: don't accept requests if stopped
#116024: cacher allow context cancellation if not ready
wongma7 pushed a commit to wongma7/kubernetes that referenced this pull request Jul 24, 2023
@aojea @wongma7
--EKS-PATCH-- Prevent watch requests on resources with an uninitializ…
2a40185 
…ed watch-cache from indefinitely counting as an inflight request

Cherry-pick of upstream Kubernetes PR kubernetes#108414 and kubernetes#116024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@liggitt liggitt liggitt left review comments

@wojtek-t wojtek-t wojtek-t left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@sttts sttts

@liggitt liggitt

@tkashem tkashem

@wojtek-t wojtek-t

@p0lyn0mial p0lyn0mial

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@aojea @jpbetz @wojtek-t @liggitt @k8s-ci-robot @sttts @tkashem @p0lyn0mial