Arvados

A few thoughts:

How do we handle failure? Is the dispatcher required to retry containers that fail, or is the dispatcher a "best effort" service and the API decides to retry by scheduling a new container?

Currently the container_uuid field only holds a single container_uuid at a time. If the API schedules a new container, does that mean any container requests associated with that container get updated with the new container?

If the container_uuid field only holds one container at a time, and container don't link back to the container requests that created, then we don't have a way to record of past attempts to fulfill this request. This means we don't have anything to check against container_count_max. A few possible solutions:

• Make container_uuid an array of containers created to fulfill a given container request (this introduces complexity)
• Decrement container_count_max on the request when submitting a new container
• Compute content address of the container request and discover containers with that content address. This would conflict with "no reuse" or "impure" requests which are supposed to ignore past execution history. Could solve this by salting the content address with a timestamp; "no reuse" containers would never ever be reusable which might be fine.

Also:

I think we should distinguish between infrastructure failure and task failure by distinguishing between "TempFail" and "PermFail" in the container state. "TempFail" shouldn't count againt the container_count_max count, or alternately we only honor container_count_max for "TempFail" tasks and don't retry "PermFail".

Ideally, "TempFail" containers should retry forever, but with a backoff. One way to do the backoff is to schedule the container to run at a specific time in the future.

Having a field specifying "wait until time X to run this container" would be generally useful for cron-style tasks.

Also missing: a "parent container" or "parent container request" concept to build process trees.

To elaborate, suggest the container request have an optional "parent_container_uuid" field. The process tree can be built by looking "container -> request -> container -> request -> ..."

Assuming the above notes are not part of this story (for the development spike of getting something working end to end as quickly as possible we can probably ignore error handling for now), we will need a follow on story to hash out and implement these details.

Also missing: a "parent container" or "parent container request" concept to build process trees.

This is what requesting_container_uuid is for, if I get your drift. E.g., "cancel all active ContainerRequests that were initiated by this Container" in the issue description.

I think we should distinguish between infrastructure failure and task failure by distinguishing between "TempFail" and "PermFail" in the container state.

I disagree. Once a container stops, it stays stopped; there is nothing temporary about it. A client's decision about whether to start a new container B after container A fails is not a property of Container A.

Deciding how much a given Container counts toward a ContainerRequest's container_count_max is an interesting issue, but however we do it I don't think it goes in the Container and I don't think it should block this story.

We probably do need to add something like "exit_code" to Container but let's implement what we have rather than wait for that bikeshed to be painted. Database migrations aren't that hard.

I don't think any of the other points in notes 8..11 need to be settled in order to implement this story as described.

#6518 is the dispatch story. Starting a page about how a dispatcher is supposed to work (on a Crunch2 dispatch wiki page), including the above questions, would be a good idea.

Tom Clegg wrote:

Also missing: a "parent container" or "parent container request" concept to build process trees.

This is what requesting_container_uuid is for, if I get your drift. E.g., "cancel all active ContainerRequests that were initiated by this Container" in the issue description.

I think we should distinguish between infrastructure failure and task failure by distinguishing between "TempFail" and "PermFail" in the container state.

I disagree. Once a container stops, it stays stopped; there is nothing temporary about it. A client's decision about whether to start a new container B after container A fails is not a property of Container A.

"tempfail" may be a bad name. However in principal I think it is important to try and distinguish between "this job could not complete for reasons outside of its control" and "this job failed due to user mistake" (if nothing else, so that the end user knows when a task failure is not their fault). The decision to schedule a container B may not be a "property" of A but it is at least partly dependent on the outcome of A.

Deciding how much a given Container counts toward a ContainerRequest's container_count_max is an interesting issue, but however we do it I don't think it goes in the Container and I don't think it should block this story.

No it doesn't have to block this story but figuring out where retry logic is going to live (API server, dispatcher, workflow runner...) has major design implications.

We probably do need to add something like "exit_code" to Container but let's implement what we have rather than wait for that bikeshed to be painted. Database migrations aren't that hard.

I'm trying to pin down what "Failed" is intended to mean for a container. Perhaps you are suggesting that a job that runs to completion regardless of its exit code is "Complete" and that "Failed" be reserved for infrastructure errors?

I don't think any of the other points in notes 8..11 need to be settled in order to implement this story as described.

Ok. If the goal is to implement the minimum API server support to be able to start prototyping the dispatcher next sprint, then I'm on board with that.

#6518 is the dispatch story. Starting a page about how a dispatcher is supposed to work (on a Crunch2 dispatch wiki page), including the above questions, would be a good idea.

I'll start adding notes on that page.

Peter Amstutz wrote:

Ok. If the goal is to implement the minimum API server support to be able to start prototyping the dispatcher next sprint, then I'm on board with that.

Indeed. From the story description: "For this initial implementation, focus on implementing and testing a minimal feature set establishing the life cycle of Container and ContainerRequest objects, thus enabling us to develop and test other Crunch2 components."

The idea is that implementing this now as (imperfectly) specified will unblock several other stories; this means we'll have some code churn as we perfect the models, but unblocking is important.

6429-crunch2-api ready for review.

at 6fc8952

apps/workbench/app/models/container_requests.rb and containers.rb -- filenames should be singular

• This looks like it would be more comfortable as a ContainerRequest instance method -- like c=req.resolve() instead of c=Container.resolve(req)?
• Literal string "Committed" should be ContainerRequest.Committed

I'm not seeing why Container#process_tree_priority is needed. It looks like a "cancel a container by setting its priority to 0" feature but I don't think we want this. The "sometimes A=0 because B=0, and sometimes B=0 because A=0" aspect seems confusing; I think "user always sets request priority, system always sets container priority" would be easier to follow.

• command, environment, mounts, runtime_constraints, and properties should be text instead of varchar(255) (containers.environment is right, but the rest aren't)
• container_count_max should be NOT NULL

services/api/test/factories/*.rb look bogus, I'm guessing "rails g migration" made them and they should be deleted?

The where/each/save loop in Container#request_finalize should use ContainerRequest.update_all({state: ContainerRequest.Final}, ['container_uuid=?', uuid])

Error message "Cannot only update container_uuid to nil." should say "can", not "cannot".

Errors should not be capitalized, and should not repeat the field name (avoid "container_uuid Has not been resolved to a container" and "Finished at Illegal update of field finished_at" coming from x.full_messages)

ContainerRequest state changes Final→Committed and Committed→Uncommitted are currently possible, but should not be.

The way state transitions are checked is rather verbose and error-prone. How about starting off validate_change by doing something like "if !state_transitions[state_was].include?(state)", and then doing all the other state-dependent validations once we've got that out of the way?

state_transitions = {
  nil => [Uncommitted, Committed],
  Uncommitted => [Uncommitted, Committed],
  Committed => [Committed, Final],
  Final => [Final],
}

Tom Clegg wrote:

at 6fc8952

apps/workbench/app/models/container_requests.rb and containers.rb -- filenames should be singular

Fixed.

Container.resolve(req)

• This looks like it would be more comfortable as a ContainerRequest instance method -- like c=req.resolve() instead of c=Container.resolve(req)?

Done.

• Literal string "Committed" should be ContainerRequest.Committed

Fixed.

I'm not seeing why Container#process_tree_priority is needed. It looks like a "cancel a container by setting its priority to 0" feature but I don't think we want this. The "sometimes A=0 because B=0, and sometimes B=0 because A=0" aspect seems confusing; I think "user always sets request priority, system always sets container priority" would be easier to follow.

Ambigious use of the word "cancel" in the description:

"When changing Container state away from Running, cancel all active ContainerRequests that were initiated by this Container (see requesting_container_uuid)."

Since "priority == 0" means "please cancel this" I interpreted this to mean that the container request should have it priority adjusted.

But I think what we actually want is that when the container completes, both the container request that created the container, and any container requests created by the container are finalized.

Migration

• command, environment, mounts, runtime_constraints, and properties should be text instead of varchar(255) (containers.environment is right, but the rest aren't)
• container_count_max should be NOT NULL

Fixed.

services/api/test/factories/*.rb look bogus, I'm guessing "rails g migration" made them and they should be deleted?

Correct. Removed.

The where/each/save loop in Container#request_finalize should use ContainerRequest.update_all({state: ContainerRequest.Final}, ['container_uuid=?', uuid])

I disagree with this, because update_all skips validation and callbacks and as a result makes it harder to reason about the behavior (by having to consider code paths that don't run validations or callbacks...)

Error message "Cannot only update container_uuid to nil." should say "can", not "cannot".

Fixed.

Errors should not be capitalized, and should not repeat the field name (avoid "container_uuid Has not been resolved to a container" and "Finished at Illegal update of field finished_at" coming from x.full_messages)

Fixed.

ContainerRequest state changes Final→Committed and Committed→Uncommitted are currently possible, but should not be.

The way state transitions are checked is rather verbose and error-prone. How about starting off validate_change by doing something like "if !state_transitions[state_was].include?(state)", and then doing all the other state-dependent validations once we've got that out of the way?

You're right, that's a nicer way to implement it. Done.

Peter Amstutz wrote:

Ambigious use of the word "cancel" in the description:

"When changing Container state away from Running, cancel all active ContainerRequests that were initiated by this Container (see requesting_container_uuid)."

Since "priority == 0" means "please cancel this" I interpreted this to mean that the container request should have it priority adjusted.

But I think what we actually want is that when the container completes, both the container request that created the container, and any container requests created by the container are finalized.

No, sorry, I think I was confused here: I misread the process_tree_priority method and thought it was cancelling the CRs that requested the finished container. It's cancelling the CRs that were requested by the finishing job, which is right. If the requester abandons them by exiting without waiting for the CRs to be satisfied, there is no point in satisfying them, or leaving them running (unless of course some other CRs are using them too). So we just have to set req.priority=0 and the CR hooks will take care of updating priority on the relevant containers, etc.

• if state_was == Running and state_changed?
    act_as_system_user do
      ContainerRequest.where('requesting_container_uuid=? and priority>0 and state in (?)', uuid, [Queued,Running]).each do |cr|
        cr.priority = 0
        cr.save!
      end
    end
  end

We don't want to touch the priority of the CRs that requested the finished container. We do want to finalize them -- at least if we're not going to reattempt, which we never do yet -- but that's already handled in request_finalize.

The where/each/save loop in Container#request_finalize should use ContainerRequest.update_all({state: ContainerRequest.Final}, ['container_uuid=?', uuid])

I disagree with this, because update_all skips validation and callbacks and as a result makes it harder to reason about the behavior (by having to consider code paths that don't run validations or callbacks...)

Fair enough. In fact, I suppose we'll crash here if there are any Uncommitted CRs referencing this container. We should select for state=Committed -- and I suppose CRs with state=Uncommitted should get their container_uuid reset to nil instead? This seems like a worthwhile test case.

• Some of the test names like "Container container complete" are a bit mysterious
• Using c for a ContainerRequest, and t for a Container, seems a bit obfuscated (how about cr and c?)

•    t = Container.find_by_uuid c.container_uuid
      assert_equal 5, t.priority
  
      c.state = "Final" 
      c.save!
  
      t.reload
      assert_equal 0, t.priority
  

Tom Clegg wrote:

No, sorry, I think I was confused here: I misread the process_tree_priority method and thought it was cancelling the CRs that requested the finished container. It's cancelling the CRs that were requested by the finishing job, which is right. If the requester abandons them by exiting without waiting for the CRs to be satisfied, there is no point in satisfying them, or leaving them running (unless of course some other CRs are using them too). So we just have to set req.priority=0 and the CR hooks will take care of updating priority on the relevant containers, etc.

Ok, the hook is now "handle_completed"

When the state changes to Cancelled or Complete:

• CR's that requested this container are moved to Final
• Committed CR's requested by this container have their priority set to 0

This will have the effect that child container requests are will be cancelled from the top down; a child process won't be cancelled until the parent process has terminated.

We could also propagate priority changes directly: setting priority of the parent container (to anything, not just 0) could automatically propagate downward to any child container requests; this would allow for raising or lowing priority on the whole process tree.

Fair enough. In fact, I suppose we'll crash here if there are any Uncommitted CRs referencing this container. We should select for state=Committed -- and I suppose CRs with state=Uncommitted should get their container_uuid reset to nil instead? This seems like a worthwhile test case.

It already only selects committed container requests.

Can you do a bit of copy editing in the tests?

• Some of the test names like "Container container complete" are a bit mysterious
• Using c for a ContainerRequest, and t for a Container, seems a bit obfuscated (how about cr and c?)

Done.

Containers API says priority should be null (not zero) if state != Committed:

• [...]

Done, added a nil validation check and tests.

A test for the "cancel remaining child jobs via requesting_container_uuid when parent ends" behavior would still be good (unless it's here and I'm just not seeing it?). The rest LGTM, thanks.

Tom Clegg wrote:

A test for the "cancel remaining child jobs via requesting_container_uuid when parent ends" behavior would still be good (unless it's here and I'm just not seeing it?). The rest LGTM, thanks.

I had just such a test, and it got lost in the confusion about process trees from note-18. Restored the test.