fix(tracers): bounded retry for DNSTracer gadget startup

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,28 @@ 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
+
+	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
 	runtime            runtime.Runtime
@@ -55,9 +72,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 +91,46 @@ func (dt *DNSTracer) Start(ctx context.Context) error {
 		gadgetcontext.WithName(dnsTraceName),
 		gadgetcontext.WithOrasReadonlyTarget(dt.ociStore),
 	)
+
+	dt.mu.Lock()
+	dt.gadgetCtx = gadgetCtx
+	dt.mu.Unlock()
+
+	return gadgetCtx
+}
+
+// Start initializes and starts the tracer
+func (dt *DNSTracer) Start(ctx context.Context) error {
+	// 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{
 			"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 +141,22 @@ func (dt *DNSTracer) Stop() error {
 	if dt.socketEnricherOp != nil {
 		dt.socketEnricherOp.Close()
 	}
-	if dt.gadgetCtx != nil {
-		dt.gadgetCtx.Cancel()
+
+	dt.mu.Lock()
+	cancel := dt.cancel
+	gadgetCtx := dt.gadgetCtx
+	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()
 	}
 	return nil
 }

pkg/containerwatcher/v2/tracers/dns_retry_test.go

@@ -0,0 +1,118 @@
+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 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)
+
+	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())
+}