Arvados

• 10701 (leaking fds) → 318dd88 10701: Remove unneeded complexity in squeue invocation. (this is an improvement that might be related, but not convincing as a bug fix)
• 10702 (buggy error handling) → e356309 10703: Do not catch signals in crunch-dispatch-slurm. Simplify "stop dispatcher loop" API. *(simplifying the dispatcher loop fixes multiple concurrent attempts of the same container and resulting log confusion)
• 10703 (deadlock) → e356309 again (SIGTERM just exits immediately now)
• 10704 (sbatch loops too fast) → ae5eb12 10704: Rate-limit startup attempts per container.
• 10705 (better error message for "Unlock unlocked container") → not addressed yet

Couple of other cleanup things included too as separate commits.

Tom Clegg wrote:

10700-dispatch @ e34a5060cfc1cc4821b431e8aa6778a31898e0eb

• 10701 (leaking fds) → 318dd88 10701: Remove unneeded complexity in squeue invocation. (this is an improvement that might be related, but not convincing as a bug fix)

Much nicer. I have to confess I haven't been writing enough Go code lately and have been missing some of the subtle tricks.

• 10702 (buggy error handling) → e356309 10703: Do not catch signals in crunch-dispatch-slurm. Simplify "stop dispatcher loop" API. *(simplifying the dispatcher loop fixes multiple concurrent attempts of the same container and resulting log confusion)
• 10703 (deadlock) → e356309 again (SIGTERM just exits immediately now)

This seems to be the primary bug. This seems to explain the flip-flopping between monitored / unmonitored and the resulting pile up of redundant squeue invocations which is consistent with my initial analysis.

• 10704 (sbatch loops too fast) → ae5eb12 10704: Rate-limit startup attempts per container.

IIRC the Go "Ticker" sends a heartbeat every N seconds so for N=10 and it takes 9 seconds to do something, you'll only get 1 second of sleep before getting another tick. So squeue could still get called twice in < PollPeriod. Suggest MinRetryPeriod should have the same default as PollPeriod, instead of being disabled by default.

• 10705 (better error message for "Unlock unlocked container") → not addressed yet

Couple of other cleanup things included too as separate commits.

683f537 → this is just an argument of code style. In the pursuit of reducing indentation, the conditional has been transformed from "do this if X is true" to "don't do this if X is not true" which is much harder to reason about. Maybe at least clarify it with comments?

Peter Amstutz wrote:

This seems to be the primary bug. This seems to explain the flip-flopping between monitored / unmonitored and the resulting pile up of redundant squeue invocations which is consistent with my initial analysis.

I don't remember seeing anything about redundant invocations. Is this written down somewhere? AFAICT the squeue goroutine can't run more than one squeue at a time.

I see I wrote "squeue" in the commit message about simplifying exec.Cmd usage, but "sbatch" was the thing I actually fixed. So... did you also really mean "sbatch" here?

• 10704 (sbatch loops too fast) → ae5eb12 10704: Rate-limit startup attempts per container.

IIRC the Go "Ticker" sends a heartbeat every N seconds so for N=10 and it takes 9 seconds to do something, you'll only get 1 second of sleep before getting another tick. So squeue could still get called twice in < PollPeriod. Suggest MinRetryPeriod should have the same default as PollPeriod, instead of being disabled by default.

MinRetryPeriod only affects how soon a container will be resubmitted with sbatch after disappearing from the slurm queue, and doesn't use a ticker.

I don't think it's a problem for squeue to be called twice in < PollPeriod -- are you seeing something I'm not?

I do see squeue.go could use some of the same exec.Cmd simplifying treatment noted above, though. I'll do that. (I don't see an FD leak there either, unfortunately.)

683f537 → this is just an argument of code style. In the pursuit of reducing indentation, the conditional has been transformed from "do this if X is true" to "don't do this if X is not true" which is much harder to reason about. Maybe at least clarify it with comments?

Fair point. Backed this out half way -- now it's "if A && (B || C)" instead of two nested ifs.

Refactored the squeue stuff, (I think) it's much simpler now.

I didn't see any leaking FDs in the old code, but I figure it can't hurt to get rid of all the pipes and stuff.

10700-dispatch @ a19ffad966b25b3869e666f749f7c6da187bef68

Tom Clegg wrote:

Peter Amstutz wrote:

This seems to be the primary bug. This seems to explain the flip-flopping between monitored / unmonitored and the resulting pile up of redundant squeue invocations which is consistent with my initial analysis.

I don't remember seeing anything about redundant invocations. Is this written down somewhere? AFAICT the squeue goroutine can't run more than one squeue at a time.

I see I wrote "squeue" in the commit message about simplifying exec.Cmd usage, but "sbatch" was the thing I actually fixed. So... did you also really mean "sbatch" here?

Yes, I meant sbatch. I have to review the logs, but I believe it was getting into a situation of running overlapping sbatch commands for the same container. Which would explain the FD leak if there was a bunch of stuck goroutines with pipes open.

• 10704 (sbatch loops too fast) → ae5eb12 10704: Rate-limit startup attempts per container.

IIRC the Go "Ticker" sends a heartbeat every N seconds so for N=10 and it takes 9 seconds to do something, you'll only get 1 second of sleep before getting another tick. So squeue could still get called twice in < PollPeriod. Suggest MinRetryPeriod should have the same default as PollPeriod, instead of being disabled by default.

MinRetryPeriod only affects how soon a container will be resubmitted with sbatch after disappearing from the slurm queue, and doesn't use a ticker.

Same confusion, I meant sbatch. I meant that if the intended default behavior is to rate-limit sbatch invocations to at least PollPeriod, with throttling disabled the actual time between two invocations could be less than PollPeriod because it could be run at the end of the previous period and again at the start of the next period. Setting throttle == PollPeriod (instead of 0) ensures that PollPeriod is the floor instead of the ceiling.

Rest LGTM.

Peter Amstutz wrote:

Yes, I meant sbatch. I have to review the logs, but I believe it was getting into a situation of running overlapping sbatch commands for the same container. Which would explain the FD leak if there was a bunch of stuck goroutines with pipes open.

OK. I looked into this on #10701. I didn't find that sbatch was getting stuck. It was possible for monitorSubmitOrCancel() to return before it even had a chance to check the slurm queue, though.

MinRetryPeriod only affects how soon a container will be resubmitted with sbatch after disappearing from the slurm queue, and doesn't use a ticker.

Same confusion, I meant sbatch. I meant that if the intended default behavior is to rate-limit sbatch invocations to at least PollPeriod, with throttling disabled the actual time between two invocations could be less than PollPeriod because it could be run at the end of the previous period and again at the start of the next period. Setting throttle == PollPeriod (instead of 0) ensures that PollPeriod is the floor instead of the ceiling.

Ah.

The intended default behavior is merely to behave correctly, i.e., avoid overlapping sbatch calls. In the default case, MinRetryPeriod is 0 and there is no effort (or need, afaik) to enforce a minimum time between two consecutive attempts. PollPeriod just establishes a minimum average time between attempts.

If you do want a minimum interval between sbatch attempts, you can use MinRetryPeriod.

LGTM.