diff --git a/pkg/containerwatcher/v2/tracers/dns.go b/pkg/containerwatcher/v2/tracers/dns.go index 70775a124d..3ff46aaa12 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,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 } 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..4cf5b4e5f4 --- /dev/null +++ b/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()) +}