Repository navigation
ateom-gvisor: treat a container runsc has no record of as already destroyed - #1666
Conversation
A teardown that runs after the sandbox has already exited, or one interrupted mid-`runsc delete`, leaves a container without its runsc state record. Every later `runsc state` and `runsc delete -force` on it then fatal-exits 128: the not-found tolerance behind -force sits after the spec fetch that fails first. cleanupContainers stopped at that error, so TerminateWorkload could never succeed again. In ateapi a failed terminate skips releasing the worker, so the actor stayed DELETING forever and its share of the worker's capacity stayed booked; every retry re-hit the same 128 at the state check. cleanupContainers now falls back to `runsc list` when `runsc state` errors: a container runsc no longer lists is already destroyed and is skipped, so the retry completes and releases the worker. `runsc list` loads no container and so never fatal-exits on a missing one, which lets it distinguish "already gone" from a real failure without matching on runsc's error text. It runs only on the error path, so the success path is unchanged. A delete that drops the record and then fails (runsc removes the record before the rootfs filestore, and a missing filestore is fatal) still fails that attempt; the retry's state check then finds the container gone and finishes. The stop and delete run detached from the caller's context with their own timeout, as the post-checkpoint resume and the Run/Restore failure cleanups already do, so a request deadline can no longer SIGKILL runsc mid-delete and manufacture the missing record. Both TerminateWorkload and CheckpointWorkload's post-checkpoint cleanup go through this path.
| "github.com/agent-substrate/substrate/internal/proto/ateompb" | ||
| ) | ||
|
|
||
| // fakeRunsc mocks the runsc binary. It lets a test mock the state, list and |
There was a problem hiding this comment.
hopefully it makes sense to create such fake
There was a problem hiding this comment.
Can we somehow avoid the need for fakeRunsc?
There was a problem hiding this comment.
Updated to use the existing containerRuntime interface - expand it to include other functions - PTAL, WDYT ?
Ron Lev (ronlv10)
changed the title
| for _, name := range names { | ||
| if err := r.cmdState(ctx, name); err != nil { | ||
| err = fmt.Errorf("while checking state of %q container: %w", name, err) | ||
| gone, listErr := r.isContainerAlreadyGone(ctx, name) |
There was a problem hiding this comment.
Instead of listing every time, why not listing at the beginning of this function and then only trying to delete the requested containers (plus pause) which are in the list response?
There was a problem hiding this comment.
As it's the path of suspend/checkpoint, I didn't want to add an extra step for the happy path.
I was ok with running it multiple times only in the edge case where the container is already deleted.
I did consider having something like a closure with a cache that will be called in the path of failure, something like
var listed []string
var listOnce sync.Once
alreadyGone := func(name string) (bool, error) {
listOnce.Do(func() { listed, listErr = r.cmdList(ctx) })
if listErr != nil {
return false, listErr
}
return !slices.Contains(listed, name), nil
}
WDYT?
There was a problem hiding this comment.
Ah, good point. Probably not worth the complexity. What you have SGTM. I think we need to track why the runsc state call is needed in the first place, as it sounds like a bug in the CLI. This is a good workaround though.
There was a problem hiding this comment.
I'm fixing runsc delete so the call to runsc state isn't needed anymore. IIRC. it had to do with a zombie sandbox process not being reaped timely.
There was a problem hiding this comment.
Fabricio Voznika (@fvoznika)
If you're already touching those areas, could we make the runsc delete command return a specific exit code for 'not found'? This way, we wouldn't need to list the containers first just to see if that was the reason.
There was a problem hiding this comment.
I think the are a couple of things happening here:
a) There is a bug in the runsc command that requires that we run runsc state before runsc delete, otherwise runsc delete fails under some conditions. This is clearly a bug in runsc. It should require a specific sequence of calls to succeed the deletion. Once we fix this bug, we should be able to delete the calls to runsc state completely.
b) The --force flag in runsc delete has unclear semantics. It's documented as a way to delete a container irrespective of it's running state, but it also means "ignore not found", although it's not documented. There seems to be also a bug in it if you pass multiple containers and some of them are not found (the executing halts on the first not found)1. I filed google/gvisor#14806 for this.
It looks like If we fix both, this should just be a single CLI execution: runsc delete --force [container names].
Fabricio Voznika (@fvoznika) thoughts?
There was a problem hiding this comment.
a) It's being addressed in google/gvisor#14797
b) delete --force is idempotent, so it still succeeds after the sandbox is deleted and no longer exists. This is dictated by the OCI (implicitly by following what runc does).
If you need something that kills the sandbox on deletion but fails if the sandbox doesn't exist anymore, we can add a different flag for this. PLMK.
…down stopContainers, cleanupContainers and isContainerAlreadyGone orchestrate several runsc commands with workload knowledge such as the pause container and the state-then-delete order. They are package-level functions that receive the runsc wrapper through containerRuntime, like killContainer, so the runsc type holds only the per-command wrappers and the teardown tests drive it with an in-Go fake instead of a shell script standing in for the runsc binary.
2dda726
into
agent-substrate:main
Issue : #1665
Problem
Once a container doesn't exists on worker anymore, ateom can never tear the actor down again.
runsc stateandrunsc delete -forceboth fatal-exit 128 on a container runsc has no record of (loading container: file does not exist; the-forcenot-found tolerance sits after the spec fetch that fails first).cleanupContainersreturned that error,TerminateWorkloadfailed, and ateapi skipped releasing the worker. The actor stayedDELETINGand its share of the worker's capacity stayed booked; every retry hit the same 128 at thestatecheck.The record goes missing in two observed ways:
runsc deleteafter the sandbox is lost removes the record and still exits 128, because gVisor'sDestroyruns every step and reports errors at the end. Seen when the ateom container was OOM-killed and restarted: the sandbox and its rootfs mounts were gone, the records were not, and the delete failed on the missing filestore after erasing the record.runsc deleteSIGKILLs runsc partway through destroying the container.Change
cleanupContainersnow treats "runsc has no record of it" as already destroyed:runsc stateerrors, it runsrunsc listand skips the container if it is not listed.runsc listloads no container, so it never fatal-exits on a missing one, which distinguishes "already gone" from a real failure without matching on runsc's error text. It runs only on the error path; the success path is unchanged.stopContainersandcleanupContainersrun detached from the request context with their own 30s timeout, as the post-checkpoint resume already does, so a caller deadline can no longer SIGKILL runsc mid-delete. BothTerminateWorkloadandCheckpointWorkload's post-checkpoint cleanup go through this path.