shim: bound the io drain in delete and release stdio when a created container stops - #14428
Open
Ayush-Rathor wants to merge 1 commit into
Open
shim: bound the io drain in delete and release stdio when a created container stops#14428Ayush-Rathor wants to merge 1 commit into
Ayush-Rathor wants to merge 1 commit into
Conversation
…ontainer stops
Init.delete() waits for the stdio copy goroutines to drain and only closes
the pipes afterwards:
p.wg.Wait()
...
if p.io != nil {
for _, c := range p.closers { c.Close() }
p.io.Close()
}
p.wg is released by the two copyPipes goroutines, which return on EOF, and
EOF requires every write end to be closed. Init.Create only hands the IO to
the runtime for the sandbox:
if p.Sandbox {
opts.IO = p.io
}
so for a sub-container the shim holds the write ends itself and releases
them only in CloseAfterStart(), which runs after runsc start has been
successfully launched. A sub-container that is created but never started
therefore leaves p.wg.Wait() waiting for an EOF that only the p.io.Close()
below it can produce. Such a shim can never be reaped, and a later
containerd restart blocks in loadShims and never finishes starting.
Note this is not specific to any one failure: the precondition is only that
runsc create succeeded (so copyPipes ran) and runsc start never did (so the
write ends were never handed over).
Two changes.
Close the stdio when a container leaves "created" for "stopped". Leaving
"created" means Start never succeeded, so the container never ran and has no
output to lose. This is done in createdState.transition() rather than at
individual call sites so that it covers every route out of "created" --
including createdState.State(), where a routine status query moves the state
machine to "stopped" without closing anything. It is deliberately not done
when leaving "running": a container that ran may still have buffered output
in flight, and closing early would truncate its logs, which is why delete()
drains first and closes after. This makes the existing s.p.io.Close() in
createdState.Start()'s failure branch redundant, so it is removed and the io
is now closed in exactly one place.
Bound the drain with waitTimeout. pkg/shim/v1/proc is derived from
containerd's runc shim (these files carry "Copyright 2018 The containerd
Authors"), which guards both drain sites with
waitTimeout(ctx, &p.wg, 10*time.Second); the bound was dropped when the code
was forked. It is needed on top of the first change because
createdState.Delete() calls p.delete() directly, with no transition at all,
so a container that is created and deleted without Start ever being called
still reaches the unbounded wait. The timeout is self-healing rather than
merely giving up: once delete() stops waiting it proceeds to p.io.Close(),
which releases the goroutines that were blocking the drain.
execProcess.delete() gains a ctx parameter to match; both callers in
exec_state.go already had one available.
Adds pkg/shim/v1/proc/delete_test.go. TestCreatedStateToStoppedClosesIO
fails without the first change with:
io was not closed on the created->stopped transition; the shim would
keep the pipe write ends and Init.delete could never drain
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14427.
Init.delete()drains the stdio copy goroutines and closes the pipes about twenty lines later:p.wgis released bycopyPipeson EOF, which requires every write end to be closed.Init.Createonly passes the IO to the runtime when
p.Sandbox, so for a sub-container the shim holds the writeends itself and drops them only in
CloseAfterStart(), afterrunsc startis successfullylaunched. A sub-container that never started therefore blocks in
p.wg.Wait()waiting for an EOFthat only the
p.io.Close()below it can produce. That shim can never be reaped, and the nextcontainerd restart hangs in
loadShims.This is not specific to a particular failure. The precondition is only:
runsc createsucceeded —copyPipesruns afterp.runtime.Createreturns, sop.wgis +2(a failed create leaves it at zero and cannot deadlock);
runsc startnever succeeded, soCloseAfterStart()never ran;1. Close the stdio on
created->stopped(init_state.go)Leaving
createdmeansStartnever succeeded, so the container never ran and has no output tolose. Done in
createdState.transition()rather than at call sites, so it covers every route outof
created— includingcreatedState.State(), where a routine status query moves the statemachine to
stoppedwithout closing anything.Deliberately not done when leaving
running: such a container may still have buffered output inflight, and closing early would truncate its logs, which is why
delete()drains first and closesafter. This makes the
s.p.io.Close()increatedState.Start()'s failure branch redundant, so itis removed and the io is closed in exactly one place.
2. Bound the drain (
init.go,exec.go,utils.go)waitTimeoutis restored from containerd's runc shim, which this package is forked from and which guards both drain sites withwaitTimeout(ctx, &p.wg, 10*time.Second).This is needed on top of change 1 because
createdState.Delete()callsp.delete()directly withno transition at all, so a container created and deleted without
Startever being called stillreaches the unbounded wait. The timeout is self-healing rather than merely giving up: once
delete()stops waiting it proceeds top.io.Close(), which releases the goroutines that wereblocking the drain.
execProcess.delete()gains actxparameter to match; both callers inexec_state.goalready had one.Testing
New
pkg/shim/v1/proc/delete_test.go(5 tests).TestCreatedStateToStoppedClosesIOfails withoutchange 1:
//pkg/shim/...23/23 pass.//runsc/container:container_test(8 shards) passes.Reproduction steps and the affected-node evidence are in #14427.
What produced our particular route into this state is a separate sentry-side problem — after a
failed restore, the sandbox-wide
l.state == restoreFailedis used to answer per-container statusqueries, so a container created after the failure is reported
RuntimeStateStoppedon arrivalinstead of
RuntimeStateCreating. That needs its own fix and I will send it separately; this PR isabout making the shim survive.