Repository navigation
report actors whose sandbox dies on its own as crashed - #2695
Scott Funkenhauser (sfunkenhauser) merged 15 commits into
Conversation
Benjamin Elder (BenTheElder)
added
area/api
f0f338d to
60e7ac7
Compare
25f2e18 to
a8429ed
Compare
47eb3fb to
f88efdc
Compare
| if sandbox == nil { | ||
| slog.WarnContext(ctx, "Cannot watch the sandbox exit after checkpoint: it was never pinned") | ||
| } | ||
| err := rcmd.cmdCheckpoint(ctx, ocispec.PauseContainer, checkpointPath, durablePaths) |
There was a problem hiding this comment.
A failed checkpoint (849) resets stopping (854), so a sandbox that checkpoint still killed is reported as a crash. Intended?
There was a problem hiding this comment.
this is intended, the actor is dead here?
| // A process already gone yields a closed channel. | ||
| func Exited(pid int) (<-chan struct{}, error) { | ||
| done := make(chan struct{}) | ||
| fd, err := unix.PidfdOpen(pid, unix.PIDFD_NONBLOCK) |
There was a problem hiding this comment.
PIDFD_NONBLOCK needs kernel 5.10+. Document the minimum?
There was a problem hiding this comment.
On older kernels it now opens the pidfd without the flag and uses the blocking-poll fallback. The package doc says Linux 5.3+ is required, and that before 5.10 each watch holds its own thread.
I think we might actually require 6+ in some other places but I don't think we needed a hard requirement here. It's an optimization we really want but we keep it onsufficiently new kernels.
| if state := actor.GetStatus().GetState(); state != ateapipb.ActorState_ACTOR_STATE_RUNNING { | ||
| return nil, apierror.FailedPrecondition("actor %s is %s, not RUNNING", actorRef, state) | ||
| } | ||
| if err := crashActor(leaseCtx, w.store, actorRef, ateattr.OperationUnknown, message); err != nil { |
There was a problem hiding this comment.
Counted as operation unknown, same bucket as every other crash. Own value or a reason label?
There was a problem hiding this comment.
Hmm, need to think on that one.
There was a problem hiding this comment.
I think this thread may need further cleanup beyond the scope of the PR.
There was a problem hiding this comment.
cc Jeff Luo (@JeffLuoo) Krisztian F (@krisztianfekete) , I think we might want to add reason but that will require updating more places and this PR is already pretty big, I think I'll defer it and we'll chat on slack about how we want to handle these generally?
095c324 to
c612ebd
Compare
| } | ||
| reportCtx, cancel := context.WithTimeout(ctx, crashReportTimeout) | ||
| defer cancel() | ||
| if err := s.crashReporter.ReportCrashed(reportCtx, attribution, message); err != nil { |
There was a problem hiding this comment.
If the ReportCrashed fails, we can have an actor still RUNNING and worker capacity being consumed. Maybe instead of having 1-off crash detection, we need some kind of status reporting (worker reports all currently running actors) and reconciliation?
I don't really have a good suggestion here, and this change is a step in the right direction so I don't think we need to block on this, but maybe file a gh issue to track the gap?
There was a problem hiding this comment.
SGTM, we can follow-up. I think we are trying to balance chattiness with accuracy but we should have some eventual reconciliation ...
There was a problem hiding this comment.
8663da6 to
272e1ed
Compare
…y exit under the pid1 init a VMM that died on its own stayed a zombie until the actor was torn down, which may never happen
the control plane drops the worker assignment when it crashes the actor, so nothing would tear it down otherwise
the sandbox is watched through a pidfd, so the init reaping it doesn't hide the exit
the checkpoint wait and the crash watcher used to read the pidfile later, when a dead sandbox's pid could already be reused
a failed checkpoint can leave the actor RUNNING, and the watcher stayed off for the rest of the activation
…e fd a read error used to look like the process exiting. this also covers kernels before 5.10, which lack PIDFD_NONBLOCK
08db104 to
6ce9dba
Compare
| if client != nil { | ||
| tShutdown := time.Now() | ||
| shutCtx, cancel := context.WithTimeout(ctx, 10*time.Second) | ||
| if err := client.Shutdown(shutCtx); err != nil { |
There was a problem hiding this comment.
qq: is this client.Shutdown necessary here (since we call kill ch+virtiofsd processes later)?
There was a problem hiding this comment.
Probably not, and it might even be wasting time, but it predates this PR, I think we should handle it in a follow-up.
Follow-up to #2626
The control plane only found out a sandbox died when the next operation on the actor failed, which for an idle actor could be never.
Tested with a new e2e test, TestSandboxCrashIsReported (kills the sandbox from an ephemeral container, expects CRASHED, then revert + resume), plus both full lanes on GKE with no false crash reports.
This PR was AI assisted. I have tested everything on GKE and reviewed the changes.