fix goroutine leaks in TestConfigurationChannels #107163

cyclinder · 2021-12-21T12:25:05Z

Signed-off-by: cyclinder qifeng.guo@daocloud.io

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix goroutine leaks in TestConfigurationChannels.

kubernetes/pkg/util/config/config_test.go

Lines 24 to 34 in ea07644

    
           func TestConfigurationChannels(t *testing.T) { 
        
           	mux := NewMux(nil) 
        
           	channelOne := mux.Channel("one") 
        
           	if channelOne != mux.Channel("one") { 
        
           		t.Error("Didn't get the same muxuration channel back with the same name") 
        
           	} 
        
           	channelTwo := mux.Channel("two") 
        
           	if channelOne == channelTwo { 
        
           		t.Error("Got back the same muxuration channel for different names") 
        
           	} 
        
           }

Which issue(s) this PR fixes:

Fixes #107089

Special notes for your reviewer:

We should consider whether it is appropriate to use wait.NeverStop here:

kubernetes/pkg/util/config/config.go

Line 77 in 13e9745

go wait.Until(func() { m.listen(source, newChannel) }, 0, wait.NeverStop)

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-12-21T12:25:13Z

@cyclinder: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

cyclinder · 2021-12-21T12:26:28Z

/sig testing
/sig node

cyclinder · 2021-12-21T13:04:28Z

/test pull-kubernetes-integration

ash2k · 2022-01-02T05:02:49Z

pkg/util/config/config.go

+		stopCh = stopChes[0]
+	} else {
+		stopCh = wait.NeverStop
+	}


Rather than doing this for backwards compatibility, I wonder if it'd be cleaner to add a new method to the type. E.g. ChannelWithContext(ctx context.Context, source string) chan interface{}.

Actually, this is not in library code (not in staging) so might just break the signature and update the callers.

hi @ash2k, if we add a new method there, do we need to look at all callers? If so, I would like the PR to add the new method and update TestConfigurationChannels . Then create a new PR to update all the callers. and decide whether they should call the new method or the old.

I think we don't need yet another method, we can just change this one. The caller should always be responsible for stopping the goroutines they start, so a method without a way to stop them is a bad API. Hence, in my opinion, we should change this one rather just than adding a new one (change bad API to good API).

I found a lot of code in the kubelet that uses wait.NeverStop, which doesn't seem to care about stopping the goroutine. If we change this method to ChannelWithContext(ctx context.Context, source string) chan interface, we need to cancel this ctx somewhere uniformly, but I can't find the right place

It's fine, we don't need to fix the world in this PR :)

cyclinder · 2022-01-07T10:26:27Z

/cc @dims @dchen1107

dims · 2022-01-07T13:11:41Z

/assign @ash2k

@ash2k please assign it back to me after you are happy with this

pkg/kubelet/config/config_test.go

pkg/util/config/config.go

cyclinder · 2022-01-10T03:51:30Z

/test pull-kubernetes-unit pull-kubernetes-e2e-kind

cyclinder · 2022-01-10T08:37:56Z

/cc @ash2k

ash2k

/lgtm
/assign @dims

pkg/kubelet/kubelet.go

Signed-off-by: cyclinder <qifeng.guo@daocloud.io>

cyclinder · 2022-01-10T12:37:55Z

/test pull-kubernetes-unit

dims · 2022-01-10T13:06:43Z

thanks @cyclinder and @ash2k. the unit test failure seems unrelated, right?

/approve

k8s-ci-robot · 2022-01-10T13:07:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, cyclinder, dims

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/OWNERS~~ [dims]

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

cyclinder · 2022-01-10T13:24:57Z

thanks @cyclinder and @ash2k. the unit test failure seems unrelated, right?

/approve

yeah, retest was successful

ash2k · 2022-01-10T23:43:28Z

/lgtm

Thanks!

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 21, 2021

k8s-ci-robot requested review from lavalamp and smarterclayton December 21, 2021 12:26

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 21, 2021

ash2k reviewed Jan 2, 2022

View reviewed changes

cyclinder force-pushed the fix_leak_goroutine branch 2 times, most recently from 31d2e39 to f871c1e Compare January 4, 2022 13:36

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/kubelet and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 4, 2022

k8s-ci-robot requested review from dchen1107 and dims January 7, 2022 10:26

k8s-ci-robot assigned ash2k Jan 7, 2022

ash2k reviewed Jan 10, 2022

View reviewed changes

pkg/kubelet/config/config_test.go Outdated Show resolved Hide resolved

pkg/util/config/config.go Outdated Show resolved Hide resolved

cyclinder force-pushed the fix_leak_goroutine branch from f871c1e to e26370a Compare January 10, 2022 02:45

k8s-ci-robot requested a review from ash2k January 10, 2022 08:37

ash2k approved these changes Jan 10, 2022

View reviewed changes

k8s-ci-robot assigned dims Jan 10, 2022

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

ash2k reviewed Jan 10, 2022

View reviewed changes

pkg/kubelet/kubelet.go Outdated Show resolved Hide resolved

fix goroutine leaks in TestConfigurationChannels

928e686

Signed-off-by: cyclinder <qifeng.guo@daocloud.io>

cyclinder force-pushed the fix_leak_goroutine branch from e26370a to 928e686 Compare January 10, 2022 11:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2022

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

k8s-ci-robot merged commit a0dfd95 into kubernetes:master Jan 11, 2022

k8s-ci-robot added this to the v1.24 milestone Jan 11, 2022

pacoxu mentioned this pull request Jun 29, 2022

Fix missing of RLock in SeenAllSources #106551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix goroutine leaks in TestConfigurationChannels #107163

fix goroutine leaks in TestConfigurationChannels #107163

cyclinder commented Dec 21, 2021

k8s-ci-robot commented Dec 21, 2021

cyclinder commented Dec 21, 2021 •

edited

Loading

cyclinder commented Dec 21, 2021

ash2k Jan 2, 2022

ash2k Jan 2, 2022

cyclinder Jan 3, 2022

ash2k Jan 3, 2022

cyclinder Jan 4, 2022

ash2k Jan 10, 2022

cyclinder commented Jan 7, 2022

dims commented Jan 7, 2022

cyclinder commented Jan 10, 2022

cyclinder commented Jan 10, 2022

ash2k left a comment

cyclinder commented Jan 10, 2022

dims commented Jan 10, 2022

k8s-ci-robot commented Jan 10, 2022

cyclinder commented Jan 10, 2022

ash2k commented Jan 10, 2022

	func TestConfigurationChannels(t *testing.T) {
	mux := NewMux(nil)
	channelOne := mux.Channel("one")
	if channelOne != mux.Channel("one") {
	t.Error("Didn't get the same muxuration channel back with the same name")
	}
	channelTwo := mux.Channel("two")
	if channelOne == channelTwo {
	t.Error("Got back the same muxuration channel for different names")
	}
	}

fix goroutine leaks in TestConfigurationChannels #107163

fix goroutine leaks in TestConfigurationChannels #107163

Conversation

cyclinder commented Dec 21, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Dec 21, 2021

cyclinder commented Dec 21, 2021 • edited Loading

cyclinder commented Dec 21, 2021

ash2k Jan 2, 2022

Choose a reason for hiding this comment

ash2k Jan 2, 2022

Choose a reason for hiding this comment

cyclinder Jan 3, 2022

Choose a reason for hiding this comment

ash2k Jan 3, 2022

Choose a reason for hiding this comment

cyclinder Jan 4, 2022

Choose a reason for hiding this comment

ash2k Jan 10, 2022

Choose a reason for hiding this comment

cyclinder commented Jan 7, 2022

dims commented Jan 7, 2022

cyclinder commented Jan 10, 2022

cyclinder commented Jan 10, 2022

ash2k left a comment

Choose a reason for hiding this comment

cyclinder commented Jan 10, 2022

dims commented Jan 10, 2022

k8s-ci-robot commented Jan 10, 2022

cyclinder commented Jan 10, 2022

ash2k commented Jan 10, 2022

cyclinder commented Dec 21, 2021 •

edited

Loading