mirror of
https://github.com/helmfile/helmfile.git
synced 2026-10-19 04:42:40 +02:00
fix: cancel kubedog-tracked helm subprocesses on SIGINT/SIGTERM (#2791)
* fix: cancel kubedog-tracked helm subprocesses on SIGINT/SIGTERM Kubedog tracking rooted helm and tracker contexts at Background (via traceOnlyContext), so App.Cancel never reached those subprocesses and Ctrl+C blocked in CleanWaitGroup until helm exited on its own. Thread the app cancel context into HelmState (SetCancelContext) and use it for the three kubedog tracking call sites, keeping the per-release WithCancel safety valve. Hooks stay on the non-canceling trace bridge. Fixes #2770 Signed-off-by: Jason Wang <vulragrag@gmail.com> * docs/test: align cancel-context docs with #2791 and pin the kubedog wiring - traceOnlyContext comment no longer lists kubedog tracking among the detached paths; only hooks (#2771) remain, with a pointer to #2791. - docs/proposals/otel-tracing.md sections 4.3/4.4/7/9 now mark the kubedog cancellation gap as fixed by #2791 (SetCancelContext), so the design doc stops misdescribing main. - TestBufferHelmOutputRootsAtCancelContext pins the actual #2770 wiring: the context bufferHelmOutput hands to ContextSwapper.WithContext must become Done when the app cancel context is canceled, and stay non-cancelable when unset. Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: Jason Wang <vulragrag@gmail.com> Signed-off-by: yxxhero <aiopsclub@163.com> Co-authored-by: yxxhero <aiopsclub@163.com>
This commit is contained in:
co-authored by
yxxhero
parent
fff84390b1
commit
d4a4135f9b
@@ -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)
|
||||
|
||||
|
||||
+24
-4
@@ -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.<verb> 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
|
||||
|
||||
@@ -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")
|
||||
|
||||
|
||||
+8
-3
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user