pillar/devicenetwork: fix goroutine leak #4409

christoph-zededa · 2024-10-31T11:57:14Z

introduced in 8573372

without this fix, the goroutine is blocked by trying to send
into a channel that has no receiver

OhmSpectator

I prefer just to believe it works... I tried and failed to understand the async and channel magic in the function. We use one channel as a semaphore, another - for signalling an IP and the third for checking to quit...

OhmSpectator · 2024-10-31T13:10:33Z

@christoph-zededa, I see you moved the PR into the draft state. Any concerns there?

christoph-zededa · 2024-10-31T13:18:15Z

I prefer just to believe it works... I tried and failed to understand the async and channel magic in the function. We use one channel as a semaphore, another - for signalling an IP and the third for checking to quit...

I heard you want more: https://github.com/lf-edge/eve/pull/4409/files#diff-73f9b01c33a31700345aa9b1f29289dfdb697a5593a9585cf68443155aa12e50R169

christoph-zededa · 2024-10-31T13:18:31Z

@christoph-zededa, I see you moved the PR into the draft state. Any concerns there?

There were some more leaks ...

OhmSpectator · 2024-10-31T13:26:26Z

pkg/pillar/devicenetwork/dns.go

 	go func() {
 		wg.Wait()
 		close(wgChan)
 	}()

+	defer close(quit)


Did you see this leak in the sigusr1 log file we investigated earlier?

I see it now.
There are goroutines that are stuck in line 164 waiting for wg.Wait() and because quit was not closed in all cases, not all wg.Done() were called.

Ok, I see. 19170 occurrences.

OhmSpectator · 2024-10-31T13:29:26Z

I prefer just to believe it works... I tried and failed to understand the async and channel magic in the function. We use one channel as a semaphore, another - for signalling an IP and the third for checking to quit...

I heard you want more: https://github.com/lf-edge/eve/pull/4409/files#diff-73f9b01c33a31700345aa9b1f29289dfdb697a5593a9585cf68443155aa12e50R169

😨😨😨

OhmSpectator

Can we use the same leak-detecting lib for the other components as well?

OhmSpectator · 2024-10-31T13:34:17Z

pkg/pillar/devicenetwork/dns.go

@@ -166,12 +166,17 @@ func ResolveWithPortsLambda(domain string,

 	wgChan := make(chan struct{})

+	wgWaitDone := make(chan struct{})


Is it a fix for a leak or a prerequisite for a test? Are you sure it's in the right commit?

It is needed for test, because otherwise the following goroutine would still persist when the test already finished and then it would get detected by the leak detector.

OhmSpectator · 2024-10-31T13:38:56Z

@christoph-zededa, if you find and fix more leaks, could you please drop here information about which versions are affected by them? We need this information to understand where to backport (or just inform users who use these versions). Also, it may make sense to make a commit per fix to make it easier to track the backports.

milan-zededa · 2024-10-31T13:43:58Z

pkg/pillar/devicenetwork/dns.go

 	}()

+	defer func() {
+		close(quit)
+		<-wgWaitDone


Why not just use <-wgChan here?

No reason - it is simply better to use <-wgChan here :-) Thx.

christoph-zededa · 2024-10-31T14:31:22Z

@christoph-zededa, if you find and fix more leaks, could you please drop here information about which versions are affected by them? We need this information to understand where to backport (or just inform users who use these versions). Also, it may make sense to make a commit per fix to make it easier to track the backports.

They were all introduced with 8573372 .

OhmSpectator · 2024-10-31T14:33:41Z

@christoph-zededa, if you find and fix more leaks, could you please drop here information about which versions are affected by them? We need this information to understand where to backport (or just inform users who use these versions). Also, it may make sense to make a commit per fix to make it easier to track the backports.

They were all introduced with 8573372 .

Got it, thanks!

eriknordmark

LGTM.

Did the added test logic detect the failiure in the old code?
(Running that test on the old code would be a useful check.)

christoph-zededa · 2024-10-31T16:46:28Z

LGTM.

Did the added test logic detect the failiure in the old code? (Running that test on the old code would be a useful check.)

Yes, that's how I found all the additional problems; the original fix was only:

diff --git a/pkg/pillar/devicenetwork/dns.go b/pkg/pillar/devicenetwork/dns.go
index 7f31eb11e..80f49be55 100644
--- a/pkg/pillar/devicenetwork/dns.go
+++ b/pkg/pillar/devicenetwork/dns.go
@@ -150,7 +150,10 @@ func ResolveWithPortsLambda(domain string,
                                                errs = append(errs, err)
                                        }
                                        if response != nil {
-                                               resolvedIPsChan <- response
+                                               select {
+                                               case <-resolvedIPsChan:
+                                               case resolvedIPsChan <- response:
+                                               }
                                        }
                                        <-work
                                        wg.Done()
@@ -183,6 +186,7 @@ func ResolveWithPortsLambda(domain string,
                }
                return responses, errs
        case ip := <-resolvedIPsChan:
+               close(resolvedIPsChan)
                close(quit)
                return ip, nil
        }

christoph-zededa · 2024-10-31T22:48:39Z

No idea what problems eden has, is it because of Halloween? 👻

Locally I could start eve and onboard it to the commercial controller.

OhmSpectator · 2024-11-01T09:08:16Z

No idea what problems eden has, is it because of Halloween? 👻

Locally I could start eve and onboard it to the commercial controller.

All the tests show the same error:

time="2024-10-31T20:25:08Z" level=info msg="Adam waiting for EVE registration (14) of (20)"
time="2024-10-31T20:25:28Z" level=info msg="Adam waiting for EVE registration (15) of (20)"
time="2024-10-31T20:25:48Z" level=info msg="Adam waiting for EVE registration (16) of (20)"
time="2024-10-31T20:26:08Z" level=info msg="Adam waiting for EVE registration (17) of (20)"
time="2024-10-31T20:26:28Z" level=info msg="Adam waiting for EVE registration (18) of (20)"
time="2024-10-31T20:26:48Z" level=info msg="Adam waiting for EVE registration (19) of (20)"
time="2024-10-31T20:27:08Z" level=fatal msg="Eve onboard failed: error onboarding onboarding timeout. You may try to run 'eden eve onboard' command again in several minutes. If not successful see logs of adam/eve"

Some glitch, I hope. I am restarting the tests.

OhmSpectator · 2024-11-01T09:34:33Z

Tests restart does not help =\

cperakis · 2024-11-01T13:21:46Z

@christoph-zededa @OhmSpectator @eriknordmark I think we should review how frequently we invoke this functionality to update the DNS resolution for the Ethernet interfaces' IP addresses.

In my opinion, the DNS names are unlikely to change often—probably only when the IP addresses of the interfaces themselves change. Therefore, we might not need to perform updates so frequently.

Also we could do that only if the IP of the ethernet interfaces will change.

What are your thoughts?

christoph-zededa · 2024-11-01T13:41:19Z

@christoph-zededa @OhmSpectator @eriknordmark I think we should review how frequently we invoke this

CC @milan-zededa

functionality to update the DNS resolution for the Ethernet interfaces' IP addresses.

In my opinion, the DNS names are unlikely to change often—probably only when the IP addresses of the interfaces themselves change. Therefore, we might not need to perform updates so frequently.

Also we could do that only if the IP of the ethernet interfaces will change.

What are your thoughts?

It is already cached here: https://github.com/lf-edge/eve/blob/master/pkg/pillar/cmd/nim/resolvercache.go#L108

I guess we do not cache if resolving the name did not succeed and then it gets called again and again.

uncleDecart · 2024-11-01T14:12:13Z

I guess we do not cache if resolving the name did not succeed and then it gets called again and again.

Yes, so calls are not that often

OhmSpectator · 2024-11-01T14:56:14Z

So, are we waiting for a new suggestion from @uncleDecart ?

christoph-zededa · 2024-11-01T15:01:11Z

So, are we waiting for a new suggestion from @uncleDecart ?

IMO we don't have to wait; we can always create another PR.

uncleDecart · 2024-11-01T15:09:28Z

I'd wait, since it's about this PR :D

Edit: giving it a second though it might take me a bit to write a fix for it, I'm fine with both options, if we merge it I'll create separate PR and we can talk about it there

mzampetakis · 2024-11-04T09:34:07Z

pkg/pillar/devicenetwork/dns.go

 	go func() {
 		wg.Wait()
 		close(wgChan)
 	}()

+	defer func() {


resolvedIPsChan and work channels are not closed at all. We could have a defer func to close them too.

They get gc-ed, but I added it explicitly.

mzampetakis · 2024-11-04T09:37:56Z

pkg/pillar/devicenetwork/dns.go

+					defer func() {
+						wg.Done()
+						<-work
+					}()


I think that the following lines (

select { case work <- struct{}{}: // if writable, means less than dnsMaxParallelRequests goroutines are currently running }

)
could be simplified to

work <- struct{}{}

uncleDecart · 2024-11-04T11:35:59Z

@christoph-zededa how do you like this? I think that's easier to read, if it's good, I'll come up with some tests for it and we can include this solution

christoph-zededa · 2024-11-04T11:53:48Z

@christoph-zededa how do you like this? I think that's easier to read, if it's good, I'll come up with some tests for it and we can include this solution

@uncleDecart I agree it makes it easier to read. Thx.

uncleDecart · 2024-11-04T12:25:41Z

Do you want me to create another PR, add commit to your PR or you want incorporate those changes yourself? I'm fine with any case

The one thing I'm not sure about is timeout, we can add timeout context in high level function and pass it to resolveDNS, but then we gotta deal with canceling things, however my concern is about weather or not we should have it in the first place. Meaning we usually have eventual consistency with no time-constraints, i.e. it might take a while to resolve dns, but we'll get there one way or another, setting timeout means that no matter what hardware or configuration we're running this thing on we should be able to resolve it in this particular timeframe. Can we guarantee that? Well, I mean, in theory yes, but then we have to remember about this particular part in the system and we should create an Eden test for it, so that. At least we will be able to catch a regression on a virtualised hardware, not the slowest one to handle things, but close

christoph-zededa · 2024-11-04T12:34:07Z

Do you want me to create another PR, add commit to your PR or you want incorporate those changes yourself? I'm fine with any case

Just create a PR into my PR.

The one thing I'm not sure about is timeout, we can add timeout context in high level function and pass it to resolveDNS

That's a new feature and not a bug fix, isn't it?

uncleDecart · 2024-11-04T12:56:09Z

Just create a PR into my PR.

So that you could review my PR while I review yours? Sure. I'll think about the tests and then open a PR

That's a new feature and not a bug fix, isn't it?

Agreed, I wouldn't go down that path, let's fix the bug

OhmSpectator · 2024-11-04T13:09:12Z

Mmm, guys, I would say, first of all, we need to fix the very specific bug in the scope of this PR. Time is still matters in this case. Refactoring and readability improvements - we can do it in dedicated PRs.

uncleDecart · 2024-11-04T13:15:43Z

Mmm, guys, I would say, first of all, we need to fix the very specific bug in the scope of this PR. Time is still matters in this case. Refactoring and readability improvements - we can do it in dedicated PRs.

Then let's merge this PR @OhmSpectator and then I'll open mine

OhmSpectator

Trying to force the workflows to go further...

christoph-zededa · 2024-11-05T18:37:39Z

Test failed, because it also needs a:

-       resolvedIPsChan := make(chan []DNSResponse)
+       resolvedIPsChan := make(chan []DNSResponse, 1)

introduced in 8573372 without this fix, the goroutine is blocked by trying to send into a channel that has no receiver Signed-off-by: Christoph Ostarek <[email protected]>

use fancy uber leak checker to check for leaks which have been fixed in the commit before Signed-off-by: Christoph Ostarek <[email protected]>

eriknordmark · 2024-11-05T21:17:35Z

I guess we do not cache if resolving the name did not succeed and then it gets called again and again.

@milan-zededa
Do we know that names we try to resolve using this code? Is it only the controller domain name? Or also things used by diag (like www.google.com)?

If we know that we should be try to reproduce the issue by having DNS lookups fail for some of the names which we resolve (but still want the device to be able to talk to the controller).

eriknordmark

LGTM - re-run tests

milan-zededa · 2024-11-06T09:21:35Z

I guess we do not cache if resolving the name did not succeed and then it gets called again and again.

@milan-zededa Do we know that names we try to resolve using this code? Is it only the controller domain name? Or also things used by diag (like www.google.com)?

If we know that we should be try to reproduce the issue by having DNS lookups fail for some of the names which we resolve (but still want the device to be able to talk to the controller).

It is only used for the controller domain name.
Maybe we could reproduce this issue by having multiple DNS servers published by the DHCP server, where one can resolve controller name while others fail (or possibly are not reachable/responsive).

OhmSpectator

I think we should merge now and backport into 13.4-stable, 12.0-stable and 11.0-stable...

christoph-zededa requested a review from milan-zededa as a code owner October 31, 2024 11:57

christoph-zededa added bug Something isn't working stable Should be backported to stable release(s) go Pull requests that update Go code labels Oct 31, 2024

github-actions bot requested a review from eriknordmark October 31, 2024 11:57

milan-zededa approved these changes Oct 31, 2024

View reviewed changes

OhmSpectator approved these changes Oct 31, 2024

View reviewed changes

christoph-zededa marked this pull request as draft October 31, 2024 12:44

christoph-zededa force-pushed the dns_fix_goroutine_leak branch from a4f6777 to efc0671 Compare October 31, 2024 13:17

github-actions bot requested a review from milan-zededa October 31, 2024 13:18

christoph-zededa force-pushed the dns_fix_goroutine_leak branch from efc0671 to 841bd3c Compare October 31, 2024 13:20

OhmSpectator reviewed Oct 31, 2024

View reviewed changes

OhmSpectator approved these changes Oct 31, 2024

View reviewed changes

milan-zededa reviewed Oct 31, 2024

View reviewed changes

christoph-zededa force-pushed the dns_fix_goroutine_leak branch from 841bd3c to cb967ba Compare October 31, 2024 14:40

github-actions bot requested a review from milan-zededa October 31, 2024 14:40

eriknordmark approved these changes Oct 31, 2024

View reviewed changes

OhmSpectator approved these changes Oct 31, 2024

View reviewed changes

christoph-zededa marked this pull request as ready for review October 31, 2024 16:46

OhmSpectator approved these changes Nov 1, 2024

View reviewed changes

mzampetakis reviewed Nov 4, 2024

View reviewed changes

christoph-zededa force-pushed the dns_fix_goroutine_leak branch from cb967ba to 688d6bc Compare November 4, 2024 13:11

github-actions bot requested a review from eriknordmark November 4, 2024 13:11

christoph-zededa force-pushed the dns_fix_goroutine_leak branch from 688d6bc to acff711 Compare November 4, 2024 13:16

OhmSpectator approved these changes Nov 4, 2024

View reviewed changes

christoph-zededa force-pushed the dns_fix_goroutine_leak branch from acff711 to 61ebeb7 Compare November 5, 2024 17:59

christoph-zededa added 2 commits November 5, 2024 19:37

pillar/devicenetwork: fix goroutine leak

af161e6

introduced in 8573372 without this fix, the goroutine is blocked by trying to send into a channel that has no receiver Signed-off-by: Christoph Ostarek <[email protected]>

pillar/devicenetwork: goroutine leak test

6bf74f5

use fancy uber leak checker to check for leaks which have been fixed in the commit before Signed-off-by: Christoph Ostarek <[email protected]>

christoph-zededa force-pushed the dns_fix_goroutine_leak branch from 61ebeb7 to 6bf74f5 Compare November 5, 2024 18:38

eriknordmark approved these changes Nov 5, 2024

View reviewed changes

OhmSpectator approved these changes Nov 6, 2024

View reviewed changes

OhmSpectator merged commit 145e98e into lf-edge:master Nov 6, 2024
45 of 67 checks passed

		@@ -166,12 +166,17 @@ func ResolveWithPortsLambda(domain string,

		wgChan := make(chan struct{})

		wgWaitDone := make(chan struct{})

pillar/devicenetwork: fix goroutine leak #4409

pillar/devicenetwork: fix goroutine leak #4409

Conversation

christoph-zededa commented Oct 31, 2024

OhmSpectator left a comment

Choose a reason for hiding this comment

OhmSpectator commented Oct 31, 2024

christoph-zededa commented Oct 31, 2024

christoph-zededa commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhmSpectator commented Oct 31, 2024

OhmSpectator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhmSpectator commented Oct 31, 2024

Choose a reason for hiding this comment

christoph-zededa Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

christoph-zededa commented Oct 31, 2024

OhmSpectator commented Oct 31, 2024

eriknordmark left a comment

Choose a reason for hiding this comment

christoph-zededa commented Oct 31, 2024

christoph-zededa commented Oct 31, 2024 • edited Loading

OhmSpectator commented Nov 1, 2024

OhmSpectator commented Nov 1, 2024

cperakis commented Nov 1, 2024

christoph-zededa commented Nov 1, 2024

uncleDecart commented Nov 1, 2024

OhmSpectator commented Nov 1, 2024

christoph-zededa commented Nov 1, 2024

uncleDecart commented Nov 1, 2024 • edited Loading

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

uncleDecart commented Nov 4, 2024

christoph-zededa commented Nov 4, 2024

uncleDecart commented Nov 4, 2024

christoph-zededa commented Nov 4, 2024

uncleDecart commented Nov 4, 2024

OhmSpectator commented Nov 4, 2024

uncleDecart commented Nov 4, 2024

OhmSpectator left a comment

Choose a reason for hiding this comment

christoph-zededa commented Nov 5, 2024

eriknordmark commented Nov 5, 2024

eriknordmark left a comment

Choose a reason for hiding this comment

milan-zededa commented Nov 6, 2024

OhmSpectator left a comment

Choose a reason for hiding this comment

christoph-zededa Oct 31, 2024 •

edited

Loading

christoph-zededa commented Oct 31, 2024 •

edited

Loading

uncleDecart commented Nov 1, 2024 •

edited

Loading