Feature/refactor container watcher by afek854 · Pull Request #591 · kubescape/node-agent

afek854 · 2025-07-15T13:42:14Z

Overview

Signed-off-by: Afek <afekb@armosec.io>

Signed-off-by: Afek Berger <afekb@armosec.io>

amitschendel · 2025-07-23T07:22:22Z

Remove this file

amitschendel · 2025-07-23T07:24:58Z

+// TracerFactoryInterface defines the interface for creating tracers
+type TracerFactoryInterface interface {
+	CreateAllTracers(manager TracerRegistrer)
+	StartThirdPartyTracers(ctx context.Context) error


Why does a factory has start/stop methods?

amitschendel · 2025-07-23T07:26:29Z

+		cw.sbomManager.ContainerCallback,
+		cw.dnsManager.ContainerCallback,
+		cw.networkStreamClient.ContainerCallback,
+		cw.containerProcessTree.ContainerCallback,


Don't you want to move the process tree callback to be before the other managers so it will reduce races?

amitschendel · 2025-07-23T07:27:39Z

+
+type OrderedEventQueue struct {
+	maxBufferSize int
+


Why do you have a new line between every field?

amitschendel · 2025-07-23T07:30:48Z

+func NewTracerManager(cfg config.Config, tracerFactory containerwatcherroot.TracerFactoryInterface) *TracerManager {
+	return &TracerManager{
+		cfg:           cfg,
+		tracers:       make(map[utils.EventType]containerwatcherroot.TracerInterface),


containerwatcherroot -> containerwatcher

amitschendel · 2025-07-23T07:33:22Z

+		tracerCollection:    tracerCollection,
+		containerSelector:   containerSelector,
+		eventCallback:       eventCallback,
+		procfsFeeder:        feeder.NewProcfsFeeder(cfg.ProcfsScanInterval), // Scan every 5 seconds


Remove the comment

amitschendel · 2025-07-23T07:37:45Z

+		path = event.Args[0]
+	}
+
+	if path == "" {


Do you use the path? it should be passed to the container profile manager if I recall.

node-agent/pkg/containerwatcher/v1/container_watcher.go

Line 235 in c451260

if path == "" {

amitschendel · 2025-07-23T07:41:08Z

+    // Get command name
+    bpf_get_current_comm(&event->comm, sizeof(event->comm));
+
+    // Get executable path


amitschendel · 2025-07-23T07:42:07Z

+}
+
+// This gadget is used to trace the sched_process_exit tracepoint.
+SEC("raw_tracepoint/sched_process_exit")


Did you test this exists in all archs (amd64/arm64)?
What happens if it doesn't load? crash the agent? do we have a fallback for this in case it can't load?

Also checked on kernel 5.14

amitschendel · 2025-07-23T07:44:25Z

+}
+
+// This gadget is used to trace the sched_process_fork tracepoint.
+SEC("raw_tracepoint/sched_process_fork")


Same comments as the exist tracer eBPF.

Why? in fork we need the attributes

Signed-off-by: Afek Berger <afekb@armosec.io>

amitschendel · 2025-07-24T06:38:57Z

@@ -0,0 +1,158 @@
+package feeder


Change according to what we talked f2f.

Signed-off-by: Afek Berger <afekb@armosec.io>

…store 5s alerting cadence The gadget's own internal fetch schedule is now disabled entirely (map-fetch-interval/count both "0"); every fetch, periodic or not, goes through SyscallTracer.Peek. A single background loop in Start calls Peek on syscallPollInterval (default 5s, was 30s) for live event delivery to every consumer (profile-building and RuleManager's real-time syscall alerting alike), and ContainerProfileManager calls Peek once more, out of that schedule, right before a container's final forced save. This restores the pre-PR-#591 architecture's actual property that mattered: RuleManager's real-time alerting ran on its own dedicated schedule (a 5s ticker per container) fully decoupled from ContainerProfileManager's save-driven Peek() calls. The destructive batch read the current OCI gadget uses was never actually why a single shared 30s schedule was needed; there was simply one internal ticker serving every consumer at the same cadence. Driving everything through one explicit Peek call, and making that call on the original 5s cadence, is now also cheaper than the pre-#591 design: one global fetch drains and dispatches every currently traced container in a batch, rather than one ticker per container.

…store 5s alerting cadence The gadget's own internal fetch schedule is now disabled entirely (map-fetch-interval/count both "0"); every fetch, periodic or not, goes through SyscallTracer.Peek. A single background loop in Start calls Peek on syscallPollInterval (default 5s, was 30s) for live event delivery to every consumer (profile-building and RuleManager's real-time syscall alerting alike), and ContainerProfileManager calls Peek once more, out of that schedule, right before a container's final forced save. This restores the pre-PR-#591 architecture's actual property that mattered: RuleManager's real-time alerting ran on its own dedicated schedule (a 5s ticker per container) fully decoupled from ContainerProfileManager's save-driven Peek() calls. The destructive batch read the current OCI gadget uses was never actually why a single shared 30s schedule was needed; there was simply one internal ticker serving every consumer at the same cadence. Driving everything through one explicit Peek call, and making that call on the original 5s cadence, is now also cheaper than the pre-#591 design: one global fetch drains and dispatches every currently traced container in a batch, rather than one ticker per container. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…ation (#924) * fix(containerprofile): reduce syscall loss window on container termination The seccomp syscall tracer only fetches the advise_seccomp eBPF map on a fixed 30s poll; a container terminating between polls loses whatever it executed since the last fetch once its profile is saved and its data is removed. Make the poll interval configurable (syscallPollInterval, default 2s, down from a hardcoded 30s) and wait one bounded poll interval before the final forced profile save on termination, so the tracer's next poll cycle has a chance to land first. Fixes #922 Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(containerprofile): flush syscalls on demand instead of shrinking the poll interval Bumps the inspektor-gadget fork to matthyx/inspektor-gadget#12, which adds ebpfoperator.TriggerManualMapFetch: an on-demand trigger for the eBPF map iterator that the periodic syscall poll relies on. ContainerProfileManager now calls this (via SyscallTracer.Peek, wired through the new SetSyscallFlusher hook) right before a container's final forced profile save on termination, instead of guessing how long to wait for the next scheduled poll. This replaces the previous mitigation (shrinking syscallPollInterval to 2s and sleeping up to that long on every termination) with an actual fix: the poll interval reverts to its original 30s default since it no longer needs to be short for correctness, and the wait before saving is now a fixed 500ms settle delay for the event pipeline, only paid when a flush was actually requested. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(containerprofile): drive all syscall map fetches through Peek, restore 5s alerting cadence The gadget's own internal fetch schedule is now disabled entirely (map-fetch-interval/count both "0"); every fetch, periodic or not, goes through SyscallTracer.Peek. A single background loop in Start calls Peek on syscallPollInterval (default 5s, was 30s) for live event delivery to every consumer (profile-building and RuleManager's real-time syscall alerting alike), and ContainerProfileManager calls Peek once more, out of that schedule, right before a container's final forced save. This restores the pre-PR-#591 architecture's actual property that mattered: RuleManager's real-time alerting ran on its own dedicated schedule (a 5s ticker per container) fully decoupled from ContainerProfileManager's save-driven Peek() calls. The destructive batch read the current OCI gadget uses was never actually why a single shared 30s schedule was needed; there was simply one internal ticker serving every consumer at the same cadence. Driving everything through one explicit Peek call, and making that call on the original 5s cadence, is now also cheaper than the pre-#591 design: one global fetch drains and dispatches every currently traced container in a batch, rather than one ticker per container. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * feat(containerprofile): batch syscall reporting, bypass generic pipeline SyscallTracer.callback decodes one eBPF fetch into a batch of syscall names per container, but was still emitting one SyscallEventType event per syscall through the generic per-event pipeline (AddEventDirect, dedup-key computation, handler dispatch) for a data source that isn't actually a discrete event stream - it's a periodic snapshot of a persistent per-mntns bitmap. That per-item overhead scales with how many distinct syscalls a container has executed since its last fetch, worst right after a container starts. RuleManager's rule matching and the generic dedup cache both key off a single event.syscall value, so they still need one event per syscall. ContainerProfileManager has no such constraint - it only folds each syscall into a per-container set - so it now gets the whole batch directly via the new ReportSyscalls(containerID, []string) method, bypassing the queue/dedup/dispatch pipeline entirely for this consumer. RuleManager and metrics are unaffected, still receiving one event per syscall exactly as before. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(containerprofile): address review — flusher race, blocking dispatch, empty containerID Fixes three issues from review on PR #924 (pullrequestreview-5027546778) plus the CI failures they caused: - ContainerProfileManager.syscallFlusher was a plain func() field written once by SetSyscallFlusher (from TracerFactory, during startup wiring) and read by flushAndSettle from concurrently-running per-container monitorContainer goroutines - an unsynchronized data race that could silently skip the flush at exactly the highest-container-churn moment there is (startup/restart). Now stored via atomic.Pointer[func()]. - SyscallTracer.callback called ContainerProfileManager.ReportSyscalls synchronously and inline, on the single goroutine the gadget uses to process every currently-traced container's fetch results. ReportSyscalls can itself block (withContainer sends on a container's bounded SyncChannel while holding its lock, on a profile-size-split). Previously this same call went through the generic queue and one of many pooled workers, so one slow container never affected any other. Now dispatched on its own goroutine so it can't stall the shared fetch-processing path. - The seccomp map covers every mount namespace on the node, including ones never resolved to a container (host processes, etc). The generic pipeline used to drop those via EventHandlerFactory.ProcessEvent's empty-ContainerID check before they reached ReportSyscall; the direct call bypasses that check, so every such row was logging "invalid empty containerID" and wasting a container-map lookup, every tick - visible as hundreds of these errors in the failing CI runs (Test_02_AllAlertsFromMaliciousApp, Test_23_RuleCooldownTest, Test_28_UserDefinedNetworkNeighborhood). callback now filters these out itself before decoding or reporting. Also: shared config.DefaultSyscallPollInterval constant instead of duplicating the 5s literal in both packages, and decodeSyscalls pre-sizes its slice since the upper bound is already known. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * docs: correct flushAndSettle description to match the direct-call path Leftover from before the review-fix commit: ContainerProfileManager no longer goes through the generic event queue/worker pool for syscalls, so the 500ms wait description was stale (CodeRabbit review comment on PR #924). Docs-exempt: documentation-only correction, no behavioral change Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> --------- Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

…ation (kubescape#924) * fix(containerprofile): reduce syscall loss window on container termination The seccomp syscall tracer only fetches the advise_seccomp eBPF map on a fixed 30s poll; a container terminating between polls loses whatever it executed since the last fetch once its profile is saved and its data is removed. Make the poll interval configurable (syscallPollInterval, default 2s, down from a hardcoded 30s) and wait one bounded poll interval before the final forced profile save on termination, so the tracer's next poll cycle has a chance to land first. Fixes kubescape#922 Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(containerprofile): flush syscalls on demand instead of shrinking the poll interval Bumps the inspektor-gadget fork to matthyx/inspektor-gadget#12, which adds ebpfoperator.TriggerManualMapFetch: an on-demand trigger for the eBPF map iterator that the periodic syscall poll relies on. ContainerProfileManager now calls this (via SyscallTracer.Peek, wired through the new SetSyscallFlusher hook) right before a container's final forced profile save on termination, instead of guessing how long to wait for the next scheduled poll. This replaces the previous mitigation (shrinking syscallPollInterval to 2s and sleeping up to that long on every termination) with an actual fix: the poll interval reverts to its original 30s default since it no longer needs to be short for correctness, and the wait before saving is now a fixed 500ms settle delay for the event pipeline, only paid when a flush was actually requested. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(containerprofile): drive all syscall map fetches through Peek, restore 5s alerting cadence The gadget's own internal fetch schedule is now disabled entirely (map-fetch-interval/count both "0"); every fetch, periodic or not, goes through SyscallTracer.Peek. A single background loop in Start calls Peek on syscallPollInterval (default 5s, was 30s) for live event delivery to every consumer (profile-building and RuleManager's real-time syscall alerting alike), and ContainerProfileManager calls Peek once more, out of that schedule, right before a container's final forced save. This restores the pre-PR-kubescape#591 architecture's actual property that mattered: RuleManager's real-time alerting ran on its own dedicated schedule (a 5s ticker per container) fully decoupled from ContainerProfileManager's save-driven Peek() calls. The destructive batch read the current OCI gadget uses was never actually why a single shared 30s schedule was needed; there was simply one internal ticker serving every consumer at the same cadence. Driving everything through one explicit Peek call, and making that call on the original 5s cadence, is now also cheaper than the pre-kubescape#591 design: one global fetch drains and dispatches every currently traced container in a batch, rather than one ticker per container. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * feat(containerprofile): batch syscall reporting, bypass generic pipeline SyscallTracer.callback decodes one eBPF fetch into a batch of syscall names per container, but was still emitting one SyscallEventType event per syscall through the generic per-event pipeline (AddEventDirect, dedup-key computation, handler dispatch) for a data source that isn't actually a discrete event stream - it's a periodic snapshot of a persistent per-mntns bitmap. That per-item overhead scales with how many distinct syscalls a container has executed since its last fetch, worst right after a container starts. RuleManager's rule matching and the generic dedup cache both key off a single event.syscall value, so they still need one event per syscall. ContainerProfileManager has no such constraint - it only folds each syscall into a per-container set - so it now gets the whole batch directly via the new ReportSyscalls(containerID, []string) method, bypassing the queue/dedup/dispatch pipeline entirely for this consumer. RuleManager and metrics are unaffected, still receiving one event per syscall exactly as before. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * fix(containerprofile): address review — flusher race, blocking dispatch, empty containerID Fixes three issues from review on PR kubescape#924 (pullrequestreview-5027546778) plus the CI failures they caused: - ContainerProfileManager.syscallFlusher was a plain func() field written once by SetSyscallFlusher (from TracerFactory, during startup wiring) and read by flushAndSettle from concurrently-running per-container monitorContainer goroutines - an unsynchronized data race that could silently skip the flush at exactly the highest-container-churn moment there is (startup/restart). Now stored via atomic.Pointer[func()]. - SyscallTracer.callback called ContainerProfileManager.ReportSyscalls synchronously and inline, on the single goroutine the gadget uses to process every currently-traced container's fetch results. ReportSyscalls can itself block (withContainer sends on a container's bounded SyncChannel while holding its lock, on a profile-size-split). Previously this same call went through the generic queue and one of many pooled workers, so one slow container never affected any other. Now dispatched on its own goroutine so it can't stall the shared fetch-processing path. - The seccomp map covers every mount namespace on the node, including ones never resolved to a container (host processes, etc). The generic pipeline used to drop those via EventHandlerFactory.ProcessEvent's empty-ContainerID check before they reached ReportSyscall; the direct call bypasses that check, so every such row was logging "invalid empty containerID" and wasting a container-map lookup, every tick - visible as hundreds of these errors in the failing CI runs (Test_02_AllAlertsFromMaliciousApp, Test_23_RuleCooldownTest, Test_28_UserDefinedNetworkNeighborhood). callback now filters these out itself before decoding or reporting. Also: shared config.DefaultSyscallPollInterval constant instead of duplicating the 5s literal in both packages, and decodeSyscalls pre-sizes its slice since the upper bound is already known. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> * docs: correct flushAndSettle description to match the direct-call path Leftover from before the review-fix commit: ContainerProfileManager no longer goes through the generic event queue/worker pool for syscalls, so the 500ms wait description was stale (CodeRabbit review comment on PR kubescape#924). Docs-exempt: documentation-only correction, no behavioral change Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> --------- Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

afek854 added 30 commits June 24, 2025 15:21

started process tree refactor process

78d658f

Signed-off-by: Afek <afekb@armosec.io>

completed process tree creator

e91c6e5

Signed-off-by: Afek <afekb@armosec.io>

added tests

bde54eb

Signed-off-by: Afek <afekb@armosec.io>

added proc fs feeder

466d609

Signed-off-by: Afek <afekb@armosec.io>

added exit process track

8651d54

Signed-off-by: Afek <afekb@armosec.io>

fixed tests

e51af88

Signed-off-by: Afek <afekb@armosec.io>

added process tree manager and container to main

7924e40

Signed-off-by: Afek <afekb@armosec.io>

added ebpf event feeder

b9668cb

Signed-off-by: Afek <afekb@armosec.io>

added initial process manager to main

2497c5f

Signed-off-by: Afek <afekb@armosec.io>

added logs and exec to report to feeder

e5d807e

Signed-off-by: Afek <afekb@armosec.io>

replace old process tree

facbe0d

Signed-off-by: Afek <afekb@armosec.io>

WIP: fixed lock

7d8778d

Signed-off-by: Afek <afekb@armosec.io>

fixed tests

affb441

Signed-off-by: Afek <afekb@armosec.io>

fixed container tree find

2625ef2

Signed-off-by: Afek <afekb@armosec.io>

handle parent bug

a53b6ed

Signed-off-by: Afek <afekb@armosec.io>

ensure efficiency and more test cases

f5550b4

Signed-off-by: Afek Berger <afekb@armosec.io>

return the correct node, use the correct shim pid

5794655

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: fork tracer

d0b7108

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: fork tracer new hook

5a98196

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: dedup fork & exec

88f5653

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: Handle runc ppid changes

159630f

Signed-off-by: Afek Berger <afekb@armosec.io>

do retry until tree ready

a90054b

Signed-off-by: Afek Berger <afekb@armosec.io>

added reparenting logic

0cd6c4b

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: Added exit strategy and tracer

9e1bf7f

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: remove set creator

4aef8e3

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: added logs

f6934be

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: added strategies

c12f495

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: optimize parenting and inserting

872faaa

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: optimize walking and updating ppid

ca38ec3

Signed-off-by: Afek Berger <afekb@armosec.io>

WIP: Optimize trees

2c5e8b9

Signed-off-by: Afek Berger <afekb@armosec.io>

afek854 added 3 commits July 22, 2025 14:03

fixed ordered event queue

6b27779

Signed-off-by: Afek Berger <afekb@armosec.io>

fixed timestamp

297e339

Signed-off-by: Afek Berger <afekb@armosec.io>

fixed worker pool

69547d2

Signed-off-by: Afek Berger <afekb@armosec.io>

afek854 force-pushed the feature/refactor_container_watcher branch from b03389a to 69547d2 Compare July 22, 2025 12:08

afek854 added 5 commits July 22, 2025 15:21

removed debug logs

3e7dc77

Signed-off-by: Afek Berger <afekb@armosec.io>

added tree depth

0a4f1b3

Signed-off-by: Afek Berger <afekb@armosec.io>

fix test

42f1d16

Signed-off-by: Afek Berger <afekb@armosec.io>

use cache only for not update events

8c7e923

Signed-off-by: Afek Berger <afekb@armosec.io>

removed types without manager

9b09400

Signed-off-by: Afek Berger <afekb@armosec.io>

afek854 force-pushed the feature/refactor_container_watcher branch from 47ec528 to ddf37b5 Compare July 22, 2025 14:12

renaming due to cr

f7a6f7b

Signed-off-by: Afek Berger <afekb@armosec.io>

afek854 force-pushed the feature/refactor_container_watcher branch from ddf37b5 to f7a6f7b Compare July 23, 2025 07:23

amitschendel requested changes Jul 23, 2025

View reviewed changes

afek854 added 2 commits July 23, 2025 14:02

fixed traces factory and naming

d08cbf6

Signed-off-by: Afek Berger <afekb@armosec.io>

fixed fork & exit tracers

b45333a

Signed-off-by: Afek Berger <afekb@armosec.io>

amitschendel requested changes Jul 24, 2025

View reviewed changes

afek854 force-pushed the feature/refactor_container_watcher branch from 3c704bc to 4f30142 Compare July 24, 2025 07:01

removed event feeder

74b9e97

Signed-off-by: Afek Berger <afekb@armosec.io>

afek854 force-pushed the feature/refactor_container_watcher branch from 4f30142 to 74b9e97 Compare July 24, 2025 07:02

amitschendel added the release Create release label Jul 24, 2025

amitschendel self-requested a review July 24, 2025 07:20

amitschendel approved these changes Jul 24, 2025

View reviewed changes

amitschendel merged commit 531bac3 into main Jul 24, 2025

amitschendel deleted the feature/refactor_container_watcher branch July 24, 2025 07:21

matthyx mentioned this pull request Aug 24, 2026

fix(containerprofile): syscalls missing from profiles due to asynchronous 30s polling and missing on-demand Peek #922

Closed

matthyx mentioned this pull request Aug 26, 2026

fix(containerprofile): reduce syscall loss window on container termination #924

Merged

Conversation

afek854 commented Jul 15, 2025

Overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afek854 Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

afek854 Jul 23, 2025 •

edited

Loading