socket enricher: Use single instance for multiple gadgets #2324

mauriciovasquezbernal · 2023-12-22T15:42:05Z

Check the details in #1695.

alban

Thanks!

pkg/operators/socketenricher/socketenricher.go

alban · 2024-01-08T12:30:31Z

pkg/gadgets/run/tracer/tracer.go

+func (t *Tracer) SetSocketEnricherMap(m *ebpf.Map) {
+	t.socketEnricherMap = m


Does it mean that image-based gadgets would still inconditionally create the socket enricher, even if they don't use it?

You're right. It's still a pending issue we have with other operators as well (#2167), I gave it a try on #2168.

mauriciovasquezbernal · 2024-01-10T22:08:39Z

There's something going on with the tests on this PR. Holding until I have the chance to investigate more.

burak-ok · 2024-01-11T10:56:52Z

There's something going on with the tests on this PR. Holding until I have the chance to investigate more.

At least for TestProfileCpu the KubeInventoryCache crashed the gadget. After rebase + #2354 at least one test should pass again.
That crash seems to be a very rare race condition

mauriciovasquezbernal · 2024-01-11T11:07:39Z

@burak-ok does it have any relation with #2347? Because that PR is not included in this branch yet.

burak-ok · 2024-01-11T11:12:47Z

@burak-ok does it have any relation with #2347? Because that PR is not included in this branch yet.

No, that rare race condition appears to have existed until that branch. I don't know if its happening more or less with #2347

eiffel-fl

Hi.

I will take a deeper look later, but I have some initial comments.

Best regards.

pkg/operators/socketenricher/socketenricher.go

eiffel-fl · 2024-01-22T04:12:57Z

pkg/gadget-collection/gadgets/trace/network/gadget.go

+	socketEnricherOp := operators.GetRaw(socketenricher.OperatorName).(*socketenricher.SocketEnricher)
+	socketEnricherOp.Init(nil)
+	t.socketEnricherInst, err = socketEnricherOp.Instantiate(nil, t.tracer, nil)
+	if err == nil {
+		t.socketEnricherInst.PreGadgetRun()
+	}
+


I am wondering if we cannot factorize this whole logic using an array and a loop.
For now, it is OK, but if we keep adding operators, the code will definitely grow and we should avoid this.

This whole file is legacy and all this operators-related stuff there is a big hack, I hope we can get rid of it very soon.

What do we need to get rid of it?
Cannot we just drop it now?

pkg/gadgets/trace/network/tracer/tracer.go

pkg/socketenricher/tracer.go

eiffel-fl

Good optimization!
I tested it and we still have N to N maps but only 1 to N instance:

$ ps aux | grep ./ig               (remotes/origin/mauricio/optmize-socket-enricher) %
root       40174  0.0  0.0  16740  5632 pts/4    SN   11:38   0:00 sudo ./ig trace network
root       40178  0.0  0.0  16740  2296 pts/5    SNs+ 11:38   0:00 sudo ./ig trace network
root       40179  0.5  0.2 5635336 96668 pts/5   SNl  11:38   0:02 ./ig trace network
root       40210  0.0  0.0  16744  5632 pts/4    SN   11:38   0:00 sudo ./ig trace dns
root       40214  0.0  0.0  16744  2296 pts/6    SNs+ 11:38   0:00 sudo ./ig trace dns
root       40215  0.6  0.3 5635016 97600 pts/6   SNl  11:38   0:03 ./ig trace dns
francis    40788  0.0  0.0  11780  2560 pts/4    S+   11:47   0:00 grep --color=auto ./ig
$ sudo bpftool map list | grep 'gadget_sockets'
67: hash  name gadget_sockets  flags 0x0
90: hash  name gadget_sockets  flags 0x0

eiffel-fl · 2024-01-23T04:13:33Z

pkg/operators/socketenricher/socketenricher.go

+
+	i.manager.refCount--
+	if i.manager.refCount == 0 {
+		i.manager.socketEnricher.Close()


Isn't there a way to call the above Close() here?

We could but I think the semantics are not the same. (s *SocketEnricher) Close() should destroy everything related to the parent, while (i *SocketEnricherInstance) PostGadgetRun() is only for this instance. They are exactly the same now, but they could be different later on.

Move it to a non-internal pkg to use it on the next commits. Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>

Split the logic into Init() and Run() functions. Init() now only installs the network tracer, and Run() runs it. It's needed by the following commit because the socket enricher operator sets the map after Init() but before Run(). Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>

Implement a new operator that provides the socket enricher map to gadgets using it. Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>

Implement the SetSocketEnricherMap() interface in gadgets using it. Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>

Fixes: 35b9e87 ("socket enricher: make iterators optional") Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

See #2364 Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

mauriciovasquezbernal requested review from flyth, alban, blanquicet and burak-ok as code owners December 22, 2023 15:42

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch 2 times, most recently from f978966 to 8849525 Compare December 22, 2023 18:31

mauriciovasquezbernal changed the title ~~socket enricher: Use single instance for multiple gadget~~ socket enricher: Use single instance for multiple gadgets Jan 3, 2024

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch 3 times, most recently from fc775e7 to 93b7e47 Compare January 4, 2024 18:38

alban reviewed Jan 8, 2024

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch from 93b7e47 to 4ea4466 Compare January 9, 2024 12:52

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch from 4ea4466 to a310bbd Compare January 11, 2024 16:14

mauriciovasquezbernal changed the base branch from main to burak/kubecache/close January 11, 2024 20:16

burak-ok force-pushed the burak/kubecache/close branch from 7cf681b to 18710d6 Compare January 12, 2024 09:33

Base automatically changed from burak/kubecache/close to main January 12, 2024 12:39

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch from a310bbd to 54895fb Compare January 12, 2024 13:44

mauriciovasquezbernal requested review from eiffel-fl and mqasimsarfraz as code owners January 15, 2024 15:02

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch from 81a62ba to 321ec53 Compare January 15, 2024 15:26

mauriciovasquezbernal mentioned this pull request Jan 15, 2024

trace/network giving bad uid/gid on some cases #2364

Open

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch 3 times, most recently from 7de748a to bc86a15 Compare January 17, 2024 14:16

eiffel-fl reviewed Jan 22, 2024

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch from bc86a15 to 65c46d4 Compare January 22, 2024 13:09

eiffel-fl approved these changes Jan 23, 2024

View reviewed changes

mauriciovasquezbernal added 7 commits January 23, 2024 08:59

Move socket enricher to pkg/

bb12aea

Move it to a non-internal pkg to use it on the next commits. Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>

Implement socketenricher operator

305be4f

Implement a new operator that provides the socket enricher map to gadgets using it. Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>

gadgets: Adapt to SocketEnricher operator

d5a6bf1

Implement the SetSocketEnricherMap() interface in gadgets using it. Signed-off-by: Mauricio Vasquez <mauriciov@microsoft.com>

socketenricher: Release iterators programs too

2c02f31

Fixes: 35b9e87 ("socket enricher: make iterators optional") Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

network/trace: Call stop when deleting tracer instance

611d10a

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

tests/trace/network: Don't check uid/gid for nginx cases

85e8b3f

See #2364 Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

mauriciovasquezbernal force-pushed the mauricio/optmize-socket-enricher branch from 65c46d4 to 85e8b3f Compare January 23, 2024 14:15

mauriciovasquezbernal merged commit f95d4fd into main Jan 23, 2024
54 checks passed

mauriciovasquezbernal deleted the mauricio/optmize-socket-enricher branch January 23, 2024 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

socket enricher: Use single instance for multiple gadgets #2324

socket enricher: Use single instance for multiple gadgets #2324

mauriciovasquezbernal commented Dec 22, 2023

alban left a comment

alban Jan 8, 2024

mauriciovasquezbernal Jan 9, 2024

mauriciovasquezbernal commented Jan 10, 2024

burak-ok commented Jan 11, 2024

mauriciovasquezbernal commented Jan 11, 2024

burak-ok commented Jan 11, 2024 •

edited

eiffel-fl left a comment

eiffel-fl Jan 22, 2024

mauriciovasquezbernal Jan 22, 2024

eiffel-fl Jan 23, 2024

eiffel-fl left a comment

eiffel-fl Jan 23, 2024

mauriciovasquezbernal Jan 23, 2024

		func (t Tracer) SetSocketEnricherMap(m ebpf.Map) {
		t.socketEnricherMap = m

socket enricher: Use single instance for multiple gadgets #2324

socket enricher: Use single instance for multiple gadgets #2324

Conversation

mauriciovasquezbernal commented Dec 22, 2023

alban left a comment

Choose a reason for hiding this comment

alban Jan 8, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 9, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal commented Jan 10, 2024

burak-ok commented Jan 11, 2024

mauriciovasquezbernal commented Jan 11, 2024

burak-ok commented Jan 11, 2024 • edited

eiffel-fl left a comment

Choose a reason for hiding this comment

eiffel-fl Jan 22, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 22, 2024

Choose a reason for hiding this comment

eiffel-fl Jan 23, 2024

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

eiffel-fl Jan 23, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 23, 2024

Choose a reason for hiding this comment

burak-ok commented Jan 11, 2024 •

edited