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 leaking goroutines in multiple integration tests #110264
Fix leaking goroutines in multiple integration tests #110264
Conversation
@wojtek-t: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
||
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() | ||
_, instanceConfig, closeFn := framework.RunAnAPIServer(controlPlaneConfig) | ||
client, err := clientset.NewForConfig(&restclient.Config{Host: instanceConfig.URL, QPS: -1}) |
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.
Do we need the -1 ?
ce4eaf3
to
4ea1e7d
Compare
/hold Need to fix job tests. |
3b952bc
to
90e02d7
Compare
90e02d7
to
f31d030
Compare
should be fixed now /hold cancel |
[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 |
/retest |
/lgtm |
/lgtm cancel the test failure is legit, it seems the event action is not created with these changes kubernetes/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller_test.go Line 175 in fa59370
|
it seems the test doesn't call Run on the controllers, maybe we should call it here kubernetes/pkg/controller/endpointslicemirroring/endpointslicemirroring_controller_test.go Line 181 in fa59370
and defer the shutdown |
2b0d166
to
afae9e9
Compare
@aojea - fixed the test PTAL |
afae9e9
to
c20f7cc
Compare
@aojea - can you please take another look? |
testCtx.informerFactory.Start(testCtx.scheduler.StopEverything) | ||
testCtx.informerFactory.WaitForCacheSync(testCtx.scheduler.StopEverything) | ||
|
||
go testCtx.scheduler.Run(testCtx.ctx) |
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.
ic, I've found it, the difference is that this helper started the scheduler inside,
@@ -99,8 +92,15 @@ func setupScheduler( | |||
} | |||
|
|||
eventBroadcaster.StartRecordingToSink(ctx.Done()) | |||
|
|||
go sched.Run(ctx) |
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.
this will run without the informers started, is that something we should be worried?
it seems all the Run and Start are done later on the tests bodys
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.
so, basically we are moving the
// Start Scheduler setupScheduler(ctx, t, clientset, informers)
to setup
, but it seems that the informers weren't started before this change , 🤷 technically should be the same
/lgtm |
Ref #108483
Note to reviewer: I recommend reviewing commits one by one - they are all independent.
/kind cleanup
/priority important-longterm
/sig testing