-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Prevent garbage collector from attempting to sync with 0 resources #61201
Prevent garbage collector from attempting to sync with 0 resources #61201
Conversation
/sig api-machinery |
Looks awesome, and given the failure mode, looks like an equally awesome unit test shouldn't be too hard to add? :) |
@lavalamp I have a unit test running locally which fails before this commit and passes after, but it only works by modifying the code for the monitors so they always say they are synced. Once I figure out how to fake that part, or get them to actually sync, I will add it here. |
50cc566
to
532872b
Compare
Resubmitting with just the test, to show that it fails, after that I'll submit the fix to show that it passes. |
532872b
to
68e2a96
Compare
Proof of test failure before the fix is made: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/61201/pull-kubernetes-bazel-test/36187/ |
I am a little worried that the test will be racy due to the time.Sleep()s, but I really want this fix. Maybe you can work on a followup? /label status/approved-for-milestone /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jennybuckley, lavalamp 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 |
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process Pull Request Labels
|
Automatic merge from submit-queue (batch tested with PRs 61284, 61119, 61201). If you want to cherry-pick this change to another branch, please follow the instructions here. |
This is awesome, thanks @jennybuckley! Since this is introduced in 1.9, can we cherrypick this to 1.9? |
Yes, also the bug causing PR has been cherrypicked to 1.8 as well, so I think we should cherrypick this to 1.8 and 1.9 |
…#61201-upstream-release-1.9 Automated cherry pick of #61201: Prevent garbage collector from attempting to sync with 0 resources
What this PR does / why we need it:
As of #55259 we enabled garbagecollector.GetDeletableResources to return partial discovery results (including an empty set of discovery results).
This had the unintended consequence of allowing the garbage collector to enter a blocked state that can only be fixed by restarting.
From this comment:
This PR prevents that specific race condition from arising.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #60037
Release note: