Arvados

• the first two commits address the story as described
• the next commit addresses the state transitions discussed in #9187 and IRC:
  • don't start the container before changing state to Running
  • don't set state to Complete if there's no output / exit code -- use Cancelled if the container is "lost"
  • reset state to Queued if there was a problem setting up the container (before changing state to Running)
• the next two commits are misc tiny improvements to the test suite

Probably best to deal with the first two commits first (i.e., review beae579) to make sure the present issue gets addressed, and then deal with the extras.

• It should call UpdateContainerRecordRunning() after StartContainer(), otherwise if StartContainer() fails it will try to go Running -> Queued
• Suggest introducing a RuntimeConstraints struct with "API" as *bool field instead of using map[string]interface{}.
• As we discussed yesterday, to solve the race described in #9328 the auth token needs to be passed in at the time the crunch-run process is dispatched. While that means code in this branch will have to change, I don't think we should block the merge on that story.

Peter Amstutz wrote:

• It should call UpdateContainerRecordRunning() after StartContainer(), otherwise if StartContainer() fails it will try to go Running -> Queued

I don't think we can call StartContainer without first setting state to Running: StartContainer might succeed, resulting in the container actually running, but the container record still marked Locked, and thus able to go back to Queued state. See Containers API ("container states").

AFAICT, with the docker library, this is the latest point where we can change state to Running (this is why I split "create and start" into two separate functions).

• Suggest introducing a RuntimeConstraints struct with "API" as *bool field instead of using map[string]interface{}.

Yeah, that would be better. Now that #9162 is merged, perhaps I should add Container to the new SDK instead of having every program come up with its own version of it...

• As we discussed yesterday, to solve the race described in #9328 the auth token needs to be passed in at the time the crunch-run process is dispatched. While that means code in this branch will have to change, I don't think we should block the merge on that story.

Agreed

Tom Clegg wrote:

Peter Amstutz wrote:

• It should call UpdateContainerRecordRunning() after StartContainer(), otherwise if StartContainer() fails it will try to go Running -> Queued

I don't think we can call StartContainer without first setting state to Running: StartContainer might succeed, resulting in the container actually running, but the container record still marked Locked, and thus able to go back to Queued state. See Containers API ("container states").

If we change it to Running before the container actually starts, then if Start() fails the existing code will go Running -> Complete which is clearly wrong. It should go Running -> Cancelled. The quick fix is to add runner.finalState = "Cancelled" to the error block right after StartContainer(). However this behavior would be inconsistent with the policy in crunch-run leading up to this point which is to to go back to Queued state if we are unable to start the container for any reason.

Once we do #9328, it is fine if crunch-run crashes between successfully starting the container and setting the container to Running. It should get cleaned up by the dispatcher, put Locked -> Queued, and can be started again. The token(s) from the old crunch-run would be revoked so the process is orphaned and can't cause any trouble.

Tom Clegg wrote:

AFAICT, with the docker library, this is the latest point where we can change state to Running (this is why I split "create and start" into two separate functions).

Just to clarify case you were assuming otherwise, unlike "docker run", Docker.StartContainer() is not a blocking call. Docker.Wait() is where we block and wait for the container to terminate. This why I think it is better to set the Running state after calling StartContainer().

Peter Amstutz wrote:

Tom Clegg wrote:

AFAICT, with the docker library, this is the latest point where we can change state to Running (this is why I split "create and start" into two separate functions).

Just to clarify case you were assuming otherwise, unlike "docker run", Docker.StartContainer() is not a blocking call. Docker.Wait() is where we block and wait for the container to terminate. This why I think it is better to set the Running state after calling StartContainer().

Thanks for clarifying. However, we need to change state to Running before the container starts (not merely before it terminates).

My understanding is that once we call StartContainer, the container might start at any time -- e.g., between the time StartContainer returns and the time a subsequent "update state" API call takes effect. Starting the container while the container record state is still Locked would violate the Containers API.

Tom Clegg wrote:

Thanks for clarifying. However, we need to change state to Running before the container starts (not merely before it terminates).

My understanding is that once we call StartContainer, the container might start at any time -- e.g., between the time StartContainer returns and the time a subsequent "update state" API call takes effect. Starting the container while the container record state is still Locked would violate the Containers API.

Ok. My comment about fixing it so it goes into "Cancelled" instead of "Complete" still holds. One failure mode I can think of would be if the program to be run turns out to not exist inside the container, which should definitely be a hard failure rather than being put back into the Queue.

Peter Amstutz wrote:

If we change it to Running before the container actually starts, then if Start() fails the existing code will go Running -> Complete which is clearly wrong. It should go Running -> Cancelled. The quick fix is to add runner.finalState = "Cancelled" to the error block right after StartContainer().

Yes, you're right. Fixed in 4a2495c.

However this behavior would be inconsistent with the policy in crunch-run leading up to this point which is to to go back to Queued state if we are unable to start the container for any reason.

1. It is possible to go Locked→Running even though the user code never actually gets invoked
2. It is possible to start a container running user code, without ever changing Locked→Running

The API specifies that the first evil is the lesser one.

Once we do #9328, it is fine if crunch-run crashes between successfully starting the container and setting the container to Running. It should get cleaned up by the dispatcher, put Locked -> Queued, and can be started again. The token(s) from the old crunch-run would be revoked so the process is orphaned and can't cause any trouble.

Not according to the API. Once a container has started (or "maybe started, we don't know"), it can't un-start. After this point, if we want to retry, we have to create a new container.

Tom Clegg wrote:

Peter Amstutz wrote:

• Suggest introducing a RuntimeConstraints struct with "API" as *bool field instead of using map[string]interface{}.

Yeah, that would be better. Now that #9162 is merged, perhaps I should add Container to the new SDK instead of having every program come up with its own version of it...

Meh, decided to punt for now. Just changed to struct with *bool. 0d8b6bf

Tom Clegg wrote:

The API specifies that the first evil is the lesser one.

Sorry, this sounds weird. I should say "the API currently specifies". The fact that I wrote it on the API wiki doesn't mean it's the right and final answer. We can certainly change it if we decide the other evil is lesser.

LGTM @ 0d8b6bf