From 61354abe6f138d8e9a003ba65efea8106a5b0a92 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Tue, 25 Aug 2026 21:06:38 +0200 Subject: [PATCH 1/6] 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 --- docs/CONFIGURATION.md | 1 + docs/features/syscall-poll-interval.md | 67 +++++++++++++++++++ pkg/config/config.go | 2 + pkg/config/config_test.go | 13 ++-- .../v1/containerprofile_manager_test.go | 20 ++++++ pkg/containerprofilemanager/v1/helpers.go | 19 ++++++ pkg/containerprofilemanager/v1/monitoring.go | 5 ++ pkg/containerwatcher/v2/tracers/syscall.go | 21 +++++- .../v2/tracers/syscall_test.go | 19 ++++++ .../v2/tracers/tracer_factory.go | 1 + 10 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 docs/features/syscall-poll-interval.md diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 1b583378ed..ac0831c78c 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -173,6 +173,7 @@ correlation is an upgrade, not a dependency. | `profilesCacheRefreshRate` | duration | `1m` | Profile cache refresh rate | | `procfsScanInterval` | duration | `30s` | Procfs scan interval | | `procfsPidScanInterval` | duration | `5s` | Per-PID procfs scan interval | +| `syscallPollInterval` | duration | `2s` | How often the seccomp syscall tracer fetches its eBPF map. Also bounds (capped at 5s) how long a container's final profile save waits on termination so the last poll cycle can land. See [docs/features/syscall-poll-interval.md](features/syscall-poll-interval.md). | | `maxDelaySeconds` | int | `30` | Max random delay for jitter | | `maxJitterPercentage` | int | `5` | Max jitter percentage | diff --git a/docs/features/syscall-poll-interval.md b/docs/features/syscall-poll-interval.md new file mode 100644 index 0000000000..c6cf524f53 --- /dev/null +++ b/docs/features/syscall-poll-interval.md @@ -0,0 +1,67 @@ +# Syscall Poll Interval and Termination Grace Period + +Container profiles could end up with 0 or partial syscalls even when the container executed +tens of distinct syscalls (kubescape/node-agent#922). This documents the root cause and the +mitigation. + +## Root cause + +`SyscallTracer` (pkg/containerwatcher/v2/tracers/syscall.go) doesn't read the kernel's +`advise_seccomp` eBPF map on demand. It runs the `advise_seccomp` OCI gadget with a periodic +map-fetch: every poll interval, the gadget operator drains the whole map +(`BPF_MAP_LOOKUP_AND_DELETE_BATCH`) and emits one datasource event per entry, which +`SyscallTracer.callback` decodes and routes to `ContainerProfileManager.ReportSyscall` by +container ID. Until the fix below, that interval was hardcoded to 30 seconds. + +`ContainerProfileManager` (pkg/containerprofilemanager/v1) reacts to container termination +synchronously: `deleteContainer` sends `ContainerHasTerminatedError` on the container's sync +channel, and `monitorContainer` (pkg/containerprofilemanager/v1/monitoring.go) immediately does +a final forced `saveProfile` and hands the ack back. Whatever syscalls the kernel recorded since +the tracer's last poll are still sitting in the eBPF map at that moment — they have not been +fetched yet — and once the profile is saved and the container's entry is removed +(`removeContainerEntry`), that data is unreachable. A container whose lifetime is shorter than +the poll interval could complete with zero syscalls recorded; any container has up to one full +poll interval of syscalls at risk on termination. + +There is no per-container on-demand flush available from the pinned `advise_seccomp` gadget: the +eBPF map iterator (`pkg/operators/ebpf/maps.go` in the `inspektor-gadget` fork) only supports a +fixed interval/count and an optional flush-on-stop that fires when the whole gadget instance +stops — not when one container among many terminates. Re-introducing a true synchronous +`Peek()` (as node-agent v1 had) would require changing that upstream gadget's fetch loop to +support an on-demand trigger, which is out of scope here. + +## Mitigation + +Two changes narrow the race instead, both purely on the node-agent side: + +1. **`syscallPollInterval` config** (default `2s`, was hardcoded `30s`) — passed to the gadget as + `operator.oci.ebpf.map-fetch-interval` (`SyscallTracer.pollInterval()`). Shortening the + interval bounds how much syscall data can be sitting unfetched in the kernel map at any given + moment, from up to 30s down to the configured value. + +2. **Termination grace period** — `ContainerProfileManager.terminationGracePeriod()` + (pkg/containerprofilemanager/v1/helpers.go) sleeps for `min(syscallPollInterval, 5s)` + immediately before the final forced `saveProfile` call on both termination paths + (`ContainerHasTerminatedError` and `ContainerReachedMaxTime` in monitoring.go). This gives the + next poll cycle — which is already running independently of any single container's lifecycle + — a chance to fetch and report the last syscalls before they become unreachable. The 5s cap + protects against a misconfigured (large) `syscallPollInterval` stalling every container + termination by that same amount; `MaxWaitForAck` (30s) has ample headroom above the cap. + +Together these shrink the loss window from "up to 30s, every termination" to "up to +`syscallPollInterval` (default 2s), and only when a poll genuinely didn't fire in that window" — +without touching the gadget's fetch mechanism. + +## Known limitations + +This is a bounded-race mitigation, not a guarantee. A container that terminates and whose final +poll cycle is delayed (e.g. by scheduler contention) beyond the grace period can still lose its +most recent syscalls. Achieving zero-loss capture would require an on-demand, per-container flush +in the underlying gadget (`matthyx/inspektor-gadget`), tracked as a separate follow-up. + +Shortening the poll interval means more frequent map fetches. Each fetch drains the entire +`advise_seccomp` map for every actively-traced container's mount namespace in one batch (not just +one), so the added overhead is one small periodic drain rather than N per-container calls; syscall +events reaching `ContainerProfileManager` are further deduplicated per (mount ns, pid, syscall) +within a 5s window (`dedupTTLSyscall`, pkg/containerwatcher/v2/event_handler_factory.go), which +absorbs most of the added repeat-event volume from polling more often. diff --git a/pkg/config/config.go b/pkg/config/config.go index cec9f41ab6..4aa3abeb32 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -122,6 +122,7 @@ type Config struct { ProfileProjection ProfileProjectionConfig `mapstructure:"profileProjection"` ProfilesCacheRefreshRate time.Duration `mapstructure:"profilesCacheRefreshRate"` StorageRPCBudget time.Duration `mapstructure:"storageRPCBudget"` + SyscallPollInterval time.Duration `mapstructure:"syscallPollInterval"` RuleCoolDown rulecooldown.RuleCooldownConfig `mapstructure:"ruleCooldown"` UpdateDataPeriod time.Duration `mapstructure:"updateDataPeriod"` WorkerChannelSize int `mapstructure:"workerChannelSize"` @@ -205,6 +206,7 @@ func LoadConfigOptional(path string, errNotFound bool) (Config, error) { viper.SetDefault("exitCleanup::cleanupInterval", 30*time.Second) viper.SetDefault("exitCleanup::cleanupDelay", 5*time.Minute) viper.SetDefault("updateDataPeriod", 10*time.Minute) + viper.SetDefault("syscallPollInterval", 2*time.Second) viper.SetDefault("workerChannelSize", 750000) viper.SetDefault("blockEvents", false) viper.SetDefault("celConfigCache::maxSize", 100000) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 754b342279..172153c99c 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -76,12 +76,13 @@ func TestLoadConfig(t *testing.T) { BulkRetryMaxDelayMs: 30000, }, }, - WorkerPoolSize: 3000, - EventBatchSize: 15000, - EventDedup: EventDedupConfig{Enabled: true, SlotsExponent: 18}, - WorkerChannelSize: 750000, - BlockEvents: false, - ProcfsScanInterval: 30 * time.Second, + WorkerPoolSize: 3000, + EventBatchSize: 15000, + EventDedup: EventDedupConfig{Enabled: true, SlotsExponent: 18}, + WorkerChannelSize: 750000, + BlockEvents: false, + ProcfsScanInterval: 30 * time.Second, + SyscallPollInterval: 2 * time.Second, RuleCoolDown: rulecooldown.RuleCooldownConfig{ CooldownDuration: 1 * time.Hour, CooldownAfterCount: 1, diff --git a/pkg/containerprofilemanager/v1/containerprofile_manager_test.go b/pkg/containerprofilemanager/v1/containerprofile_manager_test.go index 5e7dda1696..5e97e45abb 100644 --- a/pkg/containerprofilemanager/v1/containerprofile_manager_test.go +++ b/pkg/containerprofilemanager/v1/containerprofile_manager_test.go @@ -630,3 +630,23 @@ func TestMaxSniffingTimeLabelConstant(t *testing.T) { func TestMaxWaitForSharedContainerDataConstant(t *testing.T) { assert.Equal(t, 10*time.Minute, MaxWaitForSharedContainerData) } + +func TestTerminationGracePeriod(t *testing.T) { + tests := []struct { + name string + pollInterval time.Duration + want time.Duration + }{ + {"unset falls back to cap", 0, maxTerminationGracePeriod}, + {"short interval used as-is", 500 * time.Millisecond, 500 * time.Millisecond}, + {"default interval used as-is", 2 * time.Second, 2 * time.Second}, + {"interval equal to cap used as-is", maxTerminationGracePeriod, maxTerminationGracePeriod}, + {"interval above cap is capped", 30 * time.Second, maxTerminationGracePeriod}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cpm := &ContainerProfileManager{cfg: config.Config{SyscallPollInterval: tt.pollInterval}} + assert.Equal(t, tt.want, cpm.terminationGracePeriod()) + }) + } +} diff --git a/pkg/containerprofilemanager/v1/helpers.go b/pkg/containerprofilemanager/v1/helpers.go index 84e0ffa7d4..7a749d7281 100644 --- a/pkg/containerprofilemanager/v1/helpers.go +++ b/pkg/containerprofilemanager/v1/helpers.go @@ -24,8 +24,27 @@ const ( MaxSniffingTimeLabel = "kubescape.io/max-sniffing-time" MaxWaitForSharedContainerData = 10 * time.Minute MaxWaitForAck = 30 * time.Second + + // maxTerminationGracePeriod bounds terminationGracePeriod so that a misconfigured (large) + // SyscallPollInterval cannot stall every container termination by that same amount. + maxTerminationGracePeriod = 5 * time.Second ) +// terminationGracePeriod returns how long to wait, right before the final forced profile save +// on container termination or max-sniffing-time, for the syscall tracer's in-flight poll cycle +// to land. The eBPF syscall tracer (SyscallTracer) only ever surfaces the syscalls it has +// decoded from its last periodic map fetch (see pkg/containerwatcher/v2/tracers/syscall.go); if +// a container terminates between two polls, whatever it executed since the last poll is still +// sitting in the kernel map and would otherwise be silently discarded once this manager removes +// the container's data (kubescape/node-agent#922). Waiting one poll interval (capped) gives that +// last cycle a chance to fetch and report before the profile is snapshotted. +func (cpm *ContainerProfileManager) terminationGracePeriod() time.Duration { + if cpm.cfg.SyscallPollInterval <= 0 || cpm.cfg.SyscallPollInterval > maxTerminationGracePeriod { + return maxTerminationGracePeriod + } + return cpm.cfg.SyscallPollInterval +} + // createUUID generates a new UUID string func createUUID() string { return uuid.New().String() diff --git a/pkg/containerprofilemanager/v1/monitoring.go b/pkg/containerprofilemanager/v1/monitoring.go index f579cbc894..a47ba3b68f 100644 --- a/pkg/containerprofilemanager/v1/monitoring.go +++ b/pkg/containerprofilemanager/v1/monitoring.go @@ -45,6 +45,9 @@ func (cpm *ContainerProfileManager) monitorContainer(container *containercollect case err := <-watchedContainer.SyncChannel: switch { case errors.Is(err, ContainerHasTerminatedError): + // Give the syscall tracer's in-flight poll cycle a chance to land before we + // snapshot and discard this container's data (see terminationGracePeriod). + time.Sleep(cpm.terminationGracePeriod()) if err := cpm.saveProfile(watchedContainer, container, true); err != nil { logger.L().Ctx(cpm.lifecycleTracker.LearningCtx(watchedContainer.ContainerID)).Error("failed to save container profile on termination", helpers.Error(err), helpers.String("containerID", watchedContainer.ContainerID), @@ -65,6 +68,8 @@ func (cpm *ContainerProfileManager) monitorContainer(container *containercollect case errors.Is(err, ContainerReachedMaxTime): watchedContainer.SetStatus(objectcache.WatchedContainerStatusCompleted) + // See the ContainerHasTerminatedError case above. + time.Sleep(cpm.terminationGracePeriod()) if err := cpm.saveProfile(watchedContainer, container, true); err != nil { logger.L().Ctx(cpm.lifecycleTracker.LearningCtx(watchedContainer.ContainerID)).Error("failed to save container profile on max time", helpers.Error(err), helpers.String("containerID", watchedContainer.ContainerID), diff --git a/pkg/containerwatcher/v2/tracers/syscall.go b/pkg/containerwatcher/v2/tracers/syscall.go index 2e060b3b76..b3df26a82c 100644 --- a/pkg/containerwatcher/v2/tracers/syscall.go +++ b/pkg/containerwatcher/v2/tracers/syscall.go @@ -3,6 +3,7 @@ package tracers import ( "context" "fmt" + "time" "github.com/inspektor-gadget/inspektor-gadget/pkg/datasource" gadgetcontext "github.com/inspektor-gadget/inspektor-gadget/pkg/gadget-context" @@ -22,12 +23,17 @@ import ( const ( syscallImageName = "ghcr.io/inspektor-gadget/gadget/advise_seccomp:v0.48.1" syscallTraceName = "syscall_tracer" + + // defaultSyscallPollInterval is used when cfg.SyscallPollInterval is unset (e.g. tests + // constructing config.Config directly). It matches the config package's own default. + defaultSyscallPollInterval = 2 * time.Second ) var _ containerwatcher.TracerInterface = (*SyscallTracer)(nil) // SyscallTracer implements TracerInterface for events type SyscallTracer struct { + cfg config.Config eventCallback containerwatcher.ResultCallback gadgetCtx *gadgetcontext.GadgetContext kubeManager operators.DataOperator @@ -41,8 +47,10 @@ func NewSyscallTracer( runtime runtime.Runtime, ociStore *orasoci.ReadOnlyStore, eventCallback containerwatcher.ResultCallback, + cfg config.Config, ) *SyscallTracer { return &SyscallTracer{ + cfg: cfg, eventCallback: eventCallback, kubeManager: kubeManager, ociStore: ociStore, @@ -50,6 +58,17 @@ func NewSyscallTracer( } } +// pollInterval returns the configured eBPF map-fetch interval for the advise_seccomp map, +// falling back to defaultSyscallPollInterval when unset. A long interval widens the window in +// which a terminating container's last syscalls are never fetched before its profile is saved +// and its data discarded (kubescape/node-agent#922), so this is kept short by default. +func (st *SyscallTracer) pollInterval() time.Duration { + if st.cfg.SyscallPollInterval > 0 { + return st.cfg.SyscallPollInterval + } + return defaultSyscallPollInterval +} + // Start initializes and starts the tracer func (st *SyscallTracer) Start(ctx context.Context) error { st.gadgetCtx = gadgetcontext.New( @@ -68,7 +87,7 @@ func (st *SyscallTracer) Start(ctx context.Context) error { go func() { params := map[string]string{ "operator.oci.ebpf.map-fetch-count": "0", - "operator.oci.ebpf.map-fetch-interval": "30s", + "operator.oci.ebpf.map-fetch-interval": st.pollInterval().String(), } err := st.runtime.RunGadget(st.gadgetCtx, nil, params) if err != nil { diff --git a/pkg/containerwatcher/v2/tracers/syscall_test.go b/pkg/containerwatcher/v2/tracers/syscall_test.go index 7f1c35a75d..0cf84b896f 100644 --- a/pkg/containerwatcher/v2/tracers/syscall_test.go +++ b/pkg/containerwatcher/v2/tracers/syscall_test.go @@ -4,15 +4,34 @@ import ( "context" "slices" "testing" + "time" gadgetcontext "github.com/inspektor-gadget/inspektor-gadget/pkg/gadget-context" "github.com/inspektor-gadget/inspektor-gadget/pkg/gadget-service/api" ocihandler "github.com/inspektor-gadget/inspektor-gadget/pkg/operators/oci-handler" + "github.com/kubescape/node-agent/pkg/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" orasoci "oras.land/oras-go/v2/content/oci" ) +func TestSyscallTracerPollInterval(t *testing.T) { + tests := []struct { + name string + cfg config.Config + want time.Duration + }{ + {"unset config falls back to default", config.Config{}, defaultSyscallPollInterval}, + {"configured interval is used", config.Config{SyscallPollInterval: 5 * time.Second}, 5 * time.Second}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + st := &SyscallTracer{cfg: tt.cfg} + assert.Equal(t, tt.want, st.pollInterval()) + }) + } +} + func TestSyscallFields(t *testing.T) { expectedFields := map[string][]string{ "syscalls": { diff --git a/pkg/containerwatcher/v2/tracers/tracer_factory.go b/pkg/containerwatcher/v2/tracers/tracer_factory.go index 014d0600c8..0964cca4ce 100644 --- a/pkg/containerwatcher/v2/tracers/tracer_factory.go +++ b/pkg/containerwatcher/v2/tracers/tracer_factory.go @@ -110,6 +110,7 @@ func (tf *TracerFactory) CreateAllTracers(manager containerwatcher.TracerRegistr tf.runtime, tf.ociStore, tf.createEventCallback(utils.SyscallEventType), + tf.cfg, ) manager.RegisterTracer(syscallTracer) From af4f26e6cf6a6d2ec00d380f05e601dba278b635 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Tue, 25 Aug 2026 21:31:25 +0200 Subject: [PATCH 2/6] 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 --- docs/CONFIGURATION.md | 2 +- docs/features/syscall-poll-interval.md | 98 ++++++++++--------- go.mod | 2 +- go.sum | 4 +- pkg/config/config.go | 2 +- pkg/config/config_test.go | 2 +- .../containerprofile_manager_interface.go | 5 + .../containerprofile_manager_mock.go | 4 + .../v1/containerprofile_manager.go | 10 ++ .../v1/containerprofile_manager_test.go | 48 +++++---- pkg/containerprofilemanager/v1/helpers.go | 37 ++++--- pkg/containerprofilemanager/v1/monitoring.go | 8 +- pkg/containerwatcher/v2/tracers/syscall.go | 27 ++++- .../v2/tracers/syscall_test.go | 9 +- .../v2/tracers/tracer_factory.go | 6 ++ 15 files changed, 171 insertions(+), 93 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index ac0831c78c..d64a289f7f 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -173,7 +173,7 @@ correlation is an upgrade, not a dependency. | `profilesCacheRefreshRate` | duration | `1m` | Profile cache refresh rate | | `procfsScanInterval` | duration | `30s` | Procfs scan interval | | `procfsPidScanInterval` | duration | `5s` | Per-PID procfs scan interval | -| `syscallPollInterval` | duration | `2s` | How often the seccomp syscall tracer fetches its eBPF map. Also bounds (capped at 5s) how long a container's final profile save waits on termination so the last poll cycle can land. See [docs/features/syscall-poll-interval.md](features/syscall-poll-interval.md). | +| `syscallPollInterval` | duration | `30s` | How often the seccomp syscall tracer fetches its eBPF map for the live, in-progress profile. Does not affect termination-time capture — see [docs/features/syscall-poll-interval.md](features/syscall-poll-interval.md). | | `maxDelaySeconds` | int | `30` | Max random delay for jitter | | `maxJitterPercentage` | int | `5` | Max jitter percentage | diff --git a/docs/features/syscall-poll-interval.md b/docs/features/syscall-poll-interval.md index c6cf524f53..1b8e07d4ff 100644 --- a/docs/features/syscall-poll-interval.md +++ b/docs/features/syscall-poll-interval.md @@ -1,67 +1,75 @@ -# Syscall Poll Interval and Termination Grace Period +# Syscall Termination Flush Container profiles could end up with 0 or partial syscalls even when the container executed -tens of distinct syscalls (kubescape/node-agent#922). This documents the root cause and the -mitigation. +tens of distinct syscalls (kubescape/node-agent#922). This documents the root cause and the fix. ## Root cause `SyscallTracer` (pkg/containerwatcher/v2/tracers/syscall.go) doesn't read the kernel's `advise_seccomp` eBPF map on demand. It runs the `advise_seccomp` OCI gadget with a periodic -map-fetch: every poll interval, the gadget operator drains the whole map -(`BPF_MAP_LOOKUP_AND_DELETE_BATCH`) and emits one datasource event per entry, which +map-fetch (`syscallPollInterval`, default `30s`): on each tick, the gadget operator drains the +whole map (`BPF_MAP_LOOKUP_AND_DELETE_BATCH`) and emits one datasource event per entry, which `SyscallTracer.callback` decodes and routes to `ContainerProfileManager.ReportSyscall` by -container ID. Until the fix below, that interval was hardcoded to 30 seconds. +container ID. `ContainerProfileManager` (pkg/containerprofilemanager/v1) reacts to container termination synchronously: `deleteContainer` sends `ContainerHasTerminatedError` on the container's sync channel, and `monitorContainer` (pkg/containerprofilemanager/v1/monitoring.go) immediately does a final forced `saveProfile` and hands the ack back. Whatever syscalls the kernel recorded since -the tracer's last poll are still sitting in the eBPF map at that moment — they have not been -fetched yet — and once the profile is saved and the container's entry is removed -(`removeContainerEntry`), that data is unreachable. A container whose lifetime is shorter than -the poll interval could complete with zero syscalls recorded; any container has up to one full -poll interval of syscalls at risk on termination. +the tracer's last poll are still sitting in the eBPF map at that moment — not yet fetched — and +once the profile is saved and the container's entry is removed (`removeContainerEntry`), that +data is unreachable. A container whose lifetime is shorter than the poll interval could complete +with zero syscalls recorded; any container has up to one full poll interval of syscalls at risk +on termination. -There is no per-container on-demand flush available from the pinned `advise_seccomp` gadget: the -eBPF map iterator (`pkg/operators/ebpf/maps.go` in the `inspektor-gadget` fork) only supports a -fixed interval/count and an optional flush-on-stop that fires when the whole gadget instance -stops — not when one container among many terminates. Re-introducing a true synchronous -`Peek()` (as node-agent v1 had) would require changing that upstream gadget's fetch loop to -support an on-demand trigger, which is out of scope here. +## Fix: an on-demand flush trigger -## Mitigation +Rather than shrinking the poll interval (which only narrows the race, and trades it for more +frequent map fetches at 30s cadence for every container node-wide), this recovers the +at-risk data directly: `ContainerProfileManager` requests an immediate, out-of-band fetch of the +eBPF map right before a container's final forced save, instead of waiting for the tracer's next +scheduled tick. -Two changes narrow the race instead, both purely on the node-agent side: +This required a small addition to the vendored `inspektor-gadget` fork +(`github.com/matthyx/inspektor-gadget`, matthyx/inspektor-gadget#12), since the OCI gadget's +map-iterator mechanism (`pkg/operators/ebpf/maps.go`) previously only supported a fixed +interval/count schedule, plus an optional flush-on-*stop* that fires when the whole gadget +instance is torn down — not when node-agent stops tracking one container among many that a +single shared, long-running gadget instance observes. `ebpfoperator.TriggerManualMapFetch(names +...string)` adds a package-level, fire-and-forget way to request an immediate fetch from any +currently running map iterator (optionally filtered by datasource name), independent of its +normal schedule. -1. **`syscallPollInterval` config** (default `2s`, was hardcoded `30s`) — passed to the gadget as - `operator.oci.ebpf.map-fetch-interval` (`SyscallTracer.pollInterval()`). Shortening the - interval bounds how much syscall data can be sitting unfetched in the kernel map at any given - moment, from up to 30s down to the configured value. +### The pieces -2. **Termination grace period** — `ContainerProfileManager.terminationGracePeriod()` - (pkg/containerprofilemanager/v1/helpers.go) sleeps for `min(syscallPollInterval, 5s)` - immediately before the final forced `saveProfile` call on both termination paths - (`ContainerHasTerminatedError` and `ContainerReachedMaxTime` in monitoring.go). This gives the - next poll cycle — which is already running independently of any single container's lifecycle - — a chance to fetch and report the last syscalls before they become unreachable. The 5s cap - protects against a misconfigured (large) `syscallPollInterval` stalling every container - termination by that same amount; `MaxWaitForAck` (30s) has ample headroom above the cap. +1. **`SyscallTracer.Peek()`** (pkg/containerwatcher/v2/tracers/syscall.go) calls + `ebpfoperator.TriggerManualMapFetch("syscalls")`, targeting only the seccomp tracer's map + iterator. +2. **`ContainerProfileManager.SetSyscallFlusher`** + (pkg/containerprofilemanager/containerprofile_manager_interface.go) lets a caller register a + flush callback; `TracerFactory.CreateAllTracers` (pkg/containerwatcher/v2/tracers/tracer_factory.go) + wires it to `syscallTracer.Peek`, but only when the syscall tracer is actually enabled — an + unwired flusher would otherwise cost every container termination a wait for nothing. +3. **`flushAndSettle`** (pkg/containerprofilemanager/v1/helpers.go) is called on both + termination paths (`ContainerHasTerminatedError` and `ContainerReachedMaxTime` in + monitoring.go) right before the final forced `saveProfile`. It invokes the registered flusher + (no-op if none is registered) and then waits `postSyscallFlushSettleDelay` (`500ms`) — not + for the fetch itself, which is comparatively fast, but for the resulting events to travel + through the ordered event queue and worker pool into this container's collected data before + it's snapshotted and cleared. -Together these shrink the loss window from "up to 30s, every termination" to "up to -`syscallPollInterval` (default 2s), and only when a poll genuinely didn't fire in that window" — -without touching the gadget's fetch mechanism. +`syscallPollInterval` still exists and still defaults to `30s`: it only governs how fresh the +*live*, still-running profile is between periodic storage updates (`updateDataPeriod`), which is +an orthogonal concern from termination-time capture now handled by the flush trigger. ## Known limitations -This is a bounded-race mitigation, not a guarantee. A container that terminates and whose final -poll cycle is delayed (e.g. by scheduler contention) beyond the grace period can still lose its -most recent syscalls. Achieving zero-loss capture would require an on-demand, per-container flush -in the underlying gadget (`matthyx/inspektor-gadget`), tracked as a separate follow-up. - -Shortening the poll interval means more frequent map fetches. Each fetch drains the entire -`advise_seccomp` map for every actively-traced container's mount namespace in one batch (not just -one), so the added overhead is one small periodic drain rather than N per-container calls; syscall -events reaching `ContainerProfileManager` are further deduplicated per (mount ns, pid, syscall) -within a 5s window (`dedupTTLSyscall`, pkg/containerwatcher/v2/event_handler_factory.go), which -absorbs most of the added repeat-event volume from polling more often. +`TriggerManualMapFetch` is fire-and-forget — it doesn't block until the fetch completes, and +`flushAndSettle`'s fixed settle delay is a bounded wait for the event pipeline, not a guarantee +tied to the fetch itself. Under severe scheduler contention or a saturated event queue, the +flushed events could in principle still arrive after the delay elapses, in which case termination +falls back to whatever the tracer already had before the flush — the same race as before, just +far less likely to be hit since the flush is triggered immediately rather than waiting for a +30s tick. Achieving an actual guarantee would require synchronously waiting on the specific +resulting events to be processed for this container, which the event pipeline (queue + +worker pool) doesn't currently expose a way to do. diff --git a/go.mod b/go.mod index 1717ad1e39..5aed00fb4b 100644 --- a/go.mod +++ b/go.mod @@ -472,7 +472,7 @@ require ( zombiezen.com/go/sqlite v1.4.0 // indirect ) -replace github.com/inspektor-gadget/inspektor-gadget => github.com/matthyx/inspektor-gadget v0.0.0-20260824132042-5bba57473b53 +replace github.com/inspektor-gadget/inspektor-gadget => github.com/matthyx/inspektor-gadget v0.0.0-20260825192450-2b683d258349 replace github.com/anchore/syft => github.com/kubescape/syft v1.32.0-ks.2 diff --git a/go.sum b/go.sum index caec8d3f8d..b1d62cc5bd 100644 --- a/go.sum +++ b/go.sum @@ -919,8 +919,8 @@ github.com/mailru/easyjson v0.0.0-20190312143242-1de009706dbe/go.mod h1:C1wdFJiN github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= github.com/mailru/easyjson v0.9.0 h1:PrnmzHw7262yW8sTBwxi1PdJA3Iw/EKBa8psRf7d9a4= github.com/mailru/easyjson v0.9.0/go.mod h1:1+xMtQp2MRNVL/V1bOzuP3aP8VNwRW55fQUto+XFtTU= -github.com/matthyx/inspektor-gadget v0.0.0-20260824132042-5bba57473b53 h1:gFcmFBYF1u5p+T7MPY9WcvBafNIsvpnWNJb93XXhJkA= -github.com/matthyx/inspektor-gadget v0.0.0-20260824132042-5bba57473b53/go.mod h1:cwCFczq1LJ6Frpur0Vr5Ncic77a9ihki07Xpwzy+ItI= +github.com/matthyx/inspektor-gadget v0.0.0-20260825192450-2b683d258349 h1:9fxhaVRFGjJIOXR0NVU0+hQPI7ixrzYSa/bW1KWYATo= +github.com/matthyx/inspektor-gadget v0.0.0-20260825192450-2b683d258349/go.mod h1:cwCFczq1LJ6Frpur0Vr5Ncic77a9ihki07Xpwzy+ItI= github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= github.com/mattn/go-colorable v0.1.4/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE= github.com/mattn/go-colorable v0.1.6/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc= diff --git a/pkg/config/config.go b/pkg/config/config.go index 4aa3abeb32..49a47322ab 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -206,7 +206,7 @@ func LoadConfigOptional(path string, errNotFound bool) (Config, error) { viper.SetDefault("exitCleanup::cleanupInterval", 30*time.Second) viper.SetDefault("exitCleanup::cleanupDelay", 5*time.Minute) viper.SetDefault("updateDataPeriod", 10*time.Minute) - viper.SetDefault("syscallPollInterval", 2*time.Second) + viper.SetDefault("syscallPollInterval", 30*time.Second) viper.SetDefault("workerChannelSize", 750000) viper.SetDefault("blockEvents", false) viper.SetDefault("celConfigCache::maxSize", 100000) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 172153c99c..467f55d2a3 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -82,7 +82,7 @@ func TestLoadConfig(t *testing.T) { WorkerChannelSize: 750000, BlockEvents: false, ProcfsScanInterval: 30 * time.Second, - SyscallPollInterval: 2 * time.Second, + SyscallPollInterval: 30 * time.Second, RuleCoolDown: rulecooldown.RuleCooldownConfig{ CooldownDuration: 1 * time.Hour, CooldownAfterCount: 1, diff --git a/pkg/containerprofilemanager/containerprofile_manager_interface.go b/pkg/containerprofilemanager/containerprofile_manager_interface.go index 8e34a54eff..0c15d023a7 100644 --- a/pkg/containerprofilemanager/containerprofile_manager_interface.go +++ b/pkg/containerprofilemanager/containerprofile_manager_interface.go @@ -21,6 +21,11 @@ type ContainerProfileManagerClient interface { ReportDroppedEvent(containerID string) RegisterForContainerEndOfLife(notificationChannel chan *containercollection.Container) OnQueueError(profile *v1beta1.ContainerProfile, containerID string, err error) + // SetSyscallFlusher registers a callback invoked right before a container's final + // forced profile save (on termination or max-sniffing-time), requesting an immediate + // out-of-band fetch of any not-yet-polled syscalls for that container so they aren't + // silently discarded (see SyscallTracer.Peek, kubescape/node-agent#922). + SetSyscallFlusher(flush func()) } type Enricher interface { diff --git a/pkg/containerprofilemanager/containerprofile_manager_mock.go b/pkg/containerprofilemanager/containerprofile_manager_mock.go index 5570db19ed..fff1468566 100644 --- a/pkg/containerprofilemanager/containerprofile_manager_mock.go +++ b/pkg/containerprofilemanager/containerprofile_manager_mock.go @@ -63,6 +63,10 @@ func (a ContainerProfileManagerMock) RegisterForContainerEndOfLife(_ chan *conta // noop } +func (a ContainerProfileManagerMock) SetSyscallFlusher(_ func()) { + // noop +} + func (a ContainerProfileManagerMock) ReportNetworkEvent(_ string, _ utils.NetworkEvent) { // noop } diff --git a/pkg/containerprofilemanager/v1/containerprofile_manager.go b/pkg/containerprofilemanager/v1/containerprofile_manager.go index f4a5fafb3d..efa58938a7 100644 --- a/pkg/containerprofilemanager/v1/containerprofile_manager.go +++ b/pkg/containerprofilemanager/v1/containerprofile_manager.go @@ -97,12 +97,22 @@ type ContainerProfileManager struct { completionNotifier objectcache.CompletionNotifier lifecycleTracker *otelsetup.ProfileLifecycleTracker + + // syscallFlusher, if set, is called right before a container's final forced profile + // save to request an immediate out-of-band fetch of its not-yet-polled syscalls. See + // SetSyscallFlusher and kubescape/node-agent#922. + syscallFlusher func() } func (cpm *ContainerProfileManager) SetCompletionNotifier(n objectcache.CompletionNotifier) { cpm.completionNotifier = n } +// SetSyscallFlusher implements containerprofilemanager.ContainerProfileManagerClient. +func (cpm *ContainerProfileManager) SetSyscallFlusher(flush func()) { + cpm.syscallFlusher = flush +} + func (cpm *ContainerProfileManager) notifyCompleted(containerID string) { if cpm.completionNotifier != nil { cpm.completionNotifier.NotifyContainerCompleted(containerID) diff --git a/pkg/containerprofilemanager/v1/containerprofile_manager_test.go b/pkg/containerprofilemanager/v1/containerprofile_manager_test.go index 5e97e45abb..f621cfbeb7 100644 --- a/pkg/containerprofilemanager/v1/containerprofile_manager_test.go +++ b/pkg/containerprofilemanager/v1/containerprofile_manager_test.go @@ -631,22 +631,34 @@ func TestMaxWaitForSharedContainerDataConstant(t *testing.T) { assert.Equal(t, 10*time.Minute, MaxWaitForSharedContainerData) } -func TestTerminationGracePeriod(t *testing.T) { - tests := []struct { - name string - pollInterval time.Duration - want time.Duration - }{ - {"unset falls back to cap", 0, maxTerminationGracePeriod}, - {"short interval used as-is", 500 * time.Millisecond, 500 * time.Millisecond}, - {"default interval used as-is", 2 * time.Second, 2 * time.Second}, - {"interval equal to cap used as-is", maxTerminationGracePeriod, maxTerminationGracePeriod}, - {"interval above cap is capped", 30 * time.Second, maxTerminationGracePeriod}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cpm := &ContainerProfileManager{cfg: config.Config{SyscallPollInterval: tt.pollInterval}} - assert.Equal(t, tt.want, cpm.terminationGracePeriod()) - }) - } +func TestSetSyscallFlusher(t *testing.T) { + cpm := &ContainerProfileManager{} + assert.Nil(t, cpm.syscallFlusher) + + called := false + cpm.SetSyscallFlusher(func() { called = true }) + assert.NotNil(t, cpm.syscallFlusher) + + cpm.syscallFlusher() + assert.True(t, called) +} + +func TestFlushAndSettleNoFlusherIsNoopAndFast(t *testing.T) { + cpm := &ContainerProfileManager{} + start := time.Now() + cpm.flushAndSettle() + assert.Less(t, time.Since(start), postSyscallFlushSettleDelay) +} + +func TestFlushAndSettleCallsFlusherAndWaits(t *testing.T) { + called := false + cpm := &ContainerProfileManager{} + cpm.SetSyscallFlusher(func() { called = true }) + + start := time.Now() + cpm.flushAndSettle() + elapsed := time.Since(start) + + assert.True(t, called) + assert.GreaterOrEqual(t, elapsed, postSyscallFlushSettleDelay) } diff --git a/pkg/containerprofilemanager/v1/helpers.go b/pkg/containerprofilemanager/v1/helpers.go index 7a749d7281..0c0818b31b 100644 --- a/pkg/containerprofilemanager/v1/helpers.go +++ b/pkg/containerprofilemanager/v1/helpers.go @@ -25,24 +25,31 @@ const ( MaxWaitForSharedContainerData = 10 * time.Minute MaxWaitForAck = 30 * time.Second - // maxTerminationGracePeriod bounds terminationGracePeriod so that a misconfigured (large) - // SyscallPollInterval cannot stall every container termination by that same amount. - maxTerminationGracePeriod = 5 * time.Second + // postSyscallFlushSettleDelay bounds how long flushAndSettle waits, after requesting an + // immediate out-of-band syscall fetch (see SetSyscallFlusher), for the resulting events + // to travel through the event queue and worker pool into this container's data before its + // profile is snapshotted and its data is cleared. It only needs to cover that pipeline's + // latency (normally low milliseconds), not any polling interval, since the fetch itself + // was already requested rather than waited on. + postSyscallFlushSettleDelay = 500 * time.Millisecond ) -// terminationGracePeriod returns how long to wait, right before the final forced profile save -// on container termination or max-sniffing-time, for the syscall tracer's in-flight poll cycle -// to land. The eBPF syscall tracer (SyscallTracer) only ever surfaces the syscalls it has -// decoded from its last periodic map fetch (see pkg/containerwatcher/v2/tracers/syscall.go); if -// a container terminates between two polls, whatever it executed since the last poll is still -// sitting in the kernel map and would otherwise be silently discarded once this manager removes -// the container's data (kubescape/node-agent#922). Waiting one poll interval (capped) gives that -// last cycle a chance to fetch and report before the profile is snapshotted. -func (cpm *ContainerProfileManager) terminationGracePeriod() time.Duration { - if cpm.cfg.SyscallPollInterval <= 0 || cpm.cfg.SyscallPollInterval > maxTerminationGracePeriod { - return maxTerminationGracePeriod +// flushAndSettle requests an immediate, out-of-band fetch of any not-yet-polled syscalls (via +// the flusher registered with SetSyscallFlusher) and briefly waits for the result to land in +// this container's data. It is a no-op with no wait if no flusher is registered (e.g. the +// syscall tracer is disabled). +// +// Call this right before a container's final forced profile save on termination or +// max-sniffing-time: the eBPF syscall tracer only ever surfaces syscalls it has decoded from +// its last periodic map fetch (see pkg/containerwatcher/v2/tracers/syscall.go), so without this +// a container terminating between two polls would silently lose whatever it executed since the +// last one once this manager removes its data (kubescape/node-agent#922). +func (cpm *ContainerProfileManager) flushAndSettle() { + if cpm.syscallFlusher == nil { + return } - return cpm.cfg.SyscallPollInterval + cpm.syscallFlusher() + time.Sleep(postSyscallFlushSettleDelay) } // createUUID generates a new UUID string diff --git a/pkg/containerprofilemanager/v1/monitoring.go b/pkg/containerprofilemanager/v1/monitoring.go index a47ba3b68f..696787e2a8 100644 --- a/pkg/containerprofilemanager/v1/monitoring.go +++ b/pkg/containerprofilemanager/v1/monitoring.go @@ -45,9 +45,9 @@ func (cpm *ContainerProfileManager) monitorContainer(container *containercollect case err := <-watchedContainer.SyncChannel: switch { case errors.Is(err, ContainerHasTerminatedError): - // Give the syscall tracer's in-flight poll cycle a chance to land before we - // snapshot and discard this container's data (see terminationGracePeriod). - time.Sleep(cpm.terminationGracePeriod()) + // Recover any syscalls not yet surfaced by the tracer's periodic poll before + // we snapshot and discard this container's data (see flushAndSettle). + cpm.flushAndSettle() if err := cpm.saveProfile(watchedContainer, container, true); err != nil { logger.L().Ctx(cpm.lifecycleTracker.LearningCtx(watchedContainer.ContainerID)).Error("failed to save container profile on termination", helpers.Error(err), helpers.String("containerID", watchedContainer.ContainerID), @@ -69,7 +69,7 @@ func (cpm *ContainerProfileManager) monitorContainer(container *containercollect case errors.Is(err, ContainerReachedMaxTime): watchedContainer.SetStatus(objectcache.WatchedContainerStatusCompleted) // See the ContainerHasTerminatedError case above. - time.Sleep(cpm.terminationGracePeriod()) + cpm.flushAndSettle() if err := cpm.saveProfile(watchedContainer, container, true); err != nil { logger.L().Ctx(cpm.lifecycleTracker.LearningCtx(watchedContainer.ContainerID)).Error("failed to save container profile on max time", helpers.Error(err), helpers.String("containerID", watchedContainer.ContainerID), diff --git a/pkg/containerwatcher/v2/tracers/syscall.go b/pkg/containerwatcher/v2/tracers/syscall.go index b3df26a82c..8da96fd7ff 100644 --- a/pkg/containerwatcher/v2/tracers/syscall.go +++ b/pkg/containerwatcher/v2/tracers/syscall.go @@ -8,6 +8,7 @@ import ( "github.com/inspektor-gadget/inspektor-gadget/pkg/datasource" gadgetcontext "github.com/inspektor-gadget/inspektor-gadget/pkg/gadget-context" "github.com/inspektor-gadget/inspektor-gadget/pkg/operators" + ebpfoperator "github.com/inspektor-gadget/inspektor-gadget/pkg/operators/ebpf" ocihandler "github.com/inspektor-gadget/inspektor-gadget/pkg/operators/oci-handler" "github.com/inspektor-gadget/inspektor-gadget/pkg/operators/simple" "github.com/inspektor-gadget/inspektor-gadget/pkg/runtime" @@ -24,9 +25,13 @@ const ( syscallImageName = "ghcr.io/inspektor-gadget/gadget/advise_seccomp:v0.48.1" syscallTraceName = "syscall_tracer" + // syscallDataSourceName is the advise_seccomp gadget's map-iterator datasource name + // (see TestSyscallFields). Peek uses it to target only this tracer's map iterator. + syscallDataSourceName = "syscalls" + // defaultSyscallPollInterval is used when cfg.SyscallPollInterval is unset (e.g. tests // constructing config.Config directly). It matches the config package's own default. - defaultSyscallPollInterval = 2 * time.Second + defaultSyscallPollInterval = 30 * time.Second ) var _ containerwatcher.TracerInterface = (*SyscallTracer)(nil) @@ -59,9 +64,10 @@ func NewSyscallTracer( } // pollInterval returns the configured eBPF map-fetch interval for the advise_seccomp map, -// falling back to defaultSyscallPollInterval when unset. A long interval widens the window in -// which a terminating container's last syscalls are never fetched before its profile is saved -// and its data discarded (kubescape/node-agent#922), so this is kept short by default. +// falling back to defaultSyscallPollInterval when unset. This interval only bounds staleness +// of the live, in-progress profile; termination-time loss (kubescape/node-agent#922) is instead +// addressed by Peek, which recovers a terminating container's last syscalls immediately rather +// than waiting on this schedule. func (st *SyscallTracer) pollInterval() time.Duration { if st.cfg.SyscallPollInterval > 0 { return st.cfg.SyscallPollInterval @@ -69,6 +75,19 @@ func (st *SyscallTracer) pollInterval() time.Duration { return defaultSyscallPollInterval } +// Peek requests an immediate, out-of-band fetch of the advise_seccomp eBPF map, on top of the +// tracer's normal periodic poll (see pollInterval). It does not block until the fetch completes +// or until any resulting events have been processed by this tracer's eventCallback — callers +// needing that must wait separately afterward. +// +// This exists so a caller about to stop tracking a container (ContainerProfileManager, on +// termination or max-sniffing-time) can recover that container's syscalls executed since the +// tracer's last poll, which would otherwise still be sitting unfetched in the kernel map and +// become unreachable once the container's data is removed (kubescape/node-agent#922). +func (st *SyscallTracer) Peek() { + ebpfoperator.TriggerManualMapFetch(syscallDataSourceName) +} + // Start initializes and starts the tracer func (st *SyscallTracer) Start(ctx context.Context) error { st.gadgetCtx = gadgetcontext.New( diff --git a/pkg/containerwatcher/v2/tracers/syscall_test.go b/pkg/containerwatcher/v2/tracers/syscall_test.go index 0cf84b896f..b8bc0b23d5 100644 --- a/pkg/containerwatcher/v2/tracers/syscall_test.go +++ b/pkg/containerwatcher/v2/tracers/syscall_test.go @@ -32,9 +32,16 @@ func TestSyscallTracerPollInterval(t *testing.T) { } } +func TestSyscallTracerPeekDoesNotPanic(t *testing.T) { + // With no gadget instance running, this must be a silent no-op (see + // ebpfoperator.TriggerManualMapFetch). + st := &SyscallTracer{} + assert.NotPanics(t, st.Peek) +} + func TestSyscallFields(t *testing.T) { expectedFields := map[string][]string{ - "syscalls": { + syscallDataSourceName: { "mntns_id_raw", "syscalls", }, diff --git a/pkg/containerwatcher/v2/tracers/tracer_factory.go b/pkg/containerwatcher/v2/tracers/tracer_factory.go index 0964cca4ce..7bdc694fa9 100644 --- a/pkg/containerwatcher/v2/tracers/tracer_factory.go +++ b/pkg/containerwatcher/v2/tracers/tracer_factory.go @@ -113,6 +113,12 @@ func (tf *TracerFactory) CreateAllTracers(manager containerwatcher.TracerRegistr tf.cfg, ) manager.RegisterTracer(syscallTracer) + if syscallTracer.IsEnabled(tf.cfg) { + // Only wire the flusher when this tracer will actually run; otherwise + // ContainerProfileManager would pay flushAndSettle's settle delay on every + // container termination for a fetch that can never surface anything. + tf.containerProfileManager.SetSyscallFlusher(syscallTracer.Peek) + } // Create exec tracer execTracer := NewExecTracer( From cf6d247641007cca6dbb5c1fc6611968eef1efaf Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Wed, 26 Aug 2026 07:53:06 +0200 Subject: [PATCH 3/6] 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 --- docs/CONFIGURATION.md | 2 +- docs/features/syscall-poll-interval.md | 134 ++++++++++-------- pkg/config/config.go | 2 +- pkg/config/config_test.go | 2 +- pkg/containerwatcher/v2/tracers/syscall.go | 53 +++++-- .../v2/tracers/syscall_test.go | 25 +++- 6 files changed, 138 insertions(+), 80 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index d64a289f7f..69d787b105 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -173,7 +173,7 @@ correlation is an upgrade, not a dependency. | `profilesCacheRefreshRate` | duration | `1m` | Profile cache refresh rate | | `procfsScanInterval` | duration | `30s` | Procfs scan interval | | `procfsPidScanInterval` | duration | `5s` | Per-PID procfs scan interval | -| `syscallPollInterval` | duration | `30s` | How often the seccomp syscall tracer fetches its eBPF map for the live, in-progress profile. Does not affect termination-time capture — see [docs/features/syscall-poll-interval.md](features/syscall-poll-interval.md). | +| `syscallPollInterval` | duration | `5s` | How often the seccomp syscall tracer fetches its eBPF map for live event delivery (profile-building and rule-based alerting). Termination-time capture is handled separately and doesn't depend on this value — see [docs/features/syscall-poll-interval.md](features/syscall-poll-interval.md). | | `maxDelaySeconds` | int | `30` | Max random delay for jitter | | `maxJitterPercentage` | int | `5` | Max jitter percentage | diff --git a/docs/features/syscall-poll-interval.md b/docs/features/syscall-poll-interval.md index 1b8e07d4ff..0d056340a7 100644 --- a/docs/features/syscall-poll-interval.md +++ b/docs/features/syscall-poll-interval.md @@ -1,75 +1,87 @@ -# Syscall Termination Flush +# Syscall Tracer: On-Demand Fetching Container profiles could end up with 0 or partial syscalls even when the container executed -tens of distinct syscalls (kubescape/node-agent#922). This documents the root cause and the fix. +tens of distinct syscalls (kubescape/node-agent#922). This documents the root cause and the fix, +including why the fix ended up restoring the pre-PR-#591 architecture's shape rather than +tuning the post-#591 one. -## Root cause +## Background: how this worked before PR #591 -`SyscallTracer` (pkg/containerwatcher/v2/tracers/syscall.go) doesn't read the kernel's -`advise_seccomp` eBPF map on demand. It runs the `advise_seccomp` OCI gadget with a periodic -map-fetch (`syscallPollInterval`, default `30s`): on each tick, the gadget operator drains the -whole map (`BPF_MAP_LOOKUP_AND_DELETE_BATCH`) and emits one datasource event per entry, which -`SyscallTracer.callback` decodes and routes to `ContainerProfileManager.ReportSyscall` by -container ID. +Before PR #591 ("Feature/refactor container watcher"), node-agent used a **non-destructive** +`Peek(mntns)` on the vendored native seccomp tracer (`pkg/gadgets/advise/seccomp/tracer`, old +`amirmalka/inspektor-gadget` fork): a plain `bpf_map_lookup_elem`, no delete. Two independent +consumers each registered this same callback and drove it on their own schedule: -`ContainerProfileManager` (pkg/containerprofilemanager/v1) reacts to container termination -synchronously: `deleteContainer` sends `ContainerHasTerminatedError` on the container's sync -channel, and `monitorContainer` (pkg/containerprofilemanager/v1/monitoring.go) immediately does -a final forced `saveProfile` and hands the ack back. Whatever syscalls the kernel recorded since -the tracer's last poll are still sitting in the eBPF map at that moment — not yet fetched — and -once the profile is saved and the container's entry is removed (`removeContainerEntry`), that -data is unreachable. A container whose lifetime is shorter than the poll interval could complete -with zero syscalls recorded; any container has up to one full poll interval of syscalls at risk -on termination. +- `ContainerProfileManager` called it on demand, exactly when saving a profile (periodic + checkpoint or termination) — never on a timer of its own. +- `RuleManager` ran its **own 5-second ticker per monitored container** + (`pkg/rulemanager/v1/rule_manager.go`, `syscallPeriod = 5 * time.Second`) purely for real-time, + syscall-based rule alerting, fully decoupled from profile-saving. -## Fix: an on-demand flush trigger +PR #591 moved the tracer to the OCI-gadget framework (`advise_seccomp` image), whose generic +map-iterator mechanism (`GADGET_MAPITER`, `pkg/operators/ebpf/maps.go` in the +`matthyx/inspektor-gadget` fork) reads batches via `BPF_MAP_LOOKUP_AND_DELETE_BATCH` — a +destructive drain. The refactor consolidated to a single shared tracer instance with one +internal periodic fetch (`operator.oci.ebpf.map-fetch-interval`, ended up at `30s`) whose decoded +events fan out to every consumer (`ContainerProfileManager`, `RuleManager`, metrics) alike. That +single shared schedule is what regressed rule-alerting latency from the original 5s to 30s, and +is also what caused #922: a container terminating between two 30s ticks lost whatever it +executed since the last one, because nothing else could recover it before the container's data +was removed. -Rather than shrinking the poll interval (which only narrows the race, and trades it for more -frequent map fetches at 30s cadence for every container node-wide), this recovers the -at-risk data directly: `ContainerProfileManager` requests an immediate, out-of-band fetch of the -eBPF map right before a container's final forced save, instead of waiting for the tracer's next -scheduled tick. +Note: the destructiveness of the read was never actually why a single shared schedule was +required — nothing stops running two independent instances of the same gadget image, each +with its own attach and its own private map, at different cadences. The real reason a single +schedule existed is simpler: there was only one tracer instance and one internal ticker driving +it, and every consumer got whatever cadence that one ticker happened to use. -This required a small addition to the vendored `inspektor-gadget` fork -(`github.com/matthyx/inspektor-gadget`, matthyx/inspektor-gadget#12), since the OCI gadget's -map-iterator mechanism (`pkg/operators/ebpf/maps.go`) previously only supported a fixed -interval/count schedule, plus an optional flush-on-*stop* that fires when the whole gadget -instance is torn down — not when node-agent stops tracking one container among many that a -single shared, long-running gadget instance observes. `ebpfoperator.TriggerManualMapFetch(names -...string)` adds a package-level, fire-and-forget way to request an immediate fetch from any -currently running map iterator (optionally filtered by datasource name), independent of its -normal schedule. +## The fix -### The pieces +Rather than picking a single fixed interval that compromises between "fast enough for alerting" +and "cheap enough to not matter," the tracer's own internal fetch schedule is removed entirely, +and *all* fetching — periodic or not — goes through one explicit call: -1. **`SyscallTracer.Peek()`** (pkg/containerwatcher/v2/tracers/syscall.go) calls - `ebpfoperator.TriggerManualMapFetch("syscalls")`, targeting only the seccomp tracer's map - iterator. -2. **`ContainerProfileManager.SetSyscallFlusher`** - (pkg/containerprofilemanager/containerprofile_manager_interface.go) lets a caller register a - flush callback; `TracerFactory.CreateAllTracers` (pkg/containerwatcher/v2/tracers/tracer_factory.go) - wires it to `syscallTracer.Peek`, but only when the syscall tracer is actually enabled — an - unwired flusher would otherwise cost every container termination a wait for nothing. -3. **`flushAndSettle`** (pkg/containerprofilemanager/v1/helpers.go) is called on both - termination paths (`ContainerHasTerminatedError` and `ContainerReachedMaxTime` in - monitoring.go) right before the final forced `saveProfile`. It invokes the registered flusher - (no-op if none is registered) and then waits `postSyscallFlushSettleDelay` (`500ms`) — not - for the fetch itself, which is comparatively fast, but for the resulting events to travel - through the ordered event queue and worker pool into this container's collected data before - it's snapshotted and cleared. +- **`ebpfoperator.TriggerManualMapFetch(names ...string)`** (added in + `matthyx/inspektor-gadget#12`) requests an immediate, out-of-band fetch from a running map + iterator, independent of any schedule. The OCI gadget is started with + `map-fetch-interval: "0"` and `map-fetch-count: "0"`, which — per `maps.go`'s iterator loop — + disables its own ticker entirely; the iterator then does nothing except wait to be triggered + (plus the existing flush-on-stop safety net when the whole gadget shuts down). +- **`SyscallTracer.Peek()`** (pkg/containerwatcher/v2/tracers/syscall.go) calls that trigger. +- **`SyscallTracer.runPeekLoop`**, started in `Start`, calls `Peek` on `syscallPollInterval` + (default `5s`, matching the original `RuleManager` cadence) — this is what now drives live + event delivery to every consumer (profile-building and rule alerting alike), restoring the + original alerting responsiveness. Unlike the pre-#591 design, this is **one global ticker** + regardless of how many containers are being traced (the old design ran one 5s ticker *per + container*), since one `TriggerManualMapFetch` call drains and dispatches every currently + tracked container's pending syscalls in a single batch. +- **`ContainerProfileManager.SetSyscallFlusher`** lets `TracerFactory` additionally wire + `Peek` to be called once more, out of that schedule, right before a container's final forced + profile save (`flushAndSettle` in pkg/containerprofilemanager/v1/helpers.go, called from both + termination paths in monitoring.go) — closing even the up-to-`syscallPollInterval` gap the + periodic schedule alone would still leave at exactly the moment a container disappears. + `flushAndSettle` waits a fixed `postSyscallFlushSettleDelay` (`500ms`) afterward for the + resulting events to travel through the event queue/worker pool into the container's data + before it's snapshotted and cleared. This wiring only happens when the syscall tracer is + actually enabled (`SyscallTracer.IsEnabled`), so a disabled tracer costs nothing. -`syscallPollInterval` still exists and still defaults to `30s`: it only governs how fresh the -*live*, still-running profile is between periodic storage updates (`updateDataPeriod`), which is -an orthogonal concern from termination-time capture now handled by the flush trigger. +## Why this isn't a regression risk for alerting + +`SyscallEventType` events fan out to `ContainerProfileManager`, `RuleManager`, and a metrics +counter alike (`pkg/containerwatcher/v2/event_handler_factory.go`). `RuleManager` evaluates +rules against each syscall event in real time and drives rule-based alerts through +`pkg/ruleadapters/adapters/syscall.go` (e.g. "unexpected syscall executed"), including +post-learning while `EnableRuntimeDetection` keeps the tracer running. Removing the periodic +schedule entirely (fetching only at `ContainerProfileManager`'s own save points, which happen +every `updateDataPeriod` — default 10 minutes — or at termination) would have delayed that +alerting path by the same amount, which is why `syscallPollInterval` still drives a real, +independent periodic `Peek()` call — it isn't only there for `ContainerProfileManager`. ## Known limitations -`TriggerManualMapFetch` is fire-and-forget — it doesn't block until the fetch completes, and -`flushAndSettle`'s fixed settle delay is a bounded wait for the event pipeline, not a guarantee -tied to the fetch itself. Under severe scheduler contention or a saturated event queue, the -flushed events could in principle still arrive after the delay elapses, in which case termination -falls back to whatever the tracer already had before the flush — the same race as before, just -far less likely to be hit since the flush is triggered immediately rather than waiting for a -30s tick. Achieving an actual guarantee would require synchronously waiting on the specific -resulting events to be processed for this container, which the event pipeline (queue + -worker pool) doesn't currently expose a way to do. +`TriggerManualMapFetch` is fire-and-forget and `flushAndSettle`'s settle delay is a bounded wait +for the event pipeline, not an absolute guarantee tied to the fetch itself. Under severe +scheduler contention or a saturated event queue, the flushed events could in principle still +arrive after the delay elapses; termination would then fall back to whatever the tracer already +had from the last periodic `Peek` (at most `syscallPollInterval` old) — the same kind of race as +before, just far less likely and far smaller in scope than the original 30-second window. diff --git a/pkg/config/config.go b/pkg/config/config.go index 49a47322ab..f66fe352b6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -206,7 +206,7 @@ func LoadConfigOptional(path string, errNotFound bool) (Config, error) { viper.SetDefault("exitCleanup::cleanupInterval", 30*time.Second) viper.SetDefault("exitCleanup::cleanupDelay", 5*time.Minute) viper.SetDefault("updateDataPeriod", 10*time.Minute) - viper.SetDefault("syscallPollInterval", 30*time.Second) + viper.SetDefault("syscallPollInterval", 5*time.Second) viper.SetDefault("workerChannelSize", 750000) viper.SetDefault("blockEvents", false) viper.SetDefault("celConfigCache::maxSize", 100000) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 467f55d2a3..5110872768 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -82,7 +82,7 @@ func TestLoadConfig(t *testing.T) { WorkerChannelSize: 750000, BlockEvents: false, ProcfsScanInterval: 30 * time.Second, - SyscallPollInterval: 30 * time.Second, + SyscallPollInterval: 5 * time.Second, RuleCoolDown: rulecooldown.RuleCooldownConfig{ CooldownDuration: 1 * time.Hour, CooldownAfterCount: 1, diff --git a/pkg/containerwatcher/v2/tracers/syscall.go b/pkg/containerwatcher/v2/tracers/syscall.go index 8da96fd7ff..0b7c085b5f 100644 --- a/pkg/containerwatcher/v2/tracers/syscall.go +++ b/pkg/containerwatcher/v2/tracers/syscall.go @@ -31,7 +31,7 @@ const ( // defaultSyscallPollInterval is used when cfg.SyscallPollInterval is unset (e.g. tests // constructing config.Config directly). It matches the config package's own default. - defaultSyscallPollInterval = 30 * time.Second + defaultSyscallPollInterval = 5 * time.Second ) var _ containerwatcher.TracerInterface = (*SyscallTracer)(nil) @@ -44,6 +44,9 @@ type SyscallTracer struct { kubeManager operators.DataOperator ociStore *orasoci.ReadOnlyStore runtime runtime.Runtime + + // done is closed by Stop to stop the periodic Peek loop started in Start. + done chan struct{} } // NewSyscallTracer creates a new tracer @@ -63,11 +66,8 @@ func NewSyscallTracer( } } -// pollInterval returns the configured eBPF map-fetch interval for the advise_seccomp map, -// falling back to defaultSyscallPollInterval when unset. This interval only bounds staleness -// of the live, in-progress profile; termination-time loss (kubescape/node-agent#922) is instead -// addressed by Peek, which recovers a terminating container's last syscalls immediately rather -// than waiting on this schedule. +// pollInterval returns the configured cadence at which Start's background loop calls Peek, +// falling back to defaultSyscallPollInterval when unset. func (st *SyscallTracer) pollInterval() time.Duration { if st.cfg.SyscallPollInterval > 0 { return st.cfg.SyscallPollInterval @@ -75,15 +75,16 @@ func (st *SyscallTracer) pollInterval() time.Duration { return defaultSyscallPollInterval } -// Peek requests an immediate, out-of-band fetch of the advise_seccomp eBPF map, on top of the -// tracer's normal periodic poll (see pollInterval). It does not block until the fetch completes -// or until any resulting events have been processed by this tracer's eventCallback — callers -// needing that must wait separately afterward. +// Peek requests an immediate, out-of-band fetch of the advise_seccomp eBPF map. It does not +// block until the fetch completes or until any resulting events have been processed by this +// tracer's eventCallback — callers needing that must wait separately afterward. // -// This exists so a caller about to stop tracking a container (ContainerProfileManager, on -// termination or max-sniffing-time) can recover that container's syscalls executed since the -// tracer's last poll, which would otherwise still be sitting unfetched in the kernel map and -// become unreachable once the container's data is removed (kubescape/node-agent#922). +// The gadget instance itself runs with no internal fetch schedule (see Start); every fetch, +// periodic or not, goes through this one call. Start's own background loop calls it on +// pollInterval for live event delivery (profile-building and rule-based alerting alike), and +// ContainerProfileManager calls it once more, out of that schedule, right before it stops +// tracking a container (on termination or max-sniffing-time) to recover that container's last +// syscalls before they become unreachable (kubescape/node-agent#922). func (st *SyscallTracer) Peek() { ebpfoperator.TriggerManualMapFetch(syscallDataSourceName) } @@ -103,24 +104,46 @@ func (st *SyscallTracer) Start(ctx context.Context) error { gadgetcontext.WithName(syscallTraceName), gadgetcontext.WithOrasReadonlyTarget(st.ociStore), ) + st.done = make(chan struct{}) go func() { + // map-fetch-interval "0" together with map-fetch-count "0" (unlimited) disables the + // gadget's own internal fetch schedule entirely: every fetch is driven by Peek instead + // (see the loop below and Peek's doc comment). params := map[string]string{ "operator.oci.ebpf.map-fetch-count": "0", - "operator.oci.ebpf.map-fetch-interval": st.pollInterval().String(), + "operator.oci.ebpf.map-fetch-interval": "0", } err := st.runtime.RunGadget(st.gadgetCtx, nil, params) if err != nil { logger.L().Error("Error running gadget", helpers.String("gadget", st.gadgetCtx.Name()), helpers.Error(err)) } }() + go st.runPeekLoop() return nil } +// runPeekLoop calls Peek every pollInterval until Stop closes st.done. +func (st *SyscallTracer) runPeekLoop() { + ticker := time.NewTicker(st.pollInterval()) + defer ticker.Stop() + for { + select { + case <-st.done: + return + case <-ticker.C: + st.Peek() + } + } +} + // Stop gracefully stops the tracer func (st *SyscallTracer) Stop() error { if st.gadgetCtx != nil { st.gadgetCtx.Cancel() } + if st.done != nil { + close(st.done) + } return nil } diff --git a/pkg/containerwatcher/v2/tracers/syscall_test.go b/pkg/containerwatcher/v2/tracers/syscall_test.go index b8bc0b23d5..ac21206489 100644 --- a/pkg/containerwatcher/v2/tracers/syscall_test.go +++ b/pkg/containerwatcher/v2/tracers/syscall_test.go @@ -22,7 +22,7 @@ func TestSyscallTracerPollInterval(t *testing.T) { want time.Duration }{ {"unset config falls back to default", config.Config{}, defaultSyscallPollInterval}, - {"configured interval is used", config.Config{SyscallPollInterval: 5 * time.Second}, 5 * time.Second}, + {"configured interval is used", config.Config{SyscallPollInterval: 10 * time.Second}, 10 * time.Second}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -39,6 +39,29 @@ func TestSyscallTracerPeekDoesNotPanic(t *testing.T) { assert.NotPanics(t, st.Peek) } +func TestSyscallTracerRunPeekLoopStopsOnDone(t *testing.T) { + st := &SyscallTracer{ + cfg: config.Config{SyscallPollInterval: 5 * time.Millisecond}, + done: make(chan struct{}), + } + + finished := make(chan struct{}) + go func() { + st.runPeekLoop() + close(finished) + }() + + // Let a few ticks fire (each a harmless no-op Peek with no gadget running) before stopping. + time.Sleep(30 * time.Millisecond) + close(st.done) + + select { + case <-finished: + case <-time.After(time.Second): + t.Fatal("runPeekLoop did not return after done was closed") + } +} + func TestSyscallFields(t *testing.T) { expectedFields := map[string][]string{ syscallDataSourceName: { From 583b754b069ef12bf20248968255dc2ff6fe7c8c Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Wed, 26 Aug 2026 08:06:05 +0200 Subject: [PATCH 4/6] 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 --- docs/features/syscall-poll-interval.md | 29 +++++++++++++++-- .../containerprofile_manager_interface.go | 6 +++- .../containerprofile_manager_mock.go | 2 +- .../v1/event_reporting.go | 22 +++++++++---- .../v1/event_reporting_test.go | 19 ++++++++--- .../v2/event_handler_factory.go | 19 ++++++----- pkg/containerwatcher/v2/tracers/syscall.go | 32 +++++++++++++++---- .../v2/tracers/tracer_factory.go | 1 + 8 files changed, 99 insertions(+), 31 deletions(-) diff --git a/docs/features/syscall-poll-interval.md b/docs/features/syscall-poll-interval.md index 0d056340a7..7fd827de31 100644 --- a/docs/features/syscall-poll-interval.md +++ b/docs/features/syscall-poll-interval.md @@ -67,15 +67,38 @@ and *all* fetching — periodic or not — goes through one explicit call: ## Why this isn't a regression risk for alerting -`SyscallEventType` events fan out to `ContainerProfileManager`, `RuleManager`, and a metrics -counter alike (`pkg/containerwatcher/v2/event_handler_factory.go`). `RuleManager` evaluates +`SyscallEventType` events fan out to `RuleManager` and a metrics counter through the generic +per-event pipeline (`pkg/containerwatcher/v2/event_handler_factory.go`). `RuleManager` evaluates rules against each syscall event in real time and drives rule-based alerts through `pkg/ruleadapters/adapters/syscall.go` (e.g. "unexpected syscall executed"), including post-learning while `EnableRuntimeDetection` keeps the tracer running. Removing the periodic schedule entirely (fetching only at `ContainerProfileManager`'s own save points, which happen every `updateDataPeriod` — default 10 minutes — or at termination) would have delayed that alerting path by the same amount, which is why `syscallPollInterval` still drives a real, -independent periodic `Peek()` call — it isn't only there for `ContainerProfileManager`. +independent periodic `Peek()` call — it isn't only there for `ContainerProfileManager` +(see [Batching](#batching-bypassing-the-generic-pipeline-for-containerprofilemanager) below for +why `ContainerProfileManager` is no longer one of that fan-out's targets). + +## Batching: bypassing the generic pipeline for ContainerProfileManager + +`SyscallTracer.callback` decodes one eBPF fetch into a batch of syscall names per container. The +generic pipeline models one event per occurrence, so turning that batch into individual +`SyscallEventType` events (one `AddEventDirect`, dedup-key computation, and 2-handler dispatch +loop each) is pure per-item overhead for a data source that was never actually a discrete event +stream — it's a periodic snapshot of a persistent per-mntns bitmap. That overhead scales with how +many *distinct* syscalls a container has executed since its last fetch, which is 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 — batching can't safely change +that without a breaking rule-schema change (a list field with `in`/`exists` semantics instead of +scalar `==`). `ContainerProfileManager` has no such constraint: it only ever folds each syscall +into a per-container `mapset.Set[string]`, so `SyscallTracer.callback` calls +`ContainerProfileManagerClient.ReportSyscalls(containerID, syscalls []string)` directly — a +single container-lock acquisition and size update for the whole batch — instead of emitting a +separate event per syscall for it to pick up off the queue. `containerProfileManager` is +correspondingly absent from `ehf.handlers[utils.SyscallEventType]`; `RuleManager` and `metrics` +are unaffected and still receive one event per syscall exactly as before. ## Known limitations diff --git a/pkg/containerprofilemanager/containerprofile_manager_interface.go b/pkg/containerprofilemanager/containerprofile_manager_interface.go index 0c15d023a7..60948ce021 100644 --- a/pkg/containerprofilemanager/containerprofile_manager_interface.go +++ b/pkg/containerprofilemanager/containerprofile_manager_interface.go @@ -17,7 +17,11 @@ type ContainerProfileManagerClient interface { ReportSymlinkEvent(containerID string, event utils.LinkEvent) ReportHardlinkEvent(containerID string, event utils.LinkEvent) ReportNetworkEvent(containerID string, event utils.NetworkEvent) - ReportSyscall(containerID string, syscall string) + // ReportSyscalls reports a batch of syscalls for a container in a single operation. The + // syscall tracer bypasses the generic per-event pipeline entirely for this method (see + // SyscallTracer.callback), so this is the only entry point for syscall data - + // ContainerProfileManagerClient intentionally has no single-syscall equivalent. + ReportSyscalls(containerID string, syscalls []string) ReportDroppedEvent(containerID string) RegisterForContainerEndOfLife(notificationChannel chan *containercollection.Container) OnQueueError(profile *v1beta1.ContainerProfile, containerID string, err error) diff --git a/pkg/containerprofilemanager/containerprofile_manager_mock.go b/pkg/containerprofilemanager/containerprofile_manager_mock.go index fff1468566..576a46e46e 100644 --- a/pkg/containerprofilemanager/containerprofile_manager_mock.go +++ b/pkg/containerprofilemanager/containerprofile_manager_mock.go @@ -19,7 +19,7 @@ func (a ContainerProfileManagerMock) ContainerCallback(_ containercollection.Pub // noop } -func (a ContainerProfileManagerMock) ReportSyscall(_ string, _ string) { +func (a ContainerProfileManagerMock) ReportSyscalls(_ string, _ []string) { // noop } diff --git a/pkg/containerprofilemanager/v1/event_reporting.go b/pkg/containerprofilemanager/v1/event_reporting.go index 5ade624c4f..a0b07c5bc9 100644 --- a/pkg/containerprofilemanager/v1/event_reporting.go +++ b/pkg/containerprofilemanager/v1/event_reporting.go @@ -381,18 +381,26 @@ func (cpm *ContainerProfileManager) ReportDroppedEvent(containerID string) { logger.L().Debug("dropped event reported", helpers.String("containerID", containerID)) } -func (cpm *ContainerProfileManager) ReportSyscall(containerID string, syscall string) { +// ReportSyscalls reports a batch of syscalls for a container in a single locked operation - +// one container-lock acquisition and one size update, rather than one per syscall. The eBPF +// syscall tracer decodes a whole periodically- or on-demand-fetched bitmap at once (see +// pkg/containerwatcher/v2/tracers/syscall.go), so this is called with that whole batch directly, +// bypassing the generic per-event pipeline entirely for this consumer (kubescape/node-agent#922). +func (cpm *ContainerProfileManager) ReportSyscalls(containerID string, syscalls []string) { err := cpm.withContainer(containerID, func(data *containerData) (int, error) { if data.syscalls == nil { data.syscalls = mapset.NewSet[string]() } - // Append returns the number of elements newly added (0 or 1 here), not their - // serialized size - using it directly mixed element-count units into a - // byte-size accumulator and made syscalls contribute ~0 to MaxTsProfileSize. - if data.syscalls.Append(syscall) == 0 { - return 0, nil + addedSize := 0 + for _, syscall := range syscalls { + // Append returns the number of elements newly added (0 or 1 here), not their + // serialized size - using it directly mixed element-count units into a + // byte-size accumulator and made syscalls contribute ~0 to MaxTsProfileSize. + if data.syscalls.Append(syscall) > 0 { + addedSize += size.Of(syscall) + } } - return size.Of(syscall), nil + return addedSize, nil }) cpm.logEventError(err, "syscalls", containerID) diff --git a/pkg/containerprofilemanager/v1/event_reporting_test.go b/pkg/containerprofilemanager/v1/event_reporting_test.go index 02fe24ca33..2de5abf703 100644 --- a/pkg/containerprofilemanager/v1/event_reporting_test.go +++ b/pkg/containerprofilemanager/v1/event_reporting_test.go @@ -37,18 +37,29 @@ func newTestManager(t *testing.T, containerID string) (*ContainerProfileManager, return cpm, entry } -func TestReportSyscallSizeAccounting(t *testing.T) { +func TestReportSyscallsSizeAccounting(t *testing.T) { cpm, entry := newTestManager(t, "container1") - cpm.ReportSyscall("container1", "execve") + cpm.ReportSyscalls("container1", []string{"execve"}) assert.Equal(t, int64(size.Of("execve")), entry.data.size.Load(), "size must grow by the syscall's byte size, not by the set's element-count delta") // Re-reporting an already-known syscall is a set-dedup no-op and must not grow the estimate. - cpm.ReportSyscall("container1", "execve") + cpm.ReportSyscalls("container1", []string{"execve"}) assert.Equal(t, int64(size.Of("execve")), entry.data.size.Load()) - cpm.ReportSyscall("container1", "openat") + cpm.ReportSyscalls("container1", []string{"openat"}) + assert.Equal(t, int64(size.Of("execve")+size.Of("openat")), entry.data.size.Load()) +} + +func TestReportSyscallsBatchInOneCall(t *testing.T) { + cpm, entry := newTestManager(t, "container1") + + // A single batch call mixing a duplicate ("execve" appears twice) and a genuinely new + // syscall must dedup within the batch and only charge size for what's actually new. + cpm.ReportSyscalls("container1", []string{"execve", "execve", "openat"}) + + assert.ElementsMatch(t, []string{"execve", "openat"}, entry.data.syscalls.ToSlice()) assert.Equal(t, int64(size.Of("execve")+size.Of("openat")), entry.data.size.Load()) } diff --git a/pkg/containerwatcher/v2/event_handler_factory.go b/pkg/containerwatcher/v2/event_handler_factory.go index 185c719838..4cef61d4ce 100644 --- a/pkg/containerwatcher/v2/event_handler_factory.go +++ b/pkg/containerwatcher/v2/event_handler_factory.go @@ -6,9 +6,6 @@ import ( "time" mapset "github.com/deckarep/golang-set/v2" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/metric" - "github.com/kubescape/node-agent/pkg/otelsetup" "github.com/goradd/maps" containercollection "github.com/inspektor-gadget/inspektor-gadget/pkg/container-collection" "github.com/kubescape/node-agent/pkg/config" @@ -21,8 +18,11 @@ import ( "github.com/kubescape/node-agent/pkg/malwaremanager" "github.com/kubescape/node-agent/pkg/metricsmanager" "github.com/kubescape/node-agent/pkg/networkstream" + "github.com/kubescape/node-agent/pkg/otelsetup" "github.com/kubescape/node-agent/pkg/rulemanager" "github.com/kubescape/node-agent/pkg/utils" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" ) // Manager represents a component that can receive events @@ -146,10 +146,11 @@ func NewEventHandlerFactory( if networkEvent, ok := event.(utils.NetworkEvent); ok { containerProfileManager.ReportNetworkEvent(networkEvent.GetContainerID(), networkEvent) } - case utils.SyscallEventType: - if syscallEvent, ok := event.(utils.SyscallEvent); ok { - containerProfileManager.ReportSyscall(syscallEvent.GetContainerID(), syscallEvent.GetSyscall()) - } + // SyscallEventType is intentionally absent: SyscallTracer reports syscalls to + // ContainerProfileManager directly, as a batch, bypassing this generic per-event + // pipeline entirely (see ContainerProfileManagerClient.ReportSyscalls, + // kubescape/node-agent#922). RuleManager and metrics still receive individual + // SyscallEventType events through the handlers table below for rule matching. default: // For event types that don't have specific handling, we might need to add them // or handle them generically @@ -419,7 +420,9 @@ func (ehf *EventHandlerFactory) registerHandlers( ehf.handlers[utils.IoUringEventType] = []Manager{ruleManager, metrics, rulePolicy} // Syscall events - ehf.handlers[utils.SyscallEventType] = []Manager{containerProfileManager, ruleManager, metrics} + // containerProfileManager is deliberately excluded here; see the comment on + // SyscallEventType's absence from containerProfileAdapter above. + ehf.handlers[utils.SyscallEventType] = []Manager{ruleManager, metrics} // Kmod events ehf.handlers[utils.KmodEventType] = []Manager{ruleManager, metrics} diff --git a/pkg/containerwatcher/v2/tracers/syscall.go b/pkg/containerwatcher/v2/tracers/syscall.go index 0b7c085b5f..70b10a3fce 100644 --- a/pkg/containerwatcher/v2/tracers/syscall.go +++ b/pkg/containerwatcher/v2/tracers/syscall.go @@ -45,6 +45,11 @@ type SyscallTracer struct { ociStore *orasoci.ReadOnlyStore runtime runtime.Runtime + // reportSyscalls is called once per fetch with the whole decoded batch for a container, + // bypassing the generic per-event pipeline entirely for this consumer (see callback and + // ContainerProfileManagerClient.ReportSyscalls, kubescape/node-agent#922). + reportSyscalls func(containerID string, syscalls []string) + // done is closed by Stop to stop the periodic Peek loop started in Start. done chan struct{} } @@ -56,13 +61,15 @@ func NewSyscallTracer( ociStore *orasoci.ReadOnlyStore, eventCallback containerwatcher.ResultCallback, cfg config.Config, + reportSyscalls func(containerID string, syscalls []string), ) *SyscallTracer { return &SyscallTracer{ - cfg: cfg, - eventCallback: eventCallback, - kubeManager: kubeManager, - ociStore: ociStore, - runtime: runtime, + cfg: cfg, + eventCallback: eventCallback, + kubeManager: kubeManager, + ociStore: ociStore, + runtime: runtime, + reportSyscalls: reportSyscalls, } } @@ -187,8 +194,19 @@ func (st *SyscallTracer) callback(event *utils.DatasourceEvent) { containerID := event.GetContainerID() processID := event.GetPID() - syscallsBuffer := event.GetSyscalls() - for _, syscall := range decodeSyscalls(syscallsBuffer) { + syscallList := decodeSyscalls(event.GetSyscalls()) + if len(syscallList) == 0 { + event.Release() + return + } + + // Report the whole batch directly, bypassing the generic per-event pipeline entirely for + // this consumer (see reportSyscalls' doc comment). + st.reportSyscalls(containerID, syscallList) + + // RuleManager and metrics still need one event per syscall (rule matching keys off a + // single event.syscall field), so those keep going through the normal event pipeline. + for _, syscall := range syscallList { st.eventCallback(&utils.DatasourceEvent{ Data: event.Datasource.DeepCopy(event.Data), Datasource: event.Datasource, diff --git a/pkg/containerwatcher/v2/tracers/tracer_factory.go b/pkg/containerwatcher/v2/tracers/tracer_factory.go index 7bdc694fa9..15d0370816 100644 --- a/pkg/containerwatcher/v2/tracers/tracer_factory.go +++ b/pkg/containerwatcher/v2/tracers/tracer_factory.go @@ -111,6 +111,7 @@ func (tf *TracerFactory) CreateAllTracers(manager containerwatcher.TracerRegistr tf.ociStore, tf.createEventCallback(utils.SyscallEventType), tf.cfg, + tf.containerProfileManager.ReportSyscalls, ) manager.RegisterTracer(syscallTracer) if syscallTracer.IsEnabled(tf.cfg) { From 2acc7752ae2bbe22813df96d9e50d25b0915ba00 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Wed, 26 Aug 2026 09:10:44 +0200 Subject: [PATCH 5/6] =?UTF-8?q?fix(containerprofile):=20address=20review?= =?UTF-8?q?=20=E2=80=94=20flusher=20race,=20blocking=20dispatch,=20empty?= =?UTF-8?q?=20containerID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/features/syscall-poll-interval.md | 30 +++++++++++++++++++ pkg/config/config.go | 8 ++++- .../v1/containerprofile_manager.go | 10 +++++-- .../v1/containerprofile_manager_test.go | 29 ++++++++++++++++-- pkg/containerprofilemanager/v1/helpers.go | 5 ++-- pkg/containerwatcher/v2/tracers/syscall.go | 26 ++++++++++------ .../v2/tracers/syscall_test.go | 2 +- 7 files changed, 91 insertions(+), 19 deletions(-) diff --git a/docs/features/syscall-poll-interval.md b/docs/features/syscall-poll-interval.md index 7fd827de31..09cc639c6d 100644 --- a/docs/features/syscall-poll-interval.md +++ b/docs/features/syscall-poll-interval.md @@ -100,6 +100,36 @@ separate event per syscall for it to pick up off the queue. `containerProfileMan correspondingly absent from `ehf.handlers[utils.SyscallEventType]`; `RuleManager` and `metrics` are unaffected and still receive one event per syscall exactly as before. +Two things this direct call requires that the generic pipeline used to provide for free: + +- **Filtering out unresolved container IDs.** The map covers every mount namespace on the node, + including ones never resolved to a container (host processes, etc.) — + `EventHandlerFactory.ProcessEvent`'s `if enrichedEvent.ContainerID == "" { return }` used to + drop those before they ever reached `ReportSyscall`. `callback` now does that check itself + before calling `ReportSyscalls`, to avoid spamming `ContainerProfileManager.withContainer`'s + "invalid empty containerID" error on every such row, every tick. +- **Not blocking the shared fetch-processing path.** Previously `ReportSyscall` reached + `ContainerProfileManager` through the generic queue and one of `WorkerPoolSize` pooled + goroutines, so a slow or blocked container only ever tied up one interchangeable worker. + `callback` is invoked synchronously, once per currently-traced container, by the single + goroutine that processes each fetch — and `ContainerProfileManager.ReportSyscalls` can itself + block (`withContainer` sends on a container's `SyncChannel`, which is bounded, while holding + that container's lock, when a profile crosses `MaxTsProfileSize`). `callback` therefore + dispatches `ReportSyscalls` on its own goroutine rather than calling it inline, so one + container's backpressure can never stall the processing of every other container's syscalls + for that tick, or the next `Peek`-triggered fetch. + +## Concurrency + +`ContainerProfileManager.syscallFlusher` (the callback `SetSyscallFlusher` registers) is stored +via `atomic.Pointer[func()]`, not a plain field. `SetSyscallFlusher` is called once during +startup wiring (`TracerFactory.CreateAllTracers`), but by that point `StartContainerCollection` +has already begun enumerating existing containers and starting their `monitorContainer` +goroutines — a container that starts and terminates in that window would call `flushAndSettle` +(reading `syscallFlusher`) concurrently with that write. A plain field would be an unsynchronized +data race there, silently able to reintroduce #922's exact bug at exactly the highest-churn +moment there is: node-agent startup or restart. + ## Known limitations `TriggerManualMapFetch` is fire-and-forget and `flushAndSettle`'s settle delay is a bounded wait diff --git a/pkg/config/config.go b/pkg/config/config.go index f66fe352b6..1d7a38a673 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -29,6 +29,12 @@ const NodeNameEnvVar = "NODE_NAME" const PodNameEnvVar = "POD_NAME" const NamespaceEnvVar = "NAMESPACE_NAME" +// DefaultSyscallPollInterval is SyscallPollInterval's default, matching the pre-PR-#591 +// RuleManager alerting cadence (see pkg/containerwatcher/v2/tracers/syscall.go). Exported so +// SyscallTracer's own fallback (used when constructed with a zero-value config.Config, e.g. in +// tests) can reference the same value instead of duplicating the literal. +const DefaultSyscallPollInterval = 5 * time.Second + // EventDedupConfig controls eBPF event deduplication before CEL rule evaluation. // ProfileProjectionConfig controls rule-aware profile projection behaviour. type ProfileProjectionConfig struct { @@ -206,7 +212,7 @@ func LoadConfigOptional(path string, errNotFound bool) (Config, error) { viper.SetDefault("exitCleanup::cleanupInterval", 30*time.Second) viper.SetDefault("exitCleanup::cleanupDelay", 5*time.Minute) viper.SetDefault("updateDataPeriod", 10*time.Minute) - viper.SetDefault("syscallPollInterval", 5*time.Second) + viper.SetDefault("syscallPollInterval", DefaultSyscallPollInterval) viper.SetDefault("workerChannelSize", 750000) viper.SetDefault("blockEvents", false) viper.SetDefault("celConfigCache::maxSize", 100000) diff --git a/pkg/containerprofilemanager/v1/containerprofile_manager.go b/pkg/containerprofilemanager/v1/containerprofile_manager.go index efa58938a7..c41e44e4f4 100644 --- a/pkg/containerprofilemanager/v1/containerprofile_manager.go +++ b/pkg/containerprofilemanager/v1/containerprofile_manager.go @@ -100,8 +100,12 @@ type ContainerProfileManager struct { // syscallFlusher, if set, is called right before a container's final forced profile // save to request an immediate out-of-band fetch of its not-yet-polled syscalls. See - // SetSyscallFlusher and kubescape/node-agent#922. - syscallFlusher func() + // SetSyscallFlusher and kubescape/node-agent#922. Stored via atomic.Pointer: it's written + // once during startup wiring (TracerFactory.CreateAllTracers, after StartContainerCollection + // has already begun enumerating containers), and flushAndSettle reads it from whatever + // per-container monitorContainer goroutine happens to be running at that time - a plain + // field would be an unsynchronized data race between that write and those concurrent reads. + syscallFlusher atomic.Pointer[func()] } func (cpm *ContainerProfileManager) SetCompletionNotifier(n objectcache.CompletionNotifier) { @@ -110,7 +114,7 @@ func (cpm *ContainerProfileManager) SetCompletionNotifier(n objectcache.Completi // SetSyscallFlusher implements containerprofilemanager.ContainerProfileManagerClient. func (cpm *ContainerProfileManager) SetSyscallFlusher(flush func()) { - cpm.syscallFlusher = flush + cpm.syscallFlusher.Store(&flush) } func (cpm *ContainerProfileManager) notifyCompleted(containerID string) { diff --git a/pkg/containerprofilemanager/v1/containerprofile_manager_test.go b/pkg/containerprofilemanager/v1/containerprofile_manager_test.go index f621cfbeb7..c4338e421c 100644 --- a/pkg/containerprofilemanager/v1/containerprofile_manager_test.go +++ b/pkg/containerprofilemanager/v1/containerprofile_manager_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/url" "os" + "sync" "testing" "time" @@ -19,6 +20,7 @@ import ( "github.com/kubescape/node-agent/pkg/utils" "github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNetworkEventPodLabels(t *testing.T) { @@ -633,16 +635,37 @@ func TestMaxWaitForSharedContainerDataConstant(t *testing.T) { func TestSetSyscallFlusher(t *testing.T) { cpm := &ContainerProfileManager{} - assert.Nil(t, cpm.syscallFlusher) + assert.Nil(t, cpm.syscallFlusher.Load()) called := false cpm.SetSyscallFlusher(func() { called = true }) - assert.NotNil(t, cpm.syscallFlusher) + flush := cpm.syscallFlusher.Load() + require.NotNil(t, flush) - cpm.syscallFlusher() + (*flush)() assert.True(t, called) } +// TestSetSyscallFlusherConcurrentWithFlushAndSettle exercises the exact race the review on +// PR #924 flagged: SetSyscallFlusher (a startup-time write, from TracerFactory) racing with +// flushAndSettle reads from an already-running per-container monitorContainer goroutine. Run +// with -race; a plain (non-atomic) field would be flagged here. +func TestSetSyscallFlusherConcurrentWithFlushAndSettle(t *testing.T) { + cpm := &ContainerProfileManager{} + + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + cpm.SetSyscallFlusher(func() {}) + }() + go func() { + defer wg.Done() + cpm.flushAndSettle() + }() + wg.Wait() +} + func TestFlushAndSettleNoFlusherIsNoopAndFast(t *testing.T) { cpm := &ContainerProfileManager{} start := time.Now() diff --git a/pkg/containerprofilemanager/v1/helpers.go b/pkg/containerprofilemanager/v1/helpers.go index 0c0818b31b..9c40b404c2 100644 --- a/pkg/containerprofilemanager/v1/helpers.go +++ b/pkg/containerprofilemanager/v1/helpers.go @@ -45,10 +45,11 @@ const ( // a container terminating between two polls would silently lose whatever it executed since the // last one once this manager removes its data (kubescape/node-agent#922). func (cpm *ContainerProfileManager) flushAndSettle() { - if cpm.syscallFlusher == nil { + flush := cpm.syscallFlusher.Load() + if flush == nil { return } - cpm.syscallFlusher() + (*flush)() time.Sleep(postSyscallFlushSettleDelay) } diff --git a/pkg/containerwatcher/v2/tracers/syscall.go b/pkg/containerwatcher/v2/tracers/syscall.go index 70b10a3fce..590dcb94a2 100644 --- a/pkg/containerwatcher/v2/tracers/syscall.go +++ b/pkg/containerwatcher/v2/tracers/syscall.go @@ -28,10 +28,6 @@ const ( // syscallDataSourceName is the advise_seccomp gadget's map-iterator datasource name // (see TestSyscallFields). Peek uses it to target only this tracer's map iterator. syscallDataSourceName = "syscalls" - - // defaultSyscallPollInterval is used when cfg.SyscallPollInterval is unset (e.g. tests - // constructing config.Config directly). It matches the config package's own default. - defaultSyscallPollInterval = 5 * time.Second ) var _ containerwatcher.TracerInterface = (*SyscallTracer)(nil) @@ -74,12 +70,12 @@ func NewSyscallTracer( } // pollInterval returns the configured cadence at which Start's background loop calls Peek, -// falling back to defaultSyscallPollInterval when unset. +// falling back to config.DefaultSyscallPollInterval when unset. func (st *SyscallTracer) pollInterval() time.Duration { if st.cfg.SyscallPollInterval > 0 { return st.cfg.SyscallPollInterval } - return defaultSyscallPollInterval + return config.DefaultSyscallPollInterval } // Peek requests an immediate, out-of-band fetch of the advise_seccomp eBPF map. It does not @@ -194,6 +190,15 @@ func (st *SyscallTracer) callback(event *utils.DatasourceEvent) { containerID := event.GetContainerID() processID := event.GetPID() + // The map covers every mount namespace on the node, including ones not (yet, or ever) + // resolved to a container - e.g. host processes. The generic event pipeline drops those + // itself (EventHandlerFactory.ProcessEvent's empty-ContainerID check), but reportSyscalls + // is called directly, bypassing that check, so it must be done here instead. + if containerID == "" { + event.Release() + return + } + syscallList := decodeSyscalls(event.GetSyscalls()) if len(syscallList) == 0 { event.Release() @@ -201,8 +206,11 @@ func (st *SyscallTracer) callback(event *utils.DatasourceEvent) { } // Report the whole batch directly, bypassing the generic per-event pipeline entirely for - // this consumer (see reportSyscalls' doc comment). - st.reportSyscalls(containerID, syscallList) + // this consumer (see reportSyscalls' doc comment). Dispatched on its own goroutine so a + // slow or momentarily-blocked container (e.g. withContainer's SyncChannel send on a + // profile-size-split signal) can never stall this callback, which the gadget's shared + // fetch-processing goroutine calls synchronously for every currently traced container. + go st.reportSyscalls(containerID, syscallList) // RuleManager and metrics still need one event per syscall (rule matching keys off a // single event.syscall field), so those keep going through the normal event pipeline. @@ -219,7 +227,7 @@ func (st *SyscallTracer) callback(event *utils.DatasourceEvent) { } func decodeSyscalls(syscallsBuffer []byte) []string { - syscallStrings := make([]string, 0) + syscallStrings := make([]string, 0, len(syscallsBuffer)) // Syscall numbers this build cannot resolve to a name are skipped. Recording a // placeholder name would propagate into the application profile and from there // into generated seccomp profiles, where it is not a valid syscall name. The diff --git a/pkg/containerwatcher/v2/tracers/syscall_test.go b/pkg/containerwatcher/v2/tracers/syscall_test.go index ac21206489..53e48f6623 100644 --- a/pkg/containerwatcher/v2/tracers/syscall_test.go +++ b/pkg/containerwatcher/v2/tracers/syscall_test.go @@ -21,7 +21,7 @@ func TestSyscallTracerPollInterval(t *testing.T) { cfg config.Config want time.Duration }{ - {"unset config falls back to default", config.Config{}, defaultSyscallPollInterval}, + {"unset config falls back to default", config.Config{}, config.DefaultSyscallPollInterval}, {"configured interval is used", config.Config{SyscallPollInterval: 10 * time.Second}, 10 * time.Second}, } for _, tt := range tests { From 9effbc78798fbe98c57068fff0fd43591d532061 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Wed, 26 Aug 2026 09:56:38 +0200 Subject: [PATCH 6/6] 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 --- docs/features/syscall-poll-interval.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/features/syscall-poll-interval.md b/docs/features/syscall-poll-interval.md index 09cc639c6d..b41cb85aea 100644 --- a/docs/features/syscall-poll-interval.md +++ b/docs/features/syscall-poll-interval.md @@ -61,9 +61,13 @@ and *all* fetching — periodic or not — goes through one explicit call: termination paths in monitoring.go) — closing even the up-to-`syscallPollInterval` gap the periodic schedule alone would still leave at exactly the moment a container disappears. `flushAndSettle` waits a fixed `postSyscallFlushSettleDelay` (`500ms`) afterward for the - resulting events to travel through the event queue/worker pool into the container's data - before it's snapshotted and cleared. This wiring only happens when the syscall tracer is - actually enabled (`SyscallTracer.IsEnabled`), so a disabled tracer costs nothing. + resulting data to land before the profile is snapshotted and cleared. `ContainerProfileManager` + no longer goes through the generic event queue/worker pool for syscalls at all (see + [Batching](#batching-bypassing-the-generic-pipeline-for-containerprofilemanager) below); the + wait covers `SyscallTracer.callback`'s own dispatch goroutine and any per-container + `SyncChannel` backpressure in `ContainerProfileManager.ReportSyscalls`, not queue latency. This + wiring only happens when the syscall tracer is actually enabled (`SyscallTracer.IsEnabled`), so + a disabled tracer costs nothing. ## Why this isn't a regression risk for alerting