search

weaveworks / scope

Monitoring, visualisation & management for Docker & Kubernetes
https://www.weave.works/oss/scope/
Apache License 2.0
5.87k stars 714 forks source link

fix(probe/ebpf): feed initial connections synchronously on restart #3712

Closed bboreham closed 5 years ago

bboreham commented 5 years ago

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.

bboreham commented 5 years ago

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 commented 5 years ago

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

bboreham commented 5 years ago

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

fbarl commented 5 years ago

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

Yes, still LGTM, feel free to merge!