Skip to content
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(probe/ebpf): feed initial connections synchronously on restart #3712

Merged
merged 2 commits into from
Oct 22, 2019

Conversation

bboreham
Copy link
Collaborator

Fixes #3711

If we run getInitialState() async there is some chance we will see another ebpf failure and call useProcfs() before getInitialState() gets to the last line, whereupon it will crash on nil pointer.

Also it seems pointless to call performEbpfTrack() without waiting for something to feed in, so I suspect this is what the original author had in mind.

It will slow down this one Report() on machines with a lot of processes or connections, but EbpfTracker restart is supposed to be a rare event.

If we run `getInitialState()` async there is some chance we will see
another ebpf failure and call `useProcfs()` before `getInitialState()`
gets to the last line, whereupon it will crash on nil pointer.

Also it seems pointless to call `performEbpfTrack()` without waiting
for something to feed in, so I suspect this is what the original
author had in mind.

It will slow down this one `Report()` on machines with a lot of
processes or connections, but ebpfTracker restart is supposed to be a
rare event.
Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know how to test this properly but the arguments make sense :)

There is one more instance where getInitialState() is called async on initial creation, shall we consider making it sync there as well, or do the same arguments not apply in that case?

@bboreham
Copy link
Collaborator Author

It makes sense to feed async on startup, because Report() won't get called until about 1 second later.
And it's not possible for useProcfs() to get called after that until we've gone through the synchronous path. This isn't complete protection against the crash, since it's possible the first invocation takes a really long time and the second one is quick.

I could change it to pass t.ebpfTracker into getInitialState(), then it wouldn't crash.

@fbarl
Copy link
Contributor

fbarl commented Oct 14, 2019

I could change it to pass t.ebpfTracker into getInitialState(), then it wouldn't crash.

Yeah, if it's not a big change, I think I'd be nice to remove the risk in this PR :)

It was possible for `t.ebpfTracker` to change underneath this code
while running on a background goroutine, so change it to take
`ebpfTracker` as a parameter.

While we're here, rename the functions to better match what they do.
@bboreham
Copy link
Collaborator Author

@fbarl are you ok with the refactoring to remove that risk?

@fbarl
Copy link
Contributor

fbarl commented Oct 22, 2019

@fbarl are you ok with the refactoring to remove that risk?

Yes, still LGTM, feel free to merge!

@bboreham bboreham merged commit 4bb2a43 into master Oct 22, 2019
@bboreham bboreham deleted the 3711-restart-sync branch October 22, 2019 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault after shutting down ebpf tracker
2 participants