Arvados

Design proposal to implement this story in a way that also fixes #3859

• Changing state from "queued" to "running" also sets is_locked_by_uuid and started_at. Fails if the starting state is not "queued".
• Change is_locked_by_uuid to is_locked_by_token which is the API token that was issued to job instead of the user uuid. Restrict is_locked_by_token column from everybody except admins and crunch-dispatch.
• Restrict further changes to the job record to that API token.
• Set finished_at when state changes from "Running" to one of Complete, Cancelled, Failed

"Consider what happens when running a job in local mode"

Notes @ 9daebff...

• -    ended = @components.map { |cname, c| 
  -      if c[:job] and ["Complete", "Failed", "Cancelled"].include? c[:job][:state] then 1 else 0 end 
  -    }.reduce(:+) || 0
  -
  -    succeeded = @components.map { |cname, c| 
  -      if c[:job] and ["Complete"].include? c[:job][:state] then 1 else 0 end 
  -    }.reduce(:+) || 0
  -
  -    failed = @components.map { |cname, c| 
  -      if c[:job] and ["Failed", "Cancelled"].include? c[:job][:state] then 1 else 0  end 
  -    }.reduce(:+) || 0
  
  +    c_in_state = @components.values.group_by { |c| 
  +      c[:job] and c[:job][:state]
  +    }
  +    succeeded = c_in_state["Complete"].count
  +    failed = c_in_state["Failed"].count + c_in_state["Cancelled"].count
  +    ended = succeeded + failed
  

• -exit ($Job->{'success'} ? 1 : 0);
  +exit ($Job->{'state'} != 'Complete' ? 1 : 0);
  

• -      if ($Job->{'cancelled_at'}) {
  -        Log (undef, "Job cancelled at " . $Job->{cancelled_at} .
  +      if ($Job->{'state'} ne "Running") {
  +        Log (undef, "Job state changed to " . $Job->{'state'} . " at " . $Job->{cancelled_at} .
                " by user " . $Job->{cancelled_by_user_uuid});
  

• +  if ($Job->{'cancelled_at'}) {
  +    $Job->update_attributes('state' => 'Cancelled',
  +                            'finished_at' => scalar gmtime);
  

• workbench pipeline_instances_helper.rb should use new state attr
• workbench Job.state class method should go away (afaict only app/views/application/_job_status_label.html.erb uses this, presumably it should now use "j.state" instead of "Job.state j")

Tom Clegg wrote:

Notes @ 9daebff...

In arv-run-pipeline-instance the map/reduce bits deserve to be less verbose. Suggest something like this:

• [...]

Nice! Fixed.

crunch-job: This error message could contain misleading information if the state changes to something other than Cancelled. I think it's worth separating the expected case ("Job cancelled at ... by ...") from the unexpected case ("Job state unexpectedly changed to X. Who could work this way? I give up."), which would also avoid the misleading message.

• [...]

Fixed.

crunch-job: I think this should test state==Cancelled instead of testing cancelled_at, and therefore shouldn't bother setting state to Cancelled:

• [...]

Fixed.

Todo:

• workbench pipeline_instances_helper.rb should use new state attr

Fixed.

• workbench Job.state class method should go away (afaict only app/views/application/_job_status_label.html.erb uses this, presumably it should now use "j.state" instead of "Job.state j")

If we get rid of Job.state it breaks the one specific case where we're looking at an old pipeline instance record and for some reason the actual job object isn't available so we're looking at the unmigrated copy in 'components'. Unless you feel strongly about it, I would prefer to leave it since it's more effort to change it (also Job::state is used in several other partials).

Now at 2861857

Peter Amstutz wrote:

• workbench Job.state class method should go away (afaict only app/views/application/_job_status_label.html.erb uses this, presumably it should now use "j.state" instead of "Job.state j")

If we get rid of Job.state it breaks the one specific case where we're looking at an old pipeline instance record and for some reason the actual job object isn't available so we're looking at the unmigrated copy in 'components'. Unless you feel strongly about it, I would prefer to leave it since it's more effort to change it (also Job::state is used in several other partials).

Ah, I see now. Perhaps adding a state field when needed in the pipeline helper's "make a fake job object" might be a more appropriate way to do this, though? That way Job::state j could get replaced with j[:state] everywhere, instead of having that mystery scattered throughout and expected of all future code.

Everything else LGTM, thanks!

Tom Clegg wrote:

Ah, I see now. Perhaps adding a state field when needed in the pipeline helper's "make a fake job object" might be a more appropriate way to do this, though? That way Job::state j could get replaced with j[:state] everywhere, instead of having that mystery scattered throughout and expected of all future code.

Fixed, but I can't test it because of other bugs rendering the 'job' portion of pipeline components. I think Tim is working on this so I don't want to duplicate his work.

Missing newline at EOF @services/api/test/fixtures/jobs.yml.

In apps/workbench/app/helpers/pipeline_instances_helper.rb I suspect we should leave pj[:job][:state] alone if it's already there.

Thanks