tetragon: Add map ownership #2945

olsajiri · 2024-09-22T19:14:16Z

Adding the concept of owning map and splitting Map object to owners and others.

The idea is that the 'owned' Map object takes care of creating/overwriting the pin,
while the 'other' Map object is just a user, that can read/update map but can't create it.

I'm not really happy with the 'other' naming, so any ideas are welcome ;-)

netlify · 2024-09-24T08:23:17Z

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	`f98ce9d`
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/66f3c117b3d7c1000848e0ff
😎 Deploy Preview	https://deploy-preview-2945--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tixxdz · 2024-09-25T17:04:40Z

Adding the concept of owning map and splitting Map object to owners and others.

The idea is that the 'owned' Map object takes care of creating/overwriting the pin, while the 'other' Map object is just a user, that can read/update map but can't create it.

What are the possible use cases for it? restart ? or is it just to organize better?

I'm not really happy with the 'other' naming, so any ideas are welcome ;-)

Hmm others could be fine, or "non-owners"?, but those are certainly better than "users" ? or go with creators and users? ...

olsajiri · 2024-09-25T18:31:06Z

Adding the concept of owning map and splitting Map object to owners and others.
The idea is that the 'owned' Map object takes care of creating/overwriting the pin, while the 'other' Map object is just a user, that can read/update map but can't create it.

What are the possible use cases for it? restart ? or is it just to organize better?

so at the moment any user of the map will overwrite the pin file with its spec and max entries,
while in reality we always have one user that creates the map (owner) and other users that
expect the map to exist and want just to open and use it

it has also another aspect where you need all the users of the map to match the same spec
and same max entries value which is not convenient from configuration point of view, because
all users must have access to the spec and max entries

having the owner and other user force the config duty only to owner and other users just use the map

I'm not really happy with the 'other' naming, so any ideas are welcome ;-)

Hmm others could be fine, or "non-owners"?, but those are certainly better than "users" ? or go with creators and users? ...

hm, right.. maybe MapBuilder(..) for owner and MapUser(..) (instead MapOther) for other users.. with:

also perhaps the MapOwner should be just bool.. iota is perhaps too much

let's see.. thanks a lot for review

mtardy · 2024-09-26T10:41:41Z

This could be beneficial for accounting which sensor/progs/policies are responsible for BPF map memory use, cc @kkourt

tixxdz

LGTM, minor suggestions with one log message that should be fixed. Thank you!

tixxdz · 2024-09-27T09:17:22Z

pkg/sensors/program/map.go

 	// Try to open the pinPath and if it exist use the previously
 	// pinned map otherwise pin the map and next user will find
 	// it here.
-	if _, err := os.Stat(pinPath); err == nil {
+	if _, err = os.Stat(pinPath); err == nil {
 		m, err := ebpf.LoadPinnedMap(pinPath, nil)
 		if err != nil {
 			return nil, fmt.Errorf("loading pinned map from path '%s' failed: %w", pinPath, err)


Does it make sense to re-create here if we have create true?

hm, so this leg is when there is already an existing pin and we check if the pin is compatible with what we want to create, if it is, we are done, if not, we remove the pin and create new one

tixxdz · 2024-09-27T09:18:48Z

pkg/sensors/program/map.go

@@ -260,12 +261,23 @@ func LoadOrCreatePinnedMap(pinPath string, mapSpec *ebpf.MapSpec) (*ebpf.Map, er
 				"map-name": mapSpec.Name,
 			}).Warn("tetragon, incompatible map found: will delete and recreate")


Maybe this delete and recreate warn should be moved inside create true?

good catch! will move, thanks

tixxdz · 2024-09-27T09:20:53Z

pkg/sensors/program/map.go

+		return m, nil
+	}
+	if create {
+		return createPinnedMap(pinPath, mapSpec)


Could move this create just after the state(pinPath) failure , and have the rest outside of if , just for readability up to you

hm, so it is already in the place after the stat failure.. not sure what you mean

Display pin path instead of the name, before: time="2024-09-21T21:09:35Z" level=info msg="map was unloaded" map=m1 pin=m1 time="2024-09-21T21:09:35Z" level=info msg="map was unloaded" map=m2 pin=m2 after: time="2024-09-21T21:10:34Z" level=info msg="map was unloaded" map=m1 pin=m1 time="2024-09-21T21:10:34Z" level=info msg="map was unloaded" map=m2 pin=policy/m2 Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Adding GetDefaultSensorsWithBase helper function to allow overloading of the base sensor. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Introduce map owner field in Map object to state if the object is the real owner of the map. Basically the map object can be now either owner of the map or can just be its user. The ownership will matter when loading the map in following changes. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Let's distinguish the map that 'owns' the pin and the 'user' map user that's just using it. The owner map gets to set (and overwrite if needed) pined map spec and its max entries setup. The user map just follows what is set in existing pinned map and fails if it's expecting anything else. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Adding MapUser* interface to create map that you don't own. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Adding test for pinned maps in multiple sensors just to cover common use case of using same maps cross sensors. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Adding tests for 'user' maps to cover following scenarios: - user map gets properly created from pinned map - user map fails to be loaded if the pinned map is missing - user map fails to be loaded if the pinned map has different max entries setup - user map fails to be loaded if the pinned map is not compatible Signed-off-by: Jiri Olsa <jolsa@kernel.org>

kkourt

🚀

kkourt · 2024-09-30T05:56:59Z

pkg/sensors/program/map.go

+//
+//  Each map declares the ownership of the map. The map can be either
+//  owner of the map (via MapBuilder* helpers) or as an user (MapUser*
+//  helpers.


Thanks for adding the comments!

olsajiri added the release-note/minor This PR introduces a minor user-visible change label Sep 22, 2024

olsajiri force-pushed the pr/olsajiri/map_fixes branch 4 times, most recently from 9959201 to e6b9e5b Compare September 24, 2024 08:22

olsajiri force-pushed the pr/olsajiri/map_fixes branch from e6b9e5b to 9afe055 Compare September 24, 2024 08:29

olsajiri changed the title ~~Pr/olsajiri/map fixes~~ tetragon: Add map ownership Sep 24, 2024

olsajiri marked this pull request as ready for review September 24, 2024 12:56

olsajiri requested a review from a team as a code owner September 24, 2024 12:56

olsajiri requested review from jrfastab and kkourt September 24, 2024 12:56

olsajiri force-pushed the pr/olsajiri/map_fixes branch from 9afe055 to f98ce9d Compare September 25, 2024 07:51

olsajiri force-pushed the pr/olsajiri/map_fixes branch 2 times, most recently from ddc3888 to f5e02e3 Compare September 26, 2024 18:54

tixxdz approved these changes Sep 27, 2024

View reviewed changes

olsajiri added 7 commits September 27, 2024 14:59

tetragon: Add GetDefaultSensorsWithBase function

c8d476d

Adding GetDefaultSensorsWithBase helper function to allow overloading of the base sensor. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

tetragon: Add MapUser* interface

7822f97

Adding MapUser* interface to create map that you don't own. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

tetragon: Add test for pinned maps in multiple sensors

3d639d5

Adding test for pinned maps in multiple sensors just to cover common use case of using same maps cross sensors. Signed-off-by: Jiri Olsa <jolsa@kernel.org>

olsajiri force-pushed the pr/olsajiri/map_fixes branch from f5e02e3 to 47b5d10 Compare September 27, 2024 15:17

kkourt approved these changes Sep 30, 2024

View reviewed changes

olsajiri merged commit 19ba896 into main Sep 30, 2024
44 checks passed

olsajiri deleted the pr/olsajiri/map_fixes branch September 30, 2024 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tetragon: Add map ownership #2945

tetragon: Add map ownership #2945

olsajiri commented Sep 22, 2024 •

edited

Loading

netlify bot commented Sep 24, 2024 •

edited

Loading

tixxdz commented Sep 25, 2024

olsajiri commented Sep 25, 2024

mtardy commented Sep 26, 2024

tixxdz left a comment

tixxdz Sep 27, 2024

olsajiri Sep 27, 2024

tixxdz Sep 27, 2024

olsajiri Sep 27, 2024

tixxdz Sep 27, 2024

olsajiri Sep 27, 2024

kkourt left a comment

kkourt Sep 30, 2024

		@@ -260,12 +261,23 @@ func LoadOrCreatePinnedMap(pinPath string, mapSpec ebpf.MapSpec) (ebpf.Map, er
		"map-name": mapSpec.Name,
		}).Warn("tetragon, incompatible map found: will delete and recreate")

tetragon: Add map ownership #2945

tetragon: Add map ownership #2945

Conversation

olsajiri commented Sep 22, 2024 • edited Loading

netlify bot commented Sep 24, 2024 • edited Loading

✅ Deploy Preview for tetragon ready!

tixxdz commented Sep 25, 2024

olsajiri commented Sep 25, 2024

mtardy commented Sep 26, 2024

tixxdz left a comment

Choose a reason for hiding this comment

tixxdz Sep 27, 2024

Choose a reason for hiding this comment

olsajiri Sep 27, 2024

Choose a reason for hiding this comment

tixxdz Sep 27, 2024

Choose a reason for hiding this comment

olsajiri Sep 27, 2024

Choose a reason for hiding this comment

tixxdz Sep 27, 2024

Choose a reason for hiding this comment

olsajiri Sep 27, 2024

Choose a reason for hiding this comment

kkourt left a comment

Choose a reason for hiding this comment

kkourt Sep 30, 2024

Choose a reason for hiding this comment

olsajiri commented Sep 22, 2024 •

edited

Loading

netlify bot commented Sep 24, 2024 •

edited

Loading