eBPF instrumentation manager #1776

RonFed · 2024-11-18T08:51:16Z

This PR is a follow-up to #1645.
Adding the new Manager which will eventually replace the Director.
The new design has the following key features and improvements:

Use the new runtime-detector module to trigger instrument/un-instrument events. This replaces the current approach which relies on a Pod reconciler. The pod reconciler approach main disadvantage is in scenarios of multiple containers in the same Pod and multiple processes in the same container. Changing the trigger to being process creation will allow us to guarantee we won't miss a requested instrumentation.
note: the runtime-detector is configured to filter process events and will only pass events according to its configuration.
Event loop design. The current director has a lot of fixed and potential race conditions due to the concurrent nature of processes creating/exiting and Pod events from the reconciler. The new Manager does not have locks and uses an internal event loop.
Configuration updates are triggered by the InstrumentationConfig reconciler (same as before) - those updates will be handled in the event loop.
The Factory interface is refactored and a Settings option can be expanded in the future to add more initial configuration options.
The Instrumentation interface is introduced and will replace OtelEbpfSdk.
Update go.opentelemetry.io/auto to v0.18.0-alpha.

This change will currently only apply for OSS Go instrumentation.

odiglet/go.mod

blumamir

Graet job!!!

Looks good, added many comments but most are nits and style

odiglet/cmd/main.go

blumamir · 2024-11-22T06:26:20Z

odiglet/pkg/ebpf/manager.go

+func (m *Manager) UpdateConfig(ctx context.Context, workloadKey types.NamespacedName, config *odigosv1.InstrumentationConfig) error {
+	// send a config update event for the specified workload
+	select {
+	case m.configUpdates <- configUpdate{workloadKey: workloadKey, config: config}:


This can block quite easily. should we give some capacity to the channel so we can handle bursts without blocking the caller to UpdateConfig?

configUpdates: make(chan configUpdate)

I agree we should probably make this channel buffered, not sure about what the size should be?

blumamir · 2024-11-22T06:38:38Z

odiglet/pkg/kube/instrumentation_ebpf/instrumentationconfig.go

@@ -53,5 +55,12 @@ func (i *InstrumentationConfigReconciler) Reconcile(ctx context.Context, req ctr
 		}
 	}

+	if i.OnUpdate != nil {
+		err = i.OnUpdate(ctx, types.NamespacedName{Namespace: req.Namespace, Name: workloadName}, instrumentationConfig)


thoughts - maybe instead of passing a function here and potentially blocking this part for some time, WDYT about passing the channel directly and having this code pushing the event to the channel.

This make it clear that an async operation is about to take place, and let the caller handle cancelation and errors.

For example: the error returned from OnUpdate is only the error for sending to the channel, but future readers might expect the update result itself to be returned here which can lead to errors.

Having it as a channel, communicates how it is used, what to expect, and leave more flexibility to the caller.

Overall I agree, the only disadvantage to this is how we handle closing the channel.
The idiomatic way is for the sender to close the channel, but the reconciler itself is not aware if the manager is stopped or not.

odiglet/pkg/kube/instrumentation_ebpf/pods.go

odiglet/pkg/ebpf/manager.go

blumamir · 2024-11-22T08:15:56Z

odiglet/pkg/ebpf/manager.go

+	return containerName, ok
+}
+
+func (m *Manager) languageSdk(pod *corev1.Pod, containerName string) (common.ProgrammingLanguage, common.OtelSdk, error) {


suggestion for alternative name

Suggested change

func (m *Manager) languageSdk(pod *corev1.Pod, containerName string) (common.ProgrammingLanguage, common.OtelSdk, error) {

func (m *Manager) getContainerDeviceInfo(pod *corev1.Pod, containerName string) (common.ProgrammingLanguage, common.OtelSdk, error) {

Moved to k8sutils, and renamed LanguageSdkFromPodContainer

odiglet/pkg/ebpf/manager.go

blumamir · 2024-11-22T08:22:03Z

odiglet/pkg/ebpf/manager.go

+	}
+
+	// Fetch initial config based on the InstrumentationConfig CR
+	sdkConfig := m.instrumentationSDKConfig(ctx, workload, lang, types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name})


sdkConfig can be nil here, we should check for nil everytime we use it

It is used for the initial config. I added a check in the go.go file to check for that, and use the default config if nil is provided.

edeNFed · 2024-11-23T16:48:17Z

odiglet/pkg/ebpf/manager.go

+		case <-runLoopCtx.Done():
+			m.Logger.Info("stopping Odiglet instrumentation manager")
+			for pid, details := range m.DetailsByPid {
+				err := details.Inst.Close(ctx)


nit: nil check

see #1776 (comment)

odiglet/pkg/ebpf/manager.go

edeNFed · 2024-11-23T16:56:21Z

odiglet/pkg/ebpf/manager.go

+	m.stop = stop
+
+	// main event loop for handling instrumentations
+	for {


I am not sure how or if we should handle it, but I think this design has a noisy neighbor issue:
Imaging a Pod with the following bash script:

while true; do echo "test" done

This will trigger tons of ProcEvents and will cause starvation for all other pods / configuration changes.
What do you think? cc @blumamir

aka Odiglet DDos

edeNFed · 2024-11-23T16:59:18Z

odiglet/pkg/ebpf/manager.go

+
+	m.Logger.Info("cleaning instrumentation resources", "pid", pid)
+
+	err := details.Inst.Close(ctx)


same nil check, maybe we should move the Close function to the details instance instead of closing the individual fields?
This will probably never be nil but still afraid of panics

I created the startTrackInstrumentation which is the only place where instrumentationDetails is created, since this is an internal logic to this package, and not dependant on user input I think if the inst is nill - it is a result of some logical error on our side which should never happen.

odiglet/pkg/ebpf/manager.go

…directors map is provided

…e provided

edeNFed

LGTM

blumamir · 2024-11-26T15:50:02Z

odiglet/pkg/ebpf/manager.go

+
+	// active instrumentations by workload, and aggregated by pid
+	// this map is not concurrent safe, so it should be accessed only from the main event loop
+	detailsByWorkload map[types.NamespacedName]map[int]*instrumentationDetails


add workload kind and language here and simplify the usage

RonFed added 8 commits November 5, 2024 18:02

[WIP] new instrumentations manager POC

b947e22

Merge branch 'main' into instrumentation_manager

e5e311b

WIP

8239145

WIP

6ff8cc7

Merge branch 'main' into instrumentation_manager

9a90a2d

Add instrumentation instance updates and deletion to new manager

f4887a4

Handle configuration updates and intial configuration

de9e868

Add documentation for the new Factory and Instrumentation interfaces

79f2469

RonFed requested review from tamirdavid1, damemi, edeNFed, blumamir and david336362 and removed request for tamirdavid1 November 18, 2024 08:51

RonFed marked this pull request as ready for review November 18, 2024 09:17

RonFed changed the title ~~[WIP] eBPF instrumentation manager~~ eBPF instrumentation manager Nov 18, 2024

RonFed commented Nov 18, 2024

View reviewed changes

odiglet/go.mod Outdated Show resolved Hide resolved

blumamir reviewed Nov 22, 2024

View reviewed changes

remove un-used functions

d826e97

edeNFed reviewed Nov 23, 2024

View reviewed changes

odiglet/pkg/ebpf/manager.go Outdated Show resolved Hide resolved

edeNFed reviewed Nov 23, 2024

View reviewed changes

odiglet/pkg/ebpf/manager.go Show resolved Hide resolved

RonFed added 5 commits November 24, 2024 09:52

Mark director as seprecated, only register the Pod reconciler if the …

d9178c8

…directors map is provided

Merge branch 'main' into instrumentation_manager

b22a1ab

Remove ddouble error loggging

0f2ce5a

move instrumentationDetails to manager

dfd0b57

add comments about settings

748177a

RonFed added 16 commits November 24, 2024 16:30

Make proc events channel read only for manager

4afb623

Make manger fields private

7a5c521

improve log message

964288c

move process detected log message to debug level

7bbcf16

Make detector channel write-only

76e918f

Pass context to podFromProcEvent

081c35c

Add errRequiredEnvVarNotFound

549e93b

Add a map for instrumentations by workload, used for config updates

8987c70

Merge branch 'main' into instrumentation_manager

46bb2a3

improve applyInstrumentationConfigurationForSDK readability

8e9f030

move function from manager to k8sutils

64ce1b4

Add PodWorkloadObjectOrError wrapper

ed29146

Check nil SDK config for Go instrumenation and use default one if non…

197f35b

…e provided

Added comments and better nil handling

159488d

Merge branch 'main' into instrumentation_manager

7fd76bc

Update go.opentelemetry.io/auto to v0.18.0-alpha

fb167f8

edeNFed approved these changes Nov 26, 2024

View reviewed changes

blumamir reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eBPF instrumentation manager #1776

eBPF instrumentation manager #1776

RonFed commented Nov 18, 2024 •

edited

Loading

blumamir left a comment

blumamir Nov 22, 2024

RonFed Nov 25, 2024 •

edited

Loading

blumamir Nov 22, 2024

RonFed Nov 23, 2024

blumamir Nov 22, 2024

RonFed Nov 25, 2024

blumamir Nov 22, 2024

RonFed Nov 25, 2024

edeNFed Nov 23, 2024

RonFed Nov 25, 2024

edeNFed Nov 23, 2024

edeNFed Nov 23, 2024

edeNFed Nov 23, 2024

RonFed Nov 25, 2024

edeNFed left a comment

blumamir Nov 26, 2024

	func (m Manager) languageSdk(pod corev1.Pod, containerName string) (common.ProgrammingLanguage, common.OtelSdk, error) {
	func (m Manager) getContainerDeviceInfo(pod corev1.Pod, containerName string) (common.ProgrammingLanguage, common.OtelSdk, error) {


		m.Logger.Info("cleaning instrumentation resources", "pid", pid)

		err := details.Inst.Close(ctx)

eBPF instrumentation manager #1776

Are you sure you want to change the base?

eBPF instrumentation manager #1776

Conversation

RonFed commented Nov 18, 2024 • edited Loading

blumamir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonFed Nov 25, 2024 • edited Loading

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

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

edeNFed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonFed commented Nov 18, 2024 •

edited

Loading

RonFed Nov 25, 2024 •

edited

Loading