-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Fix duplicate GC event handlers getting added if discovery flutters #117992
Conversation
/retest |
1 similar comment
/retest |
f750fe4
to
2a835a2
Compare
2a835a2
to
04be0b2
Compare
[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 |
/lgtm |
LGTM label has been added. Git tree hash: 9b1d6acc781717f842b0c3f9bdaef4957e704e2a
|
04be0b2
to
12a874d
Compare
/retest |
func GroupDiscoveryFailedErrorGroups(err error) (map[schema.GroupVersion]error, bool) { | ||
var groupDiscoveryError *ErrGroupDiscoveryFailed | ||
if err != nil && goerrors.As(err, &groupDiscoveryError) { | ||
return groupDiscoveryError.Groups, true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, there's a inconveniently named field.
/lgtm |
LGTM label has been added. Git tree hash: 11a419fe1f14ee7105d168dcf9e2ec3bb6bee412
|
/hold cancel |
this is fine to merge for 1.29, I'll likely propose a backport after that |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The GC controller regularly (every 30 seconds) rediscovers available API types. Discovery can partially fail, resulting in a partial view of resource types available.
The GC controller then registers new event handlers for newly visible resource types, and forgets about event handlers for no-longer-visible resource types. This forgetting is problematic in multiple ways:
This PR keeps existing already-synced event handlers in place as long as discovery is failing to resolve their group/version.
In the resource quota controller, we already preserved previously seen resources in discovery error cases for similar reasons, this PR narrows that retention to just group/versions with discovery resolution errors and adds test coverage for it.
Does this PR introduce a user-facing change?
/assign @munnerz @deads2k