Share bpf maps internally and remove pinning / bpffs requirement #1251

rafaelroquetto · 2024-10-11T16:51:48Z

Leverage the fact that pinning/PinType is an uint and introduce a new PinType called PinInternal, which shares the corresponding maps inside the same userspace process (i.e. all of the maps sharing the same name refer to the same ebpf map object, but no actual bpffs pinning is given).

All bpf filesystem mounting code has been removed.

Resolves #1249

codecov-commenter · 2024-10-11T16:59:52Z

Codecov Report

Attention: Patch coverage is 63.88889% with 13 lines in your changes missing coverage. Please review.

Project coverage is 81.82%. Comparing base (ac3b137) to head (df95555).

Files with missing lines	Patch %	Lines
pkg/internal/ebpf/tracer_linux.go	61.76%	8 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1251      +/-   ##
==========================================
+ Coverage   81.78%   81.82%   +0.03%     
==========================================
  Files         135      135              
  Lines       11416    11384      -32     
==========================================
- Hits         9337     9315      -22     
+ Misses       1544     1543       -1     
+ Partials      535      526       -9

Flag	Coverage Δ
integration-test	`60.54% <63.88%> (-0.15%)`	⬇️
k8s-integration-test	`59.21% <63.88%> (+0.07%)`	⬆️
oats-test	`36.46% <63.88%> (-0.15%)`	⬇️
unittests	`53.60% <0.00%> (+0.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

rafaelroquetto · 2024-10-11T21:58:42Z

bpf/headers/bpf_helpers.h

@@ -137,6 +137,9 @@ enum libbpf_pin_type {
    LIBBPF_PIN_NONE,
    /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
    LIBBPF_PIN_BY_NAME,
+
+    // XXX custom
+    LIBBPF_PIN_INTERNAL = 100


I have decided to do this mainly so that we don't introduce a header that will need to be included everywhere, containing a single constant define.

I reckon this should be refactored if or when we decide to resplit the tracers.

I'm not sure if i understand what are you saying here. What do you mean resplit the tracers?

I'd add this explanation to a comment on LIBBPF_PIN_INTERNAL so we don't forget the context it in the future

I think we need to move this into a header of our own, since we might copy - paste a new version of libbpf headers here and cause an compile error. To the person that does this it may not be obvious what caused the build break.

@marctc I was under the impression everything was grouped together so that we could share the maps, but perhaps I am missing a part of the history. I intend to split the tc programs off generic_tracer to allow them to be loaded separately - it will be possible to do the same now for other programs if we conclude it's desirable.

@grcevski @mariomac I will move this constant elsewhere to avoid confusion.

rafaelroquetto · 2024-10-12T00:14:02Z

pkg/internal/ebpf/tracer_linux.go

+			internalMaps[k] = internalMap
+		}
+
+		collOpts.MapReplacements[k] = internalMap


cilium/ebpf will clone internalMap when processing MapReplacements, meaning both clones can be safely closed irrespective of each other (clones still share the underlying map object though, it's just the underlying file descriptor that is dup'd)

rafaelroquetto · 2024-10-12T02:12:12Z

charts/beyla/templates/daemon-set.yaml

@@ -45,31 +42,6 @@ spec:
      {{- if .Values.priorityClassName }}
      priorityClassName: {{ .Values.priorityClassName }}
      {{- end }}
-      {{- if not (.Values.privileged) }}


It would be prudent to get a second pair of eyes on these changes

We probably could do it in a separate PR IMO

@marctc ok, I will revert this part

marctc

LGTM amazing find @rafaelroquetto. Did you notice if this impacted the peformance? Personally i'd had removed the unprivileged stuff in a different PR (also to document how to run Beyla without privileges), but overall looks good!

mariomac

Amazing! I love removing code :)

As Marc says, I would let the changes in the helm chart for another PR, so we can focus on merging this feature and later test the unconfinement requirements.

mariomac · 2024-10-14T09:27:46Z

bpf/headers/bpf_helpers.h

@@ -137,6 +137,9 @@ enum libbpf_pin_type {
    LIBBPF_PIN_NONE,
    /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
    LIBBPF_PIN_BY_NAME,
+
+    // XXX custom
+    LIBBPF_PIN_INTERNAL = 100


I'd add this explanation to a comment on LIBBPF_PIN_INTERNAL so we don't forget the context it in the future

mariomac · 2024-10-14T09:30:11Z

charts/beyla/templates/daemon-set.yaml

@@ -25,9 +25,6 @@ spec:
        {{- with .Values.podAnnotations }}
        {{- toYaml . | nindent 8 }}
        {{- end }}
-        {{- if not (.Values.privileged) }}
-        container.apparmor.security.beta.kubernetes.io/beyla: "unconfined"


Actually we set this when the container is NOT privileged, so we can add the required capabilities. I don't remember if this was required only for SYS_ADMIN capability, or setting the rest of capabilities require it too.

From what I remember we had to use this "unconfined" because App Armour prevents us from mounting file systems or writing to /sys/fs. So if we don't have to mount anymore we don't need to remove this restriction.

However we shouldn't remove this in this PR. It should be a follow-up and we should probably confirm this is works now with real k8s install, since we have no tests for this.

grcevski

LGTM! Great work, minor comments :).

grcevski · 2024-10-15T13:22:49Z

bpf/headers/bpf_helpers.h

@@ -137,6 +137,9 @@ enum libbpf_pin_type {
    LIBBPF_PIN_NONE,
    /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
    LIBBPF_PIN_BY_NAME,
+
+    // XXX custom
+    LIBBPF_PIN_INTERNAL = 100


I think we need to move this into a header of our own, since we might copy - paste a new version of libbpf headers here and cause an compile error. To the person that does this it may not be obvious what caused the build break.

grcevski · 2024-10-15T13:25:33Z

charts/beyla/templates/daemon-set.yaml

@@ -25,9 +25,6 @@ spec:
        {{- with .Values.podAnnotations }}
        {{- toYaml . | nindent 8 }}
        {{- end }}
-        {{- if not (.Values.privileged) }}
-        container.apparmor.security.beta.kubernetes.io/beyla: "unconfined"


From what I remember we had to use this "unconfined" because App Armour prevents us from mounting file systems or writing to /sys/fs. So if we don't have to mount anymore we don't need to remove this restriction.

However we shouldn't remove this in this PR. It should be a follow-up and we should probably confirm this is works now with real k8s install, since we have no tests for this.

rafaelroquetto · 2024-10-15T15:45:08Z

@marctc Thanks. I will remove the changes to the helm chart, but I intend to keep the documentation changes (i.e. removing the mentions to the ebpf filesystem since they are no longer relevant). I will take care of a "how to run beyla unprivileged" in a separate PR, as this will mostly boil down to capabilities and these changes are "backwards compatible".

This reverts commit e4cdd4d.

rafaelroquetto · 2024-10-15T16:05:38Z

@grcevski @marctc @mariomac I have removed the helm charts changed, but kept the documentation ones. Let me know if you'd like me to revert the doc changes as well.

…fana#1251)

rafaelroquetto added the do-not-merge WIP or something that musn't be merged label Oct 11, 2024

rafaelroquetto force-pushed the no_more_pinning branch 5 times, most recently from 745afd8 to 4cf890f Compare October 11, 2024 21:57

rafaelroquetto commented Oct 11, 2024

View reviewed changes

rafaelroquetto changed the title ~~WIP: No more pinning~~ WIP: Share bpf maps internally and remove pinning / bpffs requirement Oct 11, 2024

rafaelroquetto added the documentation Improvements or additions to documentation label Oct 11, 2024

rafaelroquetto commented Oct 12, 2024

View reviewed changes

Introduce PinInternal pinning type

dffbf67

rafaelroquetto force-pushed the no_more_pinning branch from 3ae5f82 to 979b73f Compare October 12, 2024 00:32

rafaelroquetto commented Oct 12, 2024

View reviewed changes

rafaelroquetto changed the title ~~WIP: Share bpf maps internally and remove pinning / bpffs requirement~~ Share bpf maps internally and remove pinning / bpffs requirement Oct 12, 2024

rafaelroquetto removed the do-not-merge WIP or something that musn't be merged label Oct 12, 2024

rafaelroquetto marked this pull request as ready for review October 12, 2024 02:13

rafaelroquetto requested review from grcevski, mariomac and marctc as code owners October 12, 2024 02:13

marctc approved these changes Oct 14, 2024

View reviewed changes

mariomac reviewed Oct 14, 2024

View reviewed changes

grcevski approved these changes Oct 15, 2024

View reviewed changes

rafaelroquetto added 6 commits October 15, 2024 09:53

Introduce BEYLA_BPF_ENABLE_PINNING

828bae5

Revert "Introduce BEYLA_BPF_ENABLE_PINNING"

c65bc61

This reverts commit e4cdd4d.

Remove support for mounting bpffs

459f08d

Synchronize and cleanup maps appropriately

702e314

Update docs (but not helm charts)

bbc19ed

Move PIN_INTERNAL constant to own header

d1e9cfc

Update ebpf binaries

df95555

rafaelroquetto force-pushed the no_more_pinning branch from 979b73f to df95555 Compare October 15, 2024 15:55

rafaelroquetto merged commit c9d2647 into main Oct 15, 2024
14 checks passed

rafaelroquetto deleted the no_more_pinning branch October 15, 2024 16:51

rafaelroquetto mentioned this pull request Oct 15, 2024

Update helm charts to reflect latest map pinning changes #1261

Closed

mattdurham pushed a commit to mattdurham/beyla that referenced this pull request Jan 22, 2025

Share bpf maps internally and remove pinning / bpffs requirement (gra…

0113821

…fana#1251)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share bpf maps internally and remove pinning / bpffs requirement #1251

Share bpf maps internally and remove pinning / bpffs requirement #1251

rafaelroquetto commented Oct 11, 2024 •

edited

Loading

codecov-commenter commented Oct 11, 2024 •

edited

Loading

rafaelroquetto Oct 11, 2024

marctc Oct 14, 2024

mariomac Oct 14, 2024

grcevski Oct 15, 2024

rafaelroquetto Oct 15, 2024

rafaelroquetto Oct 12, 2024

rafaelroquetto Oct 12, 2024

marctc Oct 14, 2024

rafaelroquetto Oct 15, 2024

marctc left a comment

mariomac left a comment

mariomac Oct 14, 2024

mariomac Oct 14, 2024

grcevski Oct 15, 2024

grcevski left a comment

grcevski Oct 15, 2024

grcevski Oct 15, 2024

rafaelroquetto commented Oct 15, 2024

rafaelroquetto commented Oct 15, 2024

Share bpf maps internally and remove pinning / bpffs requirement #1251

Share bpf maps internally and remove pinning / bpffs requirement #1251

Conversation

rafaelroquetto commented Oct 11, 2024 • edited Loading

codecov-commenter commented Oct 11, 2024 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

marctc left a comment

Choose a reason for hiding this comment

mariomac left a comment

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

grcevski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelroquetto commented Oct 15, 2024

rafaelroquetto commented Oct 15, 2024

rafaelroquetto commented Oct 11, 2024 •

edited

Loading

codecov-commenter commented Oct 11, 2024 •

edited

Loading