From bb9676d3449c30ec8f71da44787b92f02bebb86a Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Wed, 19 Aug 2026 08:41:14 +0200 Subject: [PATCH 1/2] fix(tracers): bounded retry for DNSTracer gadget startup trace_dns's Start() only logged a failed RunGadget call and never retried, so a single transient failure permanently stopped DNS event collection (utils.DnsEventType) until the node-agent pod restarted. That failure class is inherently transient: armosec/private-node-agent#511 root-caused an intermittent "apply CO-RE relocations: load BTF for kmod kvm_intel: rebase split spec: raw BTF differs" coming from a cache race in the pinned cilium/ebpf fork's CO-RE/BTF loading layer, unrelated to trace_dns itself. A fix for that race is proposed upstream (matthyx/ebpf#1), but any gadget startup failure in this class - CO-RE relocation racing other concurrently-loading gadgets - is timing-dependent by nature, so retrying is worthwhile independent of that fix. Wrap the existing background RunGadget call in a bounded retry (dnsStartMaxRetries = 5) using this repo's existing github.com/cenkalti/backoff convention (see containercallback.go's setSharedWatchedContainerData), with each attempt getting its own GadgetContext (the previous one is left terminated after a failed run) and retries stopping promptly once the tracer's context is canceled (Stop()). Scope note: only dns.go is touched. The sibling files #511 mentions (http.go, gotls.go, ssl.go, pkg/tracermanager/tailcalls.go) either don't exist in this OSS repo (gotls.go/ssl.go/tailcalls.go are armosec/private-node-agent's own custom TLS tracers, using a different attach mechanism not exposed to this CO-RE/BTF race) or weren't part of this issue's evidence, so they're left alone. Docs-exempt: internal resilience fix to one tracer's Start(), no documented behavior or public API change. Signed-off-by: Matthias Bertschy --- pkg/containerwatcher/v2/tracers/dns.go | 65 +++++++++++-- .../v2/tracers/dns_retry_test.go | 94 +++++++++++++++++++ 2 files changed, 150 insertions(+), 9 deletions(-) create mode 100644 pkg/containerwatcher/v2/tracers/dns_retry_test.go diff --git a/pkg/containerwatcher/v2/tracers/dns.go b/pkg/containerwatcher/v2/tracers/dns.go index 70775a124d..a031bc30d6 100644 --- a/pkg/containerwatcher/v2/tracers/dns.go +++ b/pkg/containerwatcher/v2/tracers/dns.go @@ -2,7 +2,10 @@ package tracers import ( "context" + "sync" + "time" + "github.com/cenkalti/backoff" "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" @@ -21,14 +24,24 @@ import ( const ( dnsImageName = "ghcr.io/inspektor-gadget/gadget/trace_dns:v0.48.1" dnsTraceName = "trace_dns" + + // dnsStartMaxRetries bounds retries of a failed gadget start. Startup + // failures in this class are transient/timing-dependent (see + // armosec/private-node-agent#511: a CO-RE/BTF cache race in the pinned + // cilium/ebpf fork can abort relocation for any kernel module with + // split BTF present at that moment), so a single failed attempt does + // not mean the gadget can never start. + dnsStartMaxRetries = 5 ) var _ containerwatcher.TracerInterface = (*DNSTracer)(nil) // DNSTracer implements TracerInterface for events type DNSTracer struct { - eventCallback containerwatcher.ResultCallback - gadgetCtx *gadgetcontext.GadgetContext + eventCallback containerwatcher.ResultCallback + gadgetCtxMu sync.Mutex + gadgetCtx *gadgetcontext.GadgetContext + kubeManager operators.DataOperator ociStore *orasoci.ReadOnlyStore runtime runtime.Runtime @@ -55,9 +68,11 @@ func NewDNSTracer( } } -// Start initializes and starts the tracer -func (dt *DNSTracer) Start(ctx context.Context) error { - dt.gadgetCtx = gadgetcontext.New( +// newGadgetContext builds a fresh GadgetContext for a single run attempt and +// records it as the tracer's current one, so Stop() always cancels whichever +// attempt is in flight. +func (dt *DNSTracer) newGadgetContext(ctx context.Context) *gadgetcontext.GadgetContext { + gadgetCtx := gadgetcontext.New( ctx, // This is the image that contains the gadget we want to run. dnsImageName, @@ -72,13 +87,40 @@ func (dt *DNSTracer) Start(ctx context.Context) error { gadgetcontext.WithName(dnsTraceName), gadgetcontext.WithOrasReadonlyTarget(dt.ociStore), ) + + dt.gadgetCtxMu.Lock() + dt.gadgetCtx = gadgetCtx + dt.gadgetCtxMu.Unlock() + + return gadgetCtx +} + +// Start initializes and starts the tracer +func (dt *DNSTracer) Start(ctx context.Context) error { + dt.newGadgetContext(ctx) + go func() { params := map[string]string{ "operator.oci.ebpf.paths": "true", // CWD paths in events } - err := dt.runtime.RunGadget(dt.gadgetCtx, nil, params) + + attempt := 0 + bo := backoff.WithContext(backoff.WithMaxRetries(backoff.NewExponentialBackOff(), dnsStartMaxRetries-1), ctx) + err := backoff.RetryNotify(func() error { + attempt++ + // Each attempt gets its own GadgetContext: a failed run leaves + // the previous one terminated, and it also becomes the current + // one Stop() will cancel. + gadgetCtx := dt.newGadgetContext(ctx) + return dt.runtime.RunGadget(gadgetCtx, nil, params) + }, bo, func(err error, next time.Duration) { + logger.L().Warning("Error running gadget, retrying", + helpers.String("gadget", dnsTraceName), + helpers.Int("attempt", attempt), + helpers.Error(err)) + }) if err != nil { - logger.L().Error("Error running gadget", helpers.String("gadget", dt.gadgetCtx.Name()), helpers.Error(err)) + logger.L().Error("Error running gadget", helpers.String("gadget", dnsTraceName), helpers.Int("attempts", attempt), helpers.Error(err)) } }() return nil @@ -89,8 +131,13 @@ func (dt *DNSTracer) Stop() error { if dt.socketEnricherOp != nil { dt.socketEnricherOp.Close() } - if dt.gadgetCtx != nil { - dt.gadgetCtx.Cancel() + + dt.gadgetCtxMu.Lock() + gadgetCtx := dt.gadgetCtx + dt.gadgetCtxMu.Unlock() + + if gadgetCtx != nil { + gadgetCtx.Cancel() } return nil } diff --git a/pkg/containerwatcher/v2/tracers/dns_retry_test.go b/pkg/containerwatcher/v2/tracers/dns_retry_test.go new file mode 100644 index 0000000000..7bdb8b0827 --- /dev/null +++ b/pkg/containerwatcher/v2/tracers/dns_retry_test.go @@ -0,0 +1,94 @@ +package tracers + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/inspektor-gadget/inspektor-gadget/pkg/gadget-service/api" + "github.com/inspektor-gadget/inspektor-gadget/pkg/params" + "github.com/inspektor-gadget/inspektor-gadget/pkg/runtime" + "github.com/stretchr/testify/require" +) + +// fakeRuntime is a minimal runtime.Runtime that fails RunGadget a +// configurable number of times before succeeding, to exercise +// DNSTracer.Start's bounded retry (armosec/private-node-agent#511: +// gadget startup failures in this class are transient/timing-dependent). +type fakeRuntime struct { + calls int32 + failCount int32 // number of leading calls that return an error +} + +func (f *fakeRuntime) Init(*params.Params) error { return nil } +func (f *fakeRuntime) Close() error { return nil } +func (f *fakeRuntime) GlobalParamDescs() params.ParamDescs { return nil } +func (f *fakeRuntime) ParamDescs() params.ParamDescs { return nil } +func (f *fakeRuntime) SetDefaultValue(params.ValueHint, string) {} +func (f *fakeRuntime) GetDefaultValue(params.ValueHint) (string, bool) { return "", false } +func (f *fakeRuntime) IsClient() bool { return false } +func (f *fakeRuntime) GetGadgetInfo(runtime.GadgetContext, *params.Params, api.ParamValues) (*api.GadgetInfo, error) { + return nil, nil +} + +func (f *fakeRuntime) RunGadget(_ runtime.GadgetContext, _ *params.Params, _ api.ParamValues) error { + n := atomic.AddInt32(&f.calls, 1) + if n <= f.failCount { + return errors.New("simulated transient CO-RE/BTF race") + } + return nil +} + +func TestDNSTracerStartRetriesOnTransientFailure(t *testing.T) { + fr := &fakeRuntime{failCount: dnsStartMaxRetries - 1} // fail every attempt but the last + dt := NewDNSTracer(nil, fr, nil, nil, nil, nil) + + require.NoError(t, dt.Start(context.Background())) + + require.Eventually(t, func() bool { + return atomic.LoadInt32(&fr.calls) == int32(dnsStartMaxRetries) + }, 15*time.Second, 20*time.Millisecond, "expected the tracer to retry until the final attempt succeeds") + + require.NoError(t, dt.Stop()) +} + +func TestDNSTracerStartGivesUpAfterMaxRetries(t *testing.T) { + fr := &fakeRuntime{failCount: dnsStartMaxRetries + 10} // always fail + dt := NewDNSTracer(nil, fr, nil, nil, nil, nil) + + require.NoError(t, dt.Start(context.Background())) + + require.Eventually(t, func() bool { + return atomic.LoadInt32(&fr.calls) == int32(dnsStartMaxRetries) + }, 15*time.Second, 20*time.Millisecond, "expected the tracer to stop after dnsStartMaxRetries attempts") + + // Give any (incorrect) further retry a chance to happen, then confirm + // the bound was actually respected. + time.Sleep(500 * time.Millisecond) + require.Equal(t, int32(dnsStartMaxRetries), atomic.LoadInt32(&fr.calls)) + + require.NoError(t, dt.Stop()) +} + +func TestDNSTracerStartStopsRetryingWhenContextCanceled(t *testing.T) { + fr := &fakeRuntime{failCount: dnsStartMaxRetries + 10} // always fail + dt := NewDNSTracer(nil, fr, nil, nil, nil, nil) + + ctx, cancel := context.WithCancel(context.Background()) + require.NoError(t, dt.Start(ctx)) + + require.Eventually(t, func() bool { + return atomic.LoadInt32(&fr.calls) >= 1 + }, 5*time.Second, 10*time.Millisecond, "expected at least one attempt") + + cancel() + + callsAtCancel := atomic.LoadInt32(&fr.calls) + time.Sleep(1 * time.Second) + require.LessOrEqual(t, atomic.LoadInt32(&fr.calls), callsAtCancel+1, + "expected retries to stop shortly after the context is canceled") + + require.NoError(t, dt.Stop()) +} From 3340514f47b6d326c75d97e49ca399dd60499f58 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Wed, 19 Aug 2026 14:40:55 +0200 Subject: [PATCH 2/2] fix(tracers): make DNSTracer.Stop reliably stop retries Stop only canceled the currently in-flight GadgetContext, not the retry loop's own backoff.WithContext(bo, ctx) - that ctx was the raw one passed to Start, which Stop never touched. So canceling the current attempt just made that one RunGadget call fail; RetryNotify saw its context still alive and started another attempt from a fresh GadgetContext Stop never touches either, i.e. Stop could leave the tracer starting the DNS gadget again after the caller believed it was stopped - worse than the pre-retry behavior, which had nothing to race with Stop at all. Give Start its own cancelable context (derived from the one it's called with) and store the CancelFunc. Use that context - not the raw Start ctx - for backoff.WithContext and every newGadgetContext call, and have Stop cancel it before canceling the current GadgetContext. After the in-flight attempt's RunGadget call returns, RetryNotify sees the canceled context and stops instead of scheduling another attempt. Also drops Start's initial newGadgetContext call: it existed to keep dt.gadgetCtx non-nil during the narrow window between Start returning and the goroutine's first attempt, but with the retry-loop context now canceled synchronously inside Stop, any GadgetContext built during that same window already inherits a canceled context regardless of whether this call ever ran, so keeping it was YAGNI. TestDNSTracerStopStopsRetryingMidBackoff reproduces the bug (fails on the pre-fix code: a second RunGadget call happens after Stop returns) and passes with the fix (Stop leaves the call count unchanged). Reported-by: matthyx in PR review https://github.com/kubescape/node-agent/pull/906#discussion_r3812924145 Docs-exempt: bug fix to unreleased internal retry logic added earlier in this same PR, no documented behavior or public API change. Signed-off-by: Matthias Bertschy --- pkg/containerwatcher/v2/tracers/dns.go | 35 ++++++++++++++----- .../v2/tracers/dns_retry_test.go | 24 +++++++++++++ 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/pkg/containerwatcher/v2/tracers/dns.go b/pkg/containerwatcher/v2/tracers/dns.go index a031bc30d6..3ff46aaa12 100644 --- a/pkg/containerwatcher/v2/tracers/dns.go +++ b/pkg/containerwatcher/v2/tracers/dns.go @@ -39,8 +39,12 @@ var _ containerwatcher.TracerInterface = (*DNSTracer)(nil) // DNSTracer implements TracerInterface for events type DNSTracer struct { eventCallback containerwatcher.ResultCallback - gadgetCtxMu sync.Mutex - gadgetCtx *gadgetcontext.GadgetContext + + mu sync.Mutex + gadgetCtx *gadgetcontext.GadgetContext + // cancel stops the retry loop started by Start, independently of + // whichever GadgetContext is currently in flight. See Stop. + cancel context.CancelFunc kubeManager operators.DataOperator ociStore *orasoci.ReadOnlyStore @@ -88,16 +92,22 @@ func (dt *DNSTracer) newGadgetContext(ctx context.Context) *gadgetcontext.Gadget gadgetcontext.WithOrasReadonlyTarget(dt.ociStore), ) - dt.gadgetCtxMu.Lock() + dt.mu.Lock() dt.gadgetCtx = gadgetCtx - dt.gadgetCtxMu.Unlock() + dt.mu.Unlock() return gadgetCtx } // Start initializes and starts the tracer func (dt *DNSTracer) Start(ctx context.Context) error { - dt.newGadgetContext(ctx) + // The retry loop gets its own cancelable context, independent of + // whichever GadgetContext is currently in flight, so Stop can halt + // retries even between attempts (see Stop). + ctx, cancel := context.WithCancel(ctx) + dt.mu.Lock() + dt.cancel = cancel + dt.mu.Unlock() go func() { params := map[string]string{ @@ -132,10 +142,19 @@ func (dt *DNSTracer) Stop() error { dt.socketEnricherOp.Close() } - dt.gadgetCtxMu.Lock() + dt.mu.Lock() + cancel := dt.cancel gadgetCtx := dt.gadgetCtx - dt.gadgetCtxMu.Unlock() - + dt.mu.Unlock() + + // Stop the retry loop first: canceling only the in-flight GadgetContext + // below would make that one attempt fail, but RetryNotify would then + // start a new one from a fresh, uncanceled context - retrying after + // Stop was called. Canceling this context makes the backoff wrapper + // give up instead. + if cancel != nil { + cancel() + } if gadgetCtx != nil { gadgetCtx.Cancel() } diff --git a/pkg/containerwatcher/v2/tracers/dns_retry_test.go b/pkg/containerwatcher/v2/tracers/dns_retry_test.go index 7bdb8b0827..4cf5b4e5f4 100644 --- a/pkg/containerwatcher/v2/tracers/dns_retry_test.go +++ b/pkg/containerwatcher/v2/tracers/dns_retry_test.go @@ -72,6 +72,30 @@ func TestDNSTracerStartGivesUpAfterMaxRetries(t *testing.T) { require.NoError(t, dt.Stop()) } +func TestDNSTracerStopStopsRetryingMidBackoff(t *testing.T) { + fr := &fakeRuntime{failCount: dnsStartMaxRetries + 10} // always fail + dt := NewDNSTracer(nil, fr, nil, nil, nil, nil) + + require.NoError(t, dt.Start(context.Background())) + + require.Eventually(t, func() bool { + return atomic.LoadInt32(&fr.calls) >= 1 + }, 5*time.Second, 10*time.Millisecond, "expected at least one attempt") + + // Stop while an attempt is in flight or the loop is waiting between + // attempts - not after the context passed to Start was ever canceled + // by the caller, only by Stop itself. Before the fix, Stop only + // canceled the in-flight GadgetContext: that made the current attempt + // fail, but the retry loop's own context was untouched, so + // RetryNotify started yet another attempt anyway. + require.NoError(t, dt.Stop()) + + callsAtStop := atomic.LoadInt32(&fr.calls) + time.Sleep(1 * time.Second) + require.Equal(t, callsAtStop, atomic.LoadInt32(&fr.calls), + "expected no further RunGadget calls after Stop") +} + func TestDNSTracerStartStopsRetryingWhenContextCanceled(t *testing.T) { fr := &fakeRuntime{failCount: dnsStartMaxRetries + 10} // always fail dt := NewDNSTracer(nil, fr, nil, nil, nil, nil)