Fix leaking goroutines in multiple integration tests #110362

wojtek-t · 2022-06-02T19:04:22Z

Note to reviewer: I recommend reviewing commits one by one - they are all independent.

NONE

/kind cleanup
/priority important-longterm
/sig testing

k8s-ci-robot · 2022-06-02T19:07:29Z

[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:

~~pkg/controller/certificates/OWNERS~~ [wojtek-t]
~~pkg/controller/disruption/OWNERS~~ [wojtek-t]
~~pkg/controller/nodelifecycle/OWNERS~~ [wojtek-t]
~~pkg/controlplane/OWNERS~~ [wojtek-t]
~~test/OWNERS~~ [wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aojea · 2022-06-02T20:25:30Z



1/43 Tests Failed. | expand_less
-- | --
verify: gofmt expand_less6s{Script Error ScriptError diff -u ./test/integration/network/services_test.go.orig ./test/integration/network/services_test.go --- ./test/integration/network/services_test.go.orig	2022-06-02 19:27:57.762267425 +0000 +++ ./test/integration/network/services_test.go	2022-06-02 19:27:57.762267425 +0000 @@ -48,7 +48,6 @@  		ModifyServerConfig: func(cfg *controlplane.Config) {  			cfg.ExtraConfig.RepairServicesInterval = interval  		}, -  	})  	defer tearDownFn()    Run ./hack/update-gofmt.sh} | verify: gofmt expand_less | 6s | {Script Error ScriptError diff -u ./test/integration/network/services_test.go.orig ./test/integration/network/services_test.go --- ./test/integration/network/services_test.go.orig	2022-06-02 19:27:57.762267425 +0000 +++ ./test/integration/network/services_test.go	2022-06-02 19:27:57.762267425 +0000 @@ -48,7 +48,6 @@  		ModifyServerConfig: func(cfg *controlplane.Config) {  			cfg.ExtraConfig.RepairServicesInterval = interval  		}, -  	})  	defer tearDownFn()    Run ./hack/update-gofmt.sh}
verify: gofmt expand_less | 6s
{Script Error ScriptError diff -u ./test/integration/network/services_test.go.orig ./test/integration/network/services_test.go --- ./test/integration/network/services_test.go.orig	2022-06-02 19:27:57.762267425 +0000 +++ ./test/integration/network/services_test.go	2022-06-02 19:27:57.762267425 +0000 @@ -48,7 +48,6 @@  		ModifyServerConfig: func(cfg *controlplane.Config) {  			cfg.ExtraConfig.RepairServicesInterval = interval  		}, -  	})  	defer tearDownFn()    Run ./hack/update-gofmt.sh}

aojea · 2022-06-02T20:36:38Z

pkg/controller/disruption/disruption.go

+	} else {
+		klog.Infof("No api server defined - no events will be sent to API server.")
+	}
+	defer dc.broadcaster.Shutdown()


do you defer dc.broadcaster.Shutdown() always or only if it "startedRecordingToSink" ?

we defer it always (it's unrelated to whether we record to sink or not)

aojea · 2022-06-02T20:55:30Z

test/integration/disruption/disruption_test.go

@@ -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)


don't you need to start the informers too?

we don't - just creating informers doesn't start any goroutines - so we don't need to touch them

aojea · 2022-06-02T20:57:58Z

pkg/controller/certificates/certificate_controller.go

-	// Send events to the apiserver
-	eventBroadcaster := record.NewBroadcaster()
-	eventBroadcaster.StartStructuredLogging(0)
-	eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")})


This just wasn't used anywhere :D

aojea · 2022-06-02T21:08:48Z

test/integration/certificates/duration_test.go

 	s := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd())
 	t.Cleanup(s.TearDownFn)

+	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute)
+	t.Cleanup(cancel)


so many t.Cleanup() functions is not complex to follow?

somewhat - I was just trying to do minimal changes...

dims · 2022-06-03T14:05:33Z

looks like need to Run ./hack/update-gofmt.sh @wojtek-t

wojtek-t

@aojea - thanks for comments - PTAL

wojtek-t · 2022-06-06T18:31:45Z

test/integration/certificates/duration_test.go

 	s := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd())
 	t.Cleanup(s.TearDownFn)

+	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute)
+	t.Cleanup(cancel)


somewhat - I was just trying to do minimal changes...

wojtek-t · 2022-06-06T18:32:14Z

test/integration/disruption/disruption_test.go

@@ -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)


we don't - just creating informers doesn't start any goroutines - so we don't need to touch them

aojea · 2022-06-07T14:32:32Z

/lgtm

fedebongio · 2022-06-07T20:04:50Z

/triage accepted

wojtek-t added 2 commits June 2, 2022 21:02

Clean shutdown of disruption integration tests

783da34

Clean shutdown of certificates integration tests

d9d46d5

wojtek-t assigned aojea Jun 2, 2022

k8s-ci-robot added the area/test label Jun 2, 2022

k8s-ci-robot requested review from aojea and kow3ns June 2, 2022 19:07

aojea reviewed Jun 2, 2022

View reviewed changes

enj added this to Needs Triage in SIG Auth Old Jun 6, 2022

wojtek-t added 4 commits June 6, 2022 20:33

Clean shutdown of network integration tests

e5ec28f

Clean shutdown of storageversion integration tests

b54363a

Clean shutdown of events integration tests

2af8d0b

Clean shutdown of nodecontroller integration tests

006ff45

wojtek-t force-pushed the fix_leaking_goroutines_5 branch from 6ba56e2 to 006ff45 Compare June 6, 2022 18:33

wojtek-t commented Jun 6, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2022

k8s-ci-robot merged commit 39b6e6c into kubernetes:master Jun 7, 2022

SIG Auth Old automation moved this from Needs Triage to Closed / Done Jun 7, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 7, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix leaking goroutines in multiple integration tests #110362

Fix leaking goroutines in multiple integration tests #110362

wojtek-t commented Jun 2, 2022

k8s-ci-robot commented Jun 2, 2022

aojea commented Jun 2, 2022

aojea Jun 2, 2022

wojtek-t Jun 6, 2022

aojea Jun 2, 2022

wojtek-t Jun 6, 2022

aojea Jun 2, 2022

wojtek-t Jun 6, 2022

aojea Jun 2, 2022

wojtek-t Jun 6, 2022

dims commented Jun 3, 2022

wojtek-t left a comment

wojtek-t Jun 6, 2022

wojtek-t Jun 6, 2022

aojea commented Jun 7, 2022

fedebongio commented Jun 7, 2022

Fix leaking goroutines in multiple integration tests #110362

Fix leaking goroutines in multiple integration tests #110362

Conversation

wojtek-t commented Jun 2, 2022

k8s-ci-robot commented Jun 2, 2022

aojea commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Jun 3, 2022

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jun 7, 2022

fedebongio commented Jun 7, 2022