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
Don't prematurely close reflectors in case of slow initialization in watch based manager #104604
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
/assign @chenyw1990 |
@@ -287,11 +291,14 @@ func (c *objectCache) Get(namespace, name string) (runtime.Object, error) { | |||
if !exists { | |||
return nil, fmt.Errorf("object %q/%q not registered", namespace, name) | |||
} | |||
// Record last access time independently if it succeeded or not. | |||
// This protects from premature reflector closure in case of slow initializations. |
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.
Nit: I wouldn't say that if the watches are initializing fast, there is no problem to fix: In any case (fast or slow initialization) there is a race between a startRecycleIdleWatch goroutine and all initializing reflectors, it's just more visible in case of slow initialization.
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.
removed the last part of the sentence
/lgtm [hold in case you want to wait for review from sig-node, please unhold if you don't] |
/lgtm |
…watch based manager
2ffdce8
to
515106b
Compare
@mborsz @chenyw1990 - would you mind reapplying lgtm (I fixed the gofmt issue and fixed the comment as pointed out by Maciek) |
/lgtm |
/retest /hold cancel Let's have it merged to allow cherrypicking. |
@@ -405,3 +405,94 @@ func TestMaxIdleTimeStopsTheReflector(t *testing.T) { | |||
// Reflector should be running when the get function is called periodically. | |||
assert.True(t, reflectorRunning()) | |||
} | |||
|
|||
func TestReflectorNotStopedOnSlowInitialization(t *testing.T) { |
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.
Typo: Stopped
/retest |
1 similar comment
/retest |
Thanks @wojtek-t! Let me cherrypick this to 1.22 and 1.21 |
/triage accepted |
…4604-upstream-release-1.22 Automated cherry pick of #104604: Don't prematurely close reflectors in case of slow
…4604-upstream-release-1.21 Automated cherry pick of #104604: Don't prematurely close reflectors in case of slow
/kind bug
/priority important-soon
/sig node