diff --git a/docs/proposals/otel-tracing.md b/docs/proposals/otel-tracing.md index 24c7bfc0..9fd7d63e 100644 --- a/docs/proposals/otel-tracing.md +++ b/docs/proposals/otel-tracing.md @@ -233,7 +233,7 @@ All claims below were checked against the code; line numbers are anchors for rev |---|---|---| | cmd → app | `app.New` roots at `context.Background()`; `ctx, Cancel = WithCancel(ctx)` (`pkg/app/app.go`, `New`) | span must be injected here (§4.2) | | app → all helm execs | `getHelm()` constructs the `ShellRunner` with `Ctx: a.ctx` (`pkg/app/app.go:1008`); the resulting `execer` is **cached per (helm binary, kube-context)** in `a.helms` and shared by all releases and workers (`pkg/app/app.go:982–1021`) | once `App.ctx` is span-rooted, every non-kubedog helm call nests automatically, with **zero changes** to `getHelm`. The shared-instance cache is also why per-release contexts must ride per-call parameters, never mutation of the shared execer | -| kubedog path (sync with tracking) | `startBackgroundKubedogTracking(gocontext.Background(), …)` (`pkg/state/state.go:1294`) → `bufferHelmOutput` derives `releaseCtx := context.WithCancel(ctx)` and swaps it in via `execer.WithContext(releaseCtx)` (`pkg/state/helmx.go:363–365`) | helm execs on this path run on a **Background-rooted** context; runner-level spans would become **orphan traces**. Bridged in §4.4 | +| kubedog path (sync with tracking) | originally `startBackgroundKubedogTracking(gocontext.Background(), …)` (`pkg/state/state.go`), with `bufferHelmOutput` deriving `releaseCtx := context.WithCancel(ctx)` and swapping it in via `execer.WithContext(releaseCtx)` (`pkg/state/helmx.go`). **Since #2791** the three call sites pass `st.releaseCancelContext()` — the app cancel context injected via `HelmState.SetCancelContext` — so SIGINT/SIGTERM reaches these helm subprocesses too (#2770) | trace context on this path is the same command span as everywhere else: the §4.4 bridge kept spans attached while cancellation was detached, and #2791 subsequently re-attached cancellation | | hooks | both `event.Bus` constructions (`triggerGlobalReleaseEvent`, `triggerReleaseEvent`, `pkg/state/state.go:3666, 3703`) duplicate the same literal and pass **no** `Runner`, so the default kicks in: `ShellRunner{Dir: bus.BasePath, Logger: bus.Logger, Ctx: goContext.TODO()}` with an inline comment acknowledging it should be `app.Ctx` (`pkg/event/bus.go:61–71`) | hook execs are detached; spans would be orphans. Bridged in §4.4 | | non-kubedog release workers | release loops (`SyncReleases` etc., `pkg/state/state.go:1212 ff.`) call the shared `helmexec.Interface` with a `HelmContext` (`pkg/helmexec/context.go`) that carries **no go-context** | per-release spans need the §4.4 mechanism | | subprocess funnel | exactly three `ShellRunner` construction sites exist (verified exhaustive): `pkg/app/app.go:129` (`Init`, `Ctx: a.ctx`), `pkg/app/app.go:1006` (`getHelm`, `Ctx: a.ctx`), and the hooks default (`pkg/event/bus.go:62`, `Ctx: TODO` — §4.4 bridge). Every external process helmfile itself starts goes through `Execute`/`ExecuteStdIn` (`pkg/helmexec/runner.go`); helm commands additionally funnel through `execer.exec()` (`pkg/helmexec/exec.go:1207`). Exception: kustomize executes inside the chartify library, outside this funnel (§12) | one instrumentation point covers everything except chartify-internal execs; spans nest wherever the runner's `Ctx` carries a span | @@ -241,7 +241,8 @@ All claims below were checked against the code; line numbers are anchors for rev Two pre-existing gaps surfaced by this analysis — kubedog tracking not being cancellable via `App.ctx`, and hooks likewise — are **out of scope** for this proposal beyond trace-context bridging (§4.4), because fixing their *cancellation* semantics would be a behavior change. -They should be reported as separate issues. +They should be reported as separate issues. (Update: the kubedog gap was fixed by +#2791 via `HelmState.SetCancelContext`; the hooks gap remains open as #2771.) ### 4.4 Context plan @@ -258,12 +259,15 @@ They should be reported as separate issues. 2. Runner-level spans in `ShellRunner.Execute`/`ExecuteStdIn` nest for all non-kubedog, non-hook execs automatically. 3. **Orphan-bridge for kubedog and hooks, with identical cancellation semantics:** - - `pkg/state/state.go:1294`: pass `context.WithoutCancel(telemetry.CommandContext())` - instead of `gocontext.Background()`. `WithoutCancel` preserves values (the span) + - kubedog: originally pass `context.WithoutCancel(telemetry.CommandContext())` + instead of `gocontext.Background()`. `WithoutCancel` preserved values (the span) while dropping cancellation — and `Background` never carried cancellation anyway, so - SIGINT/timeout behavior is **bit-for-bit unchanged**; only trace context is added. - (Phase 1 uses the root span from `telemetry.CommandContext()`, which requires no new - plumbing in `pkg/state`; phase 2 re-parents under the per-release `st.traceCtx`.) + SIGINT/timeout behavior stayed **bit-for-bit unchanged**; only trace context was + added. (Phase 1 used the root span from `telemetry.CommandContext()`, which required + no new plumbing in `pkg/state`; phase 2 re-parented under the per-release + `st.traceCtx`.) Superseded by #2791: the three kubedog call sites now pass + `st.releaseCancelContext()` — same command-span trace context, plus app + cancellation, fixing #2770. - `pkg/event/bus.go`: add an optional `Ctx context.Context` field to `Bus`; the default runner construction uses `bus.Ctx` when set, `TODO` when nil (so behavior is unchanged for any nil-Ctx caller). The two construction sites in `pkg/state/state.go:3666, 3703` @@ -414,7 +418,8 @@ Traces leave the machine they run on. Ground rules, checked against what exists churn; cancellation semantics identical. 3. Kubedog and hook bridging uses `context.WithoutCancel`, which drops cancellation and keeps values — the swapped-out parents (`Background`/`TODO`) never propagated - cancellation either, so SIGINT/timeout behavior is unchanged. + cancellation either, so SIGINT/timeout behavior is unchanged. (Superseded for kubedog + by #2791, which re-roots tracking under the app cancel context; hooks stay bridged.) 4. Telemetry setup or export failures never fail or slow the run (warning log only, export off the critical path). 5. All pre-existing behavior, including the known cancellation gaps of §4.3 and the exact @@ -455,7 +460,7 @@ pkg/helmexec/redact.go // shared args redaction, legacy+strict profile pkg/helmexec/exit_error.go // calls shared helper with legacy profile (output byte-identical) pkg/helmexec/context.go // HelmContext.Ctx field (phase 2) pkg/helmexec/exec.go // execCtx funnel beside exec/execStdIn (phase 2) -pkg/state/state.go // WithoutCancel bridges (kubedog call site + both event.Bus constructions); release spans (phase 2) +pkg/state/state.go // WithoutCancel bridges (both event.Bus constructions; the kubedog call sites moved to st.releaseCancelContext() in #2791); release spans (phase 2) pkg/state/helmx.go // (no change — bridge happens at its caller) pkg/event/bus.go // optional Ctx field consumed by the default runner docs/experimental-features.md // feature entry → promoted out when stable diff --git a/pkg/app/app.go b/pkg/app/app.go index b8e1b073..5e715f6d 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -975,6 +975,8 @@ func (a *App) loadDesiredStateFromYamlWithBaseDir(file string, baseDir string, o // Per-release spans (pkg/state) parent under the load span. st.SetTraceContext(loadCtx) + // Kubedog tracking / buffered helm subprocesses cancel with the app. + st.SetCancelContext(a.ctx) st.SetKubeconfig(a.Kubeconfig) diff --git a/pkg/state/span.go b/pkg/state/span.go index 912bac6d..2c686dba 100644 --- a/pkg/state/span.go +++ b/pkg/state/span.go @@ -15,10 +15,12 @@ import ( // traceOnlyContext returns a context carrying trace context — the command // span, or the given parent (typically a per-release span) — while never -// propagating cancellation. The historically detached paths (kubedog -// tracking, hook execution) must keep their cancellation semantics, so only -// trace context is bridged (see docs/proposals/otel-tracing.md §4.4). With -// tracing disabled it is indistinguishable from Background. +// propagating cancellation. Hook execution must keep its detached +// cancellation semantics (#2771), so only trace context is bridged (see +// docs/proposals/otel-tracing.md §4.4). Kubedog tracking formerly rooted +// here too; it now derives from the app cancel context so SIGINT reaches +// the buffered helm subprocesses (#2770, #2791). With tracing disabled it +// is indistinguishable from Background. func traceOnlyContext(parent ...gocontext.Context) gocontext.Context { ctx := telemetry.CommandContext() if len(parent) > 0 && parent[0] != nil { @@ -35,6 +37,14 @@ func (st *HelmState) SetTraceContext(ctx gocontext.Context) { st.traceCtx = ctx } +// SetCancelContext sets the context canceled with the app on SIGINT/SIGTERM. +// Kubedog tracking (and the helm subprocesses it buffers) derive from this so +// process-wide cancellation reaches them. A nil context keeps the historical +// Background-rooted behavior. +func (st *HelmState) SetCancelContext(ctx gocontext.Context) { + st.cancelCtx = ctx +} + func (st *HelmState) releaseSpanParent() gocontext.Context { if st.traceCtx != nil { return st.traceCtx @@ -42,6 +52,16 @@ func (st *HelmState) releaseSpanParent() gocontext.Context { return gocontext.Background() } +// releaseCancelContext returns the context kubedog tracking should root under. +// When unset it falls back to Background, matching the pre-#2770 detached +// semantics used by tests that construct HelmState literals without an App. +func (st *HelmState) releaseCancelContext() gocontext.Context { + if st.cancelCtx != nil { + return st.cancelCtx + } + return gocontext.Background() +} + // startReleaseSpan starts one helmfile.release. span for a release, // parented from the state's trace context. The returned context is meant to // be stamped into the per-release helmexec.HelmContext so the release's helm diff --git a/pkg/state/span_test.go b/pkg/state/span_test.go index 45f26b2f..64db29b2 100644 --- a/pkg/state/span_test.go +++ b/pkg/state/span_test.go @@ -229,6 +229,71 @@ func TestSetTraceContext(t *testing.T) { assert.Equal(t, ctx, st.releaseSpanParent()) } +// TestSetCancelContext pins #2770: kubedog tracking roots under the app +// cancel context so SIGINT reaches buffered helm subprocesses. Unset keeps +// the historical Background fallback (detached / non-app-cancelable). +func TestSetCancelContext(t *testing.T) { + st := &HelmState{} + assert.Equal(t, gocontext.Background(), st.releaseCancelContext(), "unset cancel context must fall back to Background") + + ctx, cancel := gocontext.WithCancel(gocontext.Background()) + defer cancel() + st.SetCancelContext(ctx) + got := st.releaseCancelContext() + assert.Equal(t, ctx, got) + + cancel() + select { + case <-got.Done(): + default: + t.Fatal("releaseCancelContext must become Done when the app cancel context is canceled") + } +} + +// ctxRecordingHelm captures the context handed to it via ContextSwapper so +// tests can assert which context roots the buffered helm subprocesses. The +// embedded nil Interface panics on any other method call, which +// bufferHelmOutput never makes. +type ctxRecordingHelm struct { + helmexec.Interface + ctx gocontext.Context +} + +func (h *ctxRecordingHelm) WithLogger(_ *zap.SugaredLogger) helmexec.Interface { return h } + +func (h *ctxRecordingHelm) WithContext(ctx gocontext.Context) helmexec.Interface { + h.ctx = ctx + return h +} + +// TestBufferHelmOutputRootsAtCancelContext pins the actual #2770 wiring at +// the unit level: the release-scoped context that bufferHelmOutput swaps in +// via WithContext must derive from the app cancel context (so the runner's +// ctx.Done() SIGINT path fires), and stay non-cancelable when no cancel +// context is set. +func TestBufferHelmOutputRootsAtCancelContext(t *testing.T) { + appCtx, cancel := gocontext.WithCancel(gocontext.Background()) + defer cancel() + + tracked := &HelmState{logger: zap.NewNop().Sugar()} + tracked.SetCancelContext(appCtx) + trackedHelm := &ctxRecordingHelm{} + tracked.bufferHelmOutput(trackedHelm, tracked.releaseCancelContext(), "demo") + require.NotNil(t, trackedHelm.ctx, "bufferHelmOutput must swap in a release-scoped context via WithContext") + + cancel() + select { + case <-trackedHelm.ctx.Done(): + default: + t.Fatal("the context handed to WithContext must become Done when the app cancel context is canceled") + } + + detached := &HelmState{logger: zap.NewNop().Sugar()} + detachedHelm := &ctxRecordingHelm{} + detached.bufferHelmOutput(detachedHelm, detached.releaseCancelContext(), "demo") + assert.NoError(t, detachedHelm.ctx.Err(), "unset cancel context must keep the release context detached (Background-rooted)") +} + func TestSkipUndesired(t *testing.T) { assert.False(t, skipUndesired(&ReleaseSpec{}), "installed unset means desired") diff --git a/pkg/state/state.go b/pkg/state/state.go index a2780e74..4a1b8faa 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -148,6 +148,11 @@ type HelmState struct { // traceCtx parents per-release spans; set via SetTraceContext (see span.go). traceCtx gocontext.Context + // cancelCtx is canceled with the app on SIGINT/SIGTERM. Kubedog tracking + // and its buffered helm subprocesses derive from it so Ctrl+C reaches them + // (see issue #2770). Set via SetCancelContext; nil falls back to Background. + cancelCtx gocontext.Context + ReleaseSetSpec `yaml:",inline"` logger *zap.SugaredLogger @@ -1312,10 +1317,10 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } else if release.UpdateStrategy == UpdateStrategyReinstallIfForbidden { relErr = st.performSyncOrReinstallOfRelease(affectedReleases, helm, context, release, chart, m, flags...) if relErr == nil { - relErr = st.trackReleaseIfEnabled(traceOnlyContext(), release, helm, opts) + relErr = st.trackReleaseIfEnabled(st.releaseCancelContext(), release, helm, opts) } } else { - trackHandle, trackStarted := st.startBackgroundKubedogTracking(traceOnlyContext(), release, helm, opts) + trackHandle, trackStarted := st.startBackgroundKubedogTracking(st.releaseCancelContext(), release, helm, opts) // trackHandle.Helm is a logger-scoped helm clone that // captures output to an in-memory buffer while tracking is // active. When tracking isn't running it's the original @@ -1368,7 +1373,7 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme if trackStarted { trackErr = trackHandle.Wait() } else { - trackErr = st.trackReleaseIfEnabled(traceOnlyContext(), release, helm, opts) + trackErr = st.trackReleaseIfEnabled(st.releaseCancelContext(), release, helm, opts) } if trackErr != nil { m.Lock()