Arvados

11669-tidy-scancel-race @ f29a53dab584dcae3e4a38a7623efa6d3b30a663

Notes:

We've set up a race condition for ourselves. During normal operation, the last thing that crunch-run does is to mark the container as "Complete". If crunch-dispatch-slurm closes the updates channel (e.g. when the container goes into "Complete" state) before HasUUID() starts returning false, it will always start calling scancel.

It's possible the slurm job is lingering in "completing" state. I haven't noticed that but I haven't looked closely. It is conceivable that Arvados is propagating the container state quicker than slurm is propagating job state changes.

Issuing scancel on a non-existing job seems to exit 0.

It seems like it would make sense to check HasUUID() before calling scancel (instead of doing it after).

A one-second delay between calls to scancel seems very short (compared to, for example, the default 10 second message timeout of slurm). Looking at the logs, the scancel loop seems to resolve with 20 seconds.

We seem to have a pretty aggressive crunch-dispatch-slurm polling period configured (500ms).

1. crunch-run updates container state to Complete or Cancelled
2. crunch-dispatch-slurm waits some amount of time ("delay A") for the slurm job to disappear from the queue by itself
3. crunch-dispatch-slurm decides it's waited long enough, and starts hitting scancel
4. crunch-dispatch-slurm waits some amount of time ("delay B") before retrying if scancel does not take effect immediately

In the current implementation, "delay A" is zero and "delay B" is 1 second.

There is an unavoidable race condition: no matter what we do, it will always be possible for the slurm job to disappear from the queue in between "check whether the job is still in squeue" and "hit scancel". Fortunately, this is a harmless race: nothing bad happens when we hit scancel on a job that has already completed.

• no matter how long delay A is, we will sometimes hit scancel when waiting a bit longer would have worked.
• nothing bad happens if we hit scancel when waiting a bit longer would have worked.
• when delay B is short, we run scancel lots of times.
• when delay B is short, we often log several "job is still in queue" messages for each container.

Not sure which of these is [anticipated to be] a problem.

It seems like it would make sense to check HasUUID() before calling scancel (instead of doing it after).

AFAICT this would merely introduce a delay before the first call to scancel, without eliminating any races. The reason it's written the way it is is that in the "apiserver tells dispatch to cancel the job" case, it is better to hit scancel immediately, while in the container-finishing scenario, races are unavoidable anyway.

We could reduce the number of scancel invocations by ~1 per container by calling scancel() immediately the very first time, and after that, checking HasUUID() after the 1s delay instead of before the 1s delay.

We could rate-limit scancel invocations using a global time.Ticker(time.Second/time.Duration(maxScancelPerSecond)) so we don't flood slurm, the process table, or our file descriptor allotment in situations where we're waiting for 1000 containers' slurm jobs to exit.

We could limit the total number of outstanding scancel processes at a given time by making a buffered channel "pool" and wrapping the invocation in "<-pool" and "pool<-struct{}{}" -- this would be more certain way to avoid flooding the process table and fd allotment.

We could make the delay between consecutive scancel invocations configurable. It might be better to have a longer delay after a non-zero scancel exit code, since that means either something is broken (and will probably still be broken 1 second later) or [slurm changes its exit code rules and] it's an error to scancel a job that is already gone (in which case we've done our work and there's no particular hurry to exit our goroutine). Aside: If there is no job named X then "scancel --job-name=X" amounts to "cancel an empty set of jobs" so exit 0 seems sensible.

We could make the delay between consecutive "scancel had no effect" log messages configurable. (Perhaps it's OK to make lots of scancel attempts but annoying to fill the logs with noise.)