Story #2352

Pipeline instance state is represented with a state machine instead of "active" and "success" flags

Added by Tom Clegg over 7 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
-
Start date:
04/22/2014
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
3.0

Description

Acceptance criteria:
  • User can interrupt/continue pipelines
  • User can run pipeline from command line and doesn't conflict with crunch-dispatch
Ultimately we want to remove the active and success booleans. The only tricky bit is keeping things working during transition. This might work:
  1. add the state attribute (string) -- update all existing records to suitable state during the migration
  2. override active and success methods to return appropriate values based on current state
  3. remove the active and success booleans from the database
  4. override update_attributes so if someone tries to update "active" or "success", the new "state" gets changed instead
  5. fix clients so they read/update state instead of reading/updating active and success
  6. some weeks later: remove the active and success methods and remove from the api response

Subtasks

Task #2472: Replace active/success flags with state attribute to PipelineInstance model in api serverResolvedRadhika Chippada

Task #2473: Update crunch-dispatchResolvedRadhika Chippada

Task #2478: Update workbenchResolvedRadhika Chippada

Task #2477: Update arv-run-pipeline-instanceResolvedRadhika Chippada

Task #2677: Review 2352-phased-pipeline-instance-stateResolvedTom Clegg

Task #2725: 2352-use-state branch is ready for reviewResolvedRadhika Chippada

Task #2805: review 2352-remove-attrs branchResolvedTom Clegg

History

#1 Updated by Tom Clegg over 7 years ago

  • Target version set to 2014-04-16 Dev tools and data/resource management

#2 Updated by Peter Amstutz over 7 years ago

  • Assigned To set to Peter Amstutz

#3 Updated by Peter Amstutz over 7 years ago

  • Subject changed from User can stop and restart a pipeline that is being run by crunch-dispatch. to Pipelines status is represented with a state machine
Acceptance criteria:
  • User can interrupt/continue pipelines
  • User can run pipeline from command line and doesn't conflict with crunch-dispatch

#4 Updated by Peter Amstutz over 7 years ago

  • Description updated (diff)

#5 Updated by Peter Amstutz over 7 years ago

  • Description updated (diff)

#6 Updated by Peter Amstutz over 7 years ago

  • Assigned To changed from Peter Amstutz to Radhika Chippada

#7 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#8 Updated by Tom Clegg over 7 years ago

  • Subject changed from Pipelines status is represented with a state machine to Pipeline instance state is represented with a state machine instead of "active" and "success" flags

#9 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#10 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#11 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#12 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#13 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)
States
  • New
  • Ready
  • RunningOnServer
  • RunningOnClient
  • Paused
  • Failed
  • Complete
Progress
  • Add a components_summary attribute to PipelineInstance -- similar to tasks_summary in Job
    {
     "failed": 0,
     "todo": 4,
     "running": 1,
     "done": 1
    }
    

#14 Updated by Tom Clegg over 7 years ago

Bits that will have to be updated:
  • crunch_dispatch should look for state=RunningOnServer (instead of active=true) when deciding which pipeline instances it should be trying to run
  • Workbench pipeline instance index and show pages should look at state instead of active/complete flags when deciding what label to put on each pipeline instance
  • Workbench pipeline instance "specify inputs and start" page (show?) should only enable the "start" button only if state is Ready or Paused -- and should only show the "choose inputs" page if state=New or state=Ready
  • arv-run-pipeline-instance should set state=New when creating a pipeline instance
  • arv-run-pipeline-instance should set state=RunningOnClient if --run-here is used and arv-run-pipeline-instance is in fact still running
  • arv-run-pipeline-instance should set state=Paused before exiting, if --run-here is used (unless of course state=Failed or Complete by that point)
  • arv-run-pipeline-instance should set state=RunningOnServer if --run-on-server was used
  • API server should change from "New" to "Ready" when a pipeline instance is created or updated, if all required parameters are provided (unfortunately this duplicates some of the "ready to run?" logic from arv-run-pipeline-instance, for now)

#15 Updated by Tom Clegg over 7 years ago

  • Status changed from New to In Progress

#16 Updated by Tom Clegg over 7 years ago

  • Target version changed from 2014-04-16 Dev tools and data/resource management to 2014-05-07 Storing and Organizing Data

#17 Updated by Radhika Chippada over 7 years ago

  • Story points changed from 2.0 to 3.0

#18 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#19 Updated by Tom Clegg over 7 years ago

Notes on 2352-phased-pipeline-instance-state (5cb205c567c312345376bcd2b7104075b5710d7f)
  • is_ready should be an instance method, and operate on the instance's own components. Could also call it components_look_ready? (might be more clear that it doesn't just test state==Ready or something like that). Then:
    - if PipelineInstance.is_ready pi.components
    + if pi.components_look_ready?
  • Perhaps if there are no components, components_look_ready? should return false -- seems like state should probably stay at New instead of Ready in this case.
  • Is there a reason to accept "false", "0", etc. as false? I prefer to expect required to be a genuine false. (This seems like a bad hole to start digging. If we really must have a list of values that are false, we should at least use the same list throughout the application. But I'd be more inclined to go with "if required is something other than true or false, the component is not correctly specified and therefore never considered ready, so we refuse to switch to RunningOnServer or Ready.")
  • In tests, {"required" => "true"} should probably be {"required" => true} unless you're deliberately testing "other values that evaluate to true" behavior.
  • Should test {"required" => false}
  • Maybe add a little comment to the migration =begin / =end blocks to explain why they're commented out.
  • In elsif state_changed? block
    • This is probably the part that will remain after we get rid of vestiges of success and active
    • Do something for Paused
    • Fail validation if someone updates to state="bogus"

#20 Updated by Tom Clegg over 7 years ago

In sdk/cli/bin/arv-run-pipeline-instanceif $options[:submit]
  • Should set :state => 'RunningOnClient' here?
In services/api/app/models/pipeline_instance.rbverify_status
  • I think we should automatically state to "Ready" if the client sets/changes state to "New" but the components look ready? (If I'm reading it correctly, as it stands the client could create a new instance with components that look ready and set state to New, but the server would not admit that it thinks the instance is Ready until the client saves it again with different components or state. Perhaps change the last "elsif" to a distinct "if" block?)

#21 Updated by Tom Clegg over 7 years ago

Oh, and update PipelineInstance.queue to look for state = 'RunningOnClient' instead of active = true:

  def self.queue
    self.where('active = true')
  end

...then crunch-dispatch will leave RunningOnClient pipeline instances alone.

#22 Updated by Ward Vandewege over 7 years ago

  • Target version changed from 2014-05-07 Storing and Organizing Data to 2014-05-28 Pipeline Factory

#23 Updated by Tom Clegg over 7 years ago

@ eff3277

Hm. What I wrote above doesn't make sense -- I should have said RunningOnServer, right? So instead of

self.where("active = true or state = 'RunningOnClient'")

we should have

self.where("state = ?", RunningOnServer)

(PipelineInstance.queue is only used by crunch_dispatch so it should only return RunningOnServer, not RunningOnClient -- and active=true is not necessary to check any more now that we have "state" -- right?)

Everything else looks good.

#24 Updated by Tom Clegg over 7 years ago

@ faf355b

If you start a pipeline, then stop it, it goes to "Paused" state, right? In that case I think this should use editable:true for either @object.state=='New' or @object.state=='Ready'

+  <% if @object.state == 'New' %>
+    <%= render partial: 'show_components_editable', locals: {editable: true} %>
+  <% else %>
+    <%= render partial: 'show_components_editable', locals: {editable: false} %>
+  <% end %>

Remove obsolete code instead of leaving it commented out (we always have git history if we want them back):

-      @instance[:active] = moretodo
+#      @instance[:active] = moretodo

In the "cleanup" function I think the main thing is to prevent "says RunningOnClient but in fact the client has stopped", so instead of

    if @instance
#      @instance[:active] = false
      @instance[:state] = 'New'  # should it be Failed?
      @instance.save
    end

maybe just

if @instance and @instance[:state] == 'RunningOnClient'
  @instance[:state] = 'Paused'
  @instance.save
end

(If the instance is in any other state when a-r-p-i crashes or gets ^C, it should probably be left alone.)

Things sure do look better with "state==X" than with "active && !success" etc.!

#25 Updated by Radhika Chippada over 7 years ago

  • Status changed from In Progress to Resolved

This had been a very interesting story to work on. I am pleased with what had been accomplished in this story.

Also available in: Atom PDF