[config/configtls] Enable goleak check #9220

crobert-1 · 2024-01-04T22:52:55Z

Description:

This change enables goleak to check the config/configtls package for memory and goroutine leaks. There was originally a goroutine leak which is fixed by calling the newly added method TLSServerSetting.Shutdown.

The major change of this PR is to make the *clientCAsFileReloader object a member of the TLSServerSetting struct, as well as adding a public Shutdown method to TLSServerSetting. This was the cleanest way I could add a way to shut down the reloader, otherwise it will be a leaked goroutine.

Also, I had to change LoadTLSConfig() to be a pointer type receiver, so that it could modify the given object by reference instead of only having the value copy of the TLSServerSetting object it was called on.

Link to tracking Issue:
#9165

Testing:
Goleak is passing successfully, as well as all existing tests.

codecov · 2024-01-04T22:58:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.69%. Comparing base (93cbae4) to head (28e1284).
Report is 194 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9220      +/-   ##
==========================================
- Coverage   91.71%   91.69%   -0.03%     
==========================================
  Files         406      406              
  Lines       18947    18958      +11     
==========================================
+ Hits        17378    17383       +5     
- Misses       1209     1214       +5     
- Partials      360      361       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jpkrohling · 2024-01-18T14:41:09Z

Sorry about the go.mod conflict. Once that is fixed, this can be merged. Thank you!

bogdandrutu · 2024-01-22T21:06:05Z

config/configtls/configtls.go

 		if err != nil {
 			return nil, err
 		}
 		if c.ReloadClientCAFile {
-			err = reloader.startWatching()
+			err = c.reloader.startWatching()


Strange logic that we create a reloader even when not starting, maybe split the logic? Separate PR would be preferred for that.

To make sure I understand, you're saying it's possible to get here even when the collector is not starting, so we should fix this logic to make sure the reloader only starts watching during start, am I following your thought?

config/configtls/configtls.go

bogdandrutu · 2024-01-22T21:08:15Z

@jpkrohling removed the ready-to-use since I believe this does not work for multiple calls to get settings, because of that I think is better to design a different API.

github-actions · 2024-02-07T03:15:39Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

crobert-1 · 2024-02-07T23:05:12Z

config/configtls/clientcasfilereloader.go

@@ -132,7 +132,7 @@ func (r *clientCAsFileReloader) handleWatcherEvents() {

 func (r *clientCAsFileReloader) shutdown() error {
 	if r.shutdownCH == nil {
-		return fmt.Errorf("client CAs file watcher is not running")


I don't think we should return an error if the shutdown channel is nil.

From documentation:

// This method must be safe to call: // - without Start() having been called // - if the component is in a shutdown state already

Maybe I'm misinterpreting what safe to call means, but I don't think we should return an error unless there's an issue shutting down. I don't think we should error if shutdown is successful. (Which I believe is the second point from docs here).

Even though this itself isn't specifically a collector component, I believe docs still apply as it's a part of collector shutdown.

I think your change makes sense. I interpret that comment to mean "calling shutdown on something not started should not error".

.chloggen/goleak_configtls.yaml

github-actions · 2024-02-28T03:15:30Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

crobert-1 · 2024-03-08T00:37:45Z

@jpkrohling and @bogdandrutu: I believe I've addressed the concerns mentioned, another look would be appreciated when you're able!

github-actions · 2024-03-22T03:15:41Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

config/configtls/configtls.go

receiver/otlpreceiver/otlp.go

TylerHelmuth · 2024-03-25T16:41:35Z

receiver/otlpreceiver/otlp.go

+	if r.cfg != nil {
+		if r.cfg.HTTP != nil && r.cfg.HTTP.TLSSetting != nil {
+			err = r.cfg.HTTP.TLSSetting.Shutdown()
+		}
+
+		if r.cfg.GRPC != nil && r.cfg.GRPC.TLSSetting != nil {
+			err = multierr.Append(err, r.cfg.GRPC.TLSSetting.Shutdown())
+		}
+	}


Looking at this some more I really don't like that any component that uses configtls has to add this check. I am worried about components not realizing that the configtls.TLSSetting embedded in confighttp.ServerConfig needs a Shutdown call.

I think it is out of scope of this PR, which is only trying to solve a leak added by existing functionality, but I think we should take another look at how config modules should handle goroutines and Shutdown.

github-actions · 2024-05-02T03:15:44Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

yurishkuro · 2024-05-19T22:31:51Z

Q: why do we implement reloading of client CA file with a background goroutine instead of using the same approach as certReloader which does it on-demand when GetCertificate is called?

opentelemetry-collector/config/configtls/configtls.go

Line 140 in ede9e30

type certReloader struct {

This would avoid a goroutine leak and the unpleasant side-effects of having to call Shutdown on a "config" object.

github-actions · 2024-06-05T03:16:24Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2024-06-23T03:15:52Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

mx-psi · 2024-07-18T11:18:16Z

@crobert-1 What's the status of this PR? Does it need more reviews? Do you need to fix something? (No rush, just trying to clean up our backlog)

crobert-1 · 2024-07-18T15:47:01Z

@crobert-1 What's the status of this PR? Does it need more reviews? Do you need to fix something? (No rush, just trying to clean up our backlog)

Just waiting for reviews 👍

mx-psi · 2024-07-19T08:29:37Z

Can you take a look at Yuri's question above?

mx-psi · 2024-08-06T08:28:18Z

@crobert-1 Mind taking a look at #9220 (comment) ?

crobert-1 · 2024-08-08T17:42:30Z

@crobert-1 Mind taking a look at #9220 (comment) ?

My apologies for the delay, thanks for the ping! I'm taking a look at this. There's no context from the original issue or implementing PR why this design was chosen. However, the issue says it should be done like the certReloader, which, as @yurishkuro pointed out, is done in a lazy manner on Get.

I'm working on implementing this to simply read the file on Get rather than the proactive watching approach that's currently implemented. If it works properly, I'll open a new PR instead of this approach.

Marking as draft until I get a chance to properly prove the alternative approach will work.

github-actions · 2024-08-23T03:16:10Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2024-09-11T03:16:12Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

akorompai · 2024-10-15T21:38:28Z

@crobert-1 Mind taking a look at #9220 (comment) ?

My apologies for the delay, thanks for the ping! I'm taking a look at this. There's no context from the original issue or implementing PR why this design was chosen. However, the issue says it should be done like the certReloader, which, as @yurishkuro pointed out, is done in a lazy manner on Get.

I'm working on implementing this to simply read the file on Get rather than the proactive watching approach that's currently implemented. If it works properly, I'll open a new PR instead of this approach.

Marking as draft until I get a chance to properly prove the alternative approach will work.

@crobert-1 Sorry for all the troubles my PR is causing, I didn't realize that the shutdown concern wasn't addressed before it got merged (#6708 (comment))
Please note that a dedicated client certReloader was implemented in a previous PR, but got rejected in favor of a fs watcher solution (#6525 (comment))
I'm happy to recreate the PR with some additional tests if needed!

crobert-1 requested review from a team and jpkrohling January 4, 2024 22:52

crobert-1 mentioned this pull request Jan 4, 2024

[config/configtls] Document possible goroutine leak caused by TLSServerSetting #9221

Open

jpkrohling approved these changes Jan 18, 2024

View reviewed changes

jpkrohling approved these changes Jan 22, 2024

View reviewed changes

jpkrohling added the ready-to-merge Code review completed; ready to merge by maintainers label Jan 22, 2024

bogdandrutu reviewed Jan 22, 2024

View reviewed changes

bogdandrutu removed the ready-to-merge Code review completed; ready to merge by maintainers label Jan 22, 2024

github-actions bot added the Stale label Feb 7, 2024

crobert-1 force-pushed the goleak_configtls branch from f00f265 to f805a97 Compare February 7, 2024 23:01

crobert-1 commented Feb 7, 2024

View reviewed changes

crobert-1 marked this pull request as draft February 7, 2024 23:15

github-actions bot removed the Stale label Feb 8, 2024

crobert-1 force-pushed the goleak_configtls branch from 04a3d73 to 5cefecb Compare February 8, 2024 20:44

crobert-1 marked this pull request as ready for review February 8, 2024 21:54

github-actions bot added Stale and removed Stale labels Feb 28, 2024

github-actions bot added the Stale label Mar 22, 2024

TylerHelmuth reviewed Mar 22, 2024

View reviewed changes

config/configtls/configtls.go Outdated Show resolved Hide resolved

config/configtls/configtls.go Outdated Show resolved Hide resolved

TylerHelmuth reviewed Mar 22, 2024

View reviewed changes

receiver/otlpreceiver/otlp.go Outdated Show resolved Hide resolved

TylerHelmuth reviewed Mar 25, 2024

View reviewed changes

TylerHelmuth approved these changes Mar 25, 2024

View reviewed changes

TylerHelmuth mentioned this pull request Mar 25, 2024

[config] Investigate how goroutines and Shutdown should be handled in config modules #9831

Open

crobert-1 force-pushed the goleak_configtls branch from 9683569 to d3e9273 Compare April 17, 2024 21:38

github-actions bot added the Stale label May 2, 2024

Merge branch 'main' into goleak_configtls

56286ea

github-actions bot removed the Stale label May 9, 2024

yurishkuro mentioned this pull request May 20, 2024

Create new grpc storage configuration to align with OTEL jaegertracing/jaeger#5331

Merged

4 tasks

github-actions bot added the Stale label Jun 5, 2024

crobert-1 added 3 commits June 6, 2024 08:59

Merge branch 'main' into goleak_configtls

0445a81

Remove reference to deleted method

7466188

Remove deprecated method

f24d549

github-actions bot removed the Stale label Jun 8, 2024

github-actions bot added the Stale label Jun 23, 2024

Merge branch 'main' into goleak_configtls

50913fd

github-actions bot removed the Stale label Jul 8, 2024

Merge branch 'main' into goleak_configtls

60860a4

Merge branch 'main' into goleak_configtls

28e1284

crobert-1 marked this pull request as draft August 8, 2024 17:42

github-actions bot added the Stale label Aug 23, 2024

github-actions bot closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[config/configtls] Enable goleak check #9220

[config/configtls] Enable goleak check #9220

crobert-1 commented Jan 4, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

jpkrohling commented Jan 18, 2024

bogdandrutu Jan 22, 2024

crobert-1 Jan 23, 2024

bogdandrutu commented Jan 22, 2024

github-actions bot commented Feb 7, 2024

crobert-1 Feb 7, 2024

TylerHelmuth Mar 22, 2024

github-actions bot commented Feb 28, 2024

crobert-1 commented Mar 8, 2024

github-actions bot commented Mar 22, 2024

TylerHelmuth Mar 25, 2024 •

edited

Loading

github-actions bot commented May 2, 2024

yurishkuro commented May 19, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 23, 2024

mx-psi commented Jul 18, 2024

crobert-1 commented Jul 18, 2024

mx-psi commented Jul 19, 2024

mx-psi commented Aug 6, 2024

crobert-1 commented Aug 8, 2024

github-actions bot commented Aug 23, 2024

github-actions bot commented Sep 11, 2024

akorompai commented Oct 15, 2024 •

edited

Loading

[config/configtls] Enable goleak check #9220

[config/configtls] Enable goleak check #9220

Conversation

crobert-1 commented Jan 4, 2024

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

jpkrohling commented Jan 18, 2024

bogdandrutu Jan 22, 2024

Choose a reason for hiding this comment

crobert-1 Jan 23, 2024

Choose a reason for hiding this comment

bogdandrutu commented Jan 22, 2024

github-actions bot commented Feb 7, 2024

crobert-1 Feb 7, 2024

Choose a reason for hiding this comment

TylerHelmuth Mar 22, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 28, 2024

crobert-1 commented Mar 8, 2024

github-actions bot commented Mar 22, 2024

TylerHelmuth Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented May 2, 2024

yurishkuro commented May 19, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 23, 2024

mx-psi commented Jul 18, 2024

crobert-1 commented Jul 18, 2024

mx-psi commented Jul 19, 2024

mx-psi commented Aug 6, 2024

crobert-1 commented Aug 8, 2024

github-actions bot commented Aug 23, 2024

github-actions bot commented Sep 11, 2024

akorompai commented Oct 15, 2024 • edited Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

TylerHelmuth Mar 25, 2024 •

edited

Loading

akorompai commented Oct 15, 2024 •

edited

Loading