-
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 leaking goroutines in multiple integration tests #110362
Changes from all commits
783da34
d9d46d5
e5ec28f
b54363a
2af8d0b
006ff45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -358,7 +358,18 @@ func verifyGroupKind(controllerRef *metav1.OwnerReference, expectedKind string, | |
|
||
func (dc *DisruptionController) Run(ctx context.Context) { | ||
defer utilruntime.HandleCrash() | ||
|
||
// Start events processing pipeline. | ||
if dc.kubeClient != nil { | ||
klog.Infof("Sending events to api server.") | ||
dc.broadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: dc.kubeClient.CoreV1().Events("")}) | ||
} else { | ||
klog.Infof("No api server defined - no events will be sent to API server.") | ||
} | ||
defer dc.broadcaster.Shutdown() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you defer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we defer it always (it's unrelated to whether we record to sink or not) |
||
|
||
defer dc.queue.ShutDown() | ||
defer dc.recheckQueue.ShutDown() | ||
|
||
klog.Infof("Starting disruption controller") | ||
defer klog.Infof("Shutting down disruption controller") | ||
|
@@ -367,12 +378,6 @@ func (dc *DisruptionController) Run(ctx context.Context) { | |
return | ||
} | ||
|
||
if dc.kubeClient != nil { | ||
klog.Infof("Sending events to api server.") | ||
dc.broadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: dc.kubeClient.CoreV1().Events("")}) | ||
} else { | ||
klog.Infof("No api server defined - no events will be sent to API server.") | ||
} | ||
go wait.UntilWithContext(ctx, dc.worker, time.Second) | ||
go wait.Until(dc.recheckWorker, time.Second, ctx.Done()) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,12 +51,12 @@ import ( | |
func TestCSRDuration(t *testing.T) { | ||
t.Parallel() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) | ||
t.Cleanup(cancel) | ||
|
||
s := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd()) | ||
t.Cleanup(s.TearDownFn) | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) | ||
t.Cleanup(cancel) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so many t.Cleanup() functions is not complex to follow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. somewhat - I was just trying to do minimal changes... |
||
|
||
// assert that the metrics we collect during the test run match expectations | ||
// we have 7 valid test cases below that request a duration of which 6 should have their duration honored | ||
wantMetricStrings := []string{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,7 +101,9 @@ func setup(t *testing.T) (*kubeapiservertesting.TestServer, *disruption.Disrupti | |
func TestPDBWithScaleSubresource(t *testing.T) { | ||
s, pdbc, informers, clientSet, apiExtensionClient, dynamicClient := setup(t) | ||
defer s.TearDownFn() | ||
ctx := context.TODO() | ||
|
||
ctx, cancel := context.WithCancel(context.Background()) | ||
defer cancel() | ||
nsName := "pdb-scale-subresource" | ||
createNs(ctx, t, nsName, clientSet) | ||
|
||
|
@@ -187,16 +189,14 @@ func TestPDBWithScaleSubresource(t *testing.T) { | |
} | ||
|
||
func TestEmptySelector(t *testing.T) { | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
defer cancel() | ||
testcases := []struct { | ||
name string | ||
createPDBFunc func(clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error | ||
createPDBFunc func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error | ||
expectedCurrentHealthy int32 | ||
}{ | ||
{ | ||
name: "v1beta1 should not target any pods", | ||
createPDBFunc: func(clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
pdb := &v1beta1.PodDisruptionBudget{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
|
@@ -213,7 +213,7 @@ func TestEmptySelector(t *testing.T) { | |
}, | ||
{ | ||
name: "v1 should target all pods", | ||
createPDBFunc: func(clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
pdb := &policyv1.PodDisruptionBudget{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
|
@@ -235,6 +235,9 @@ func TestEmptySelector(t *testing.T) { | |
s, pdbc, informers, clientSet, _, _ := setup(t) | ||
defer s.TearDownFn() | ||
|
||
ctx, cancel := context.WithCancel(context.Background()) | ||
defer cancel() | ||
|
||
nsName := fmt.Sprintf("pdb-empty-selector-%d", i) | ||
createNs(ctx, t, nsName, clientSet) | ||
|
||
|
@@ -252,7 +255,7 @@ func TestEmptySelector(t *testing.T) { | |
waitToObservePods(t, informers.Core().V1().Pods().Informer(), 4, v1.PodRunning) | ||
|
||
pdbName := "test-pdb" | ||
if err := tc.createPDBFunc(clientSet, pdbName, nsName, minAvailable); err != nil { | ||
if err := tc.createPDBFunc(ctx, clientSet, pdbName, nsName, minAvailable); err != nil { | ||
t.Errorf("Error creating PodDisruptionBudget: %v", err) | ||
} | ||
|
||
|
@@ -271,16 +274,14 @@ func TestEmptySelector(t *testing.T) { | |
} | ||
|
||
func TestSelectorsForPodsWithoutLabels(t *testing.T) { | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
defer cancel() | ||
testcases := []struct { | ||
name string | ||
createPDBFunc func(clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error | ||
createPDBFunc func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error | ||
expectedCurrentHealthy int32 | ||
}{ | ||
{ | ||
name: "pods with no labels can be targeted by v1 PDBs with empty selector", | ||
createPDBFunc: func(clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
pdb := &policyv1.PodDisruptionBudget{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
|
@@ -297,7 +298,7 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) { | |
}, | ||
{ | ||
name: "pods with no labels can be targeted by v1 PDBs with DoesNotExist selector", | ||
createPDBFunc: func(clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
pdb := &policyv1.PodDisruptionBudget{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
|
@@ -321,7 +322,7 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) { | |
}, | ||
{ | ||
name: "pods with no labels can be targeted by v1beta1 PDBs with DoesNotExist selector", | ||
createPDBFunc: func(clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error { | ||
pdb := &v1beta1.PodDisruptionBudget{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
|
@@ -350,6 +351,9 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) { | |
s, pdbc, informers, clientSet, _, _ := setup(t) | ||
defer s.TearDownFn() | ||
|
||
ctx, cancel := context.WithCancel(context.Background()) | ||
defer cancel() | ||
|
||
nsName := fmt.Sprintf("pdb-selectors-%d", i) | ||
createNs(ctx, t, nsName, clientSet) | ||
|
||
|
@@ -360,7 +364,7 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) { | |
|
||
// Create the PDB first and wait for it to settle. | ||
pdbName := "test-pdb" | ||
if err := tc.createPDBFunc(clientSet, pdbName, nsName, minAvailable); err != nil { | ||
if err := tc.createPDBFunc(ctx, clientSet, pdbName, nsName, minAvailable); err != nil { | ||
t.Errorf("Error creating PodDisruptionBudget: %v", err) | ||
} | ||
waitPDBStable(ctx, t, clientSet, 0, nsName, pdbName) | ||
|
@@ -498,9 +502,15 @@ func waitToObservePods(t *testing.T, podInformer cache.SharedIndexInformer, podN | |
} | ||
|
||
func TestPatchCompatibility(t *testing.T) { | ||
s, _, _, clientSet, _, _ := setup(t) | ||
s, pdbc, _, clientSet, _, _ := setup(t) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't you need to start the informers too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't - just creating informers doesn't start any goroutines - so we don't need to touch them |
||
defer s.TearDownFn() | ||
|
||
// Even though pdbc isn't used in this test, its creation is already | ||
// spawning some goroutines. So we need to run it to ensure they won't leak. | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
cancel() | ||
pdbc.Run(ctx) | ||
|
||
testcases := []struct { | ||
name string | ||
version string | ||
|
@@ -634,5 +644,4 @@ func TestPatchCompatibility(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.
?
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 just wasn't used anywhere :D