Provider native operations state machine #14405

agrare · 2017-03-20T13:58:10Z

This provides the basics for a provider extensible CRUD state machine.

The basic workflow is:

Run a native operation and wait for it to complete
Start a refresh and wait for it to complete
Notify the user about the operation completing successfully or failing

State transition diagram:

    *                          /------\
    | :initialize              |      |  :poll_native_task
    v               :start     v      |
 waiting_to_start  -------> running ----------> refreshing <-\
                               |     :refresh        |       | :poll_refresh
                               v                     |-------/
                             error <-----------------|
                               |                     |
                               v                     |
       finished <---------- notifying --------------/
                   :finish              :notify

Examples of a successful and an unsuccessful notification from a NativeCrud job:

Fryguy · 2017-03-20T21:40:16Z

Overall LGTM. Still wonder if it might be nicer to use the better state machine system.

blomquisg · 2017-03-22T17:42:11Z

I love the ascii art state diagram.

Also:

Overall LGTM. Still wonder if it might be nicer to use the better state machine system.

LOL, I heard this coming before I read it.

blomquisg

Excited about this!

I have a few comments, but nothing really mandatory.

blomquisg · 2017-03-22T17:52:18Z

app/models/manageiq/providers/native_crud.rb

+ # finished <---------- notifying --------------/
+ # :finish :notify
+ #
+ def load_transitions


Could this method be protected?

I'm thinking of the possibility of later converging on one state machine. And this feels a bit like an internal detail that I would hope could be hidden away.

It certainly can, done

blomquisg · 2017-03-22T18:02:02Z

app/models/manageiq/providers/native_crud.rb

+ end
+ end
+
+ if refresh_finished


Can refresh_finished? become a private method that's called by poll_refresh?

Then, this method becomes:

def poll_refresh if refreshed_finished? queue_signal(:notify) else queue_signal(:poll_refresh, :deliver_on => Time.now.utc + 1.minute) end end

I suppose you could one-line that into a ternary, but the if/else looks fine to me.

Can refresh_finished? become a private method that's called by poll_refresh?

Yeah I like that a lot, I'll just need to handle the refresh task error differently since right now it is returning out of poll_refresh and signaling :error

blomquisg · 2017-03-22T18:03:45Z

app/models/manageiq/providers/native_crud.rb

@@ -0,0 +1,129 @@
+class ManageIQ::Providers::NativeCrud < Job


Naming is hard...

ManageIQ::Providers::NativeOperationWorkflow?

+1 for not Crud

I guess it's for any operation thats to be scheduled (async) and which needs a refresh after it.

Yeah I like Workflow, aligns better with the miq_provision_workflow/miq_request_workflow

blomquisg · 2017-03-22T18:06:25Z

app/models/manageiq/providers/native_crud.rb

+ # v |
+ # finished <---------- notifying --------------/
+ # :finish :notify
+ #


agrare · 2017-03-22T20:56:44Z

Okay moved everything from NativeCrud -> NativeOperationWorkflow

miq-bot · 2017-03-27T22:42:01Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-04-05T08:37:30Z

This pull request is not mergeable. Please rebase and repush.

jameswnl · 2017-04-05T19:44:00Z

spec/models/manageiq/providers/native_operation_workflow_spec.rb

+ @job.signal(:start)
+ end
+
+ it 'doesn\'t allow poll_native_task signal' do


nitpick: if you use double quote, then you don't have to escape the '

Yeah, I started with ' and wanted to keep it consistent...maybe should switch to " everywhere here

miq-bot · 2017-05-02T14:58:12Z

Checked commits agrare/manageiq@a32c8a1~...1764f21 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

Fryguy · 2017-05-02T22:13:36Z

This LGTM... @blomquisg if you're happy with it, please merge.

durandom · 2017-05-03T16:08:43Z

@agrare do you plan using this anywhere? Otherwise I could use https://www.pivotaltracker.com/story/show/144430761 to make use of this...

agrare · 2017-05-03T16:11:25Z

@durandom I was going to start moving some vm power ops over to use this, but by all means start using/modifying it for whatever you want :)

the sleep here is also needed because tower needs some time to actually propagate it's updates if we would return immediately it _could_ be that the we get the old playbooks the whole sleep business is a workaround anyway until we get proper polling via ManageIQ#14405

the sleep here is also needed because tower needs some time to actually propagate it's updates if we would return immediately it _could_ be that the we get the old playbooks the whole sleep business is a workaround anyway until we get proper polling via ManageIQ/manageiq#14405 (transferred from ManageIQ/manageiq@85090c9)

agrare · 2018-03-23T12:22:48Z

@Glutexo yes it is used by https://github.com/ManageIQ/manageiq/blob/master/app/models/service/resource_linking.rb#L21-L23 via https://github.com/ManageIQ/manageiq/blob/master/app/models/service/linking_workflow.rb.

It is really based around something that needs to do an EmsRefresh and get the resulting object though.

Glutexo · 2018-03-23T12:35:30Z

Sorry, my bad, I deleted my comment instead of editing it. N00b. Pasting here for the context:

Is this actually used somewhere already? I‘ve found a reference to this PR in ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::ConfigurationScriptSource. There is some nasty sleep loop going on, waiting for a project refresh (cloning/sync) in the provider finishes. I guess it’s there instead of its own job so a right error/success notification is sent. It this the case your ManageIQ::Providers::NativeOperationWorkflow can help with? If so, would you mind sharing how, please?

@jameswnl @agrare

agrare · 2018-03-23T12:38:11Z

@Glutexo if you don't need to do the EmsRefresh step you can probably just use the first few states, you should be able to provide your own state transitions in a subclass

Glutexo · 2018-03-23T12:43:55Z

Thanks, @agrare! I’ll take a look at it for reference.

I guess there are two reasons why there are those nasty things going on:

Doing a refresh (unfortunately untargeted this time, but I guess it can be fixed) after creating a resource in EMS, verifying it is really present in there.
Waiting for the resource (project) to do its initial refresh/update/sync (which takes some time) in the EMS right after it is created. This is done so we can deliver success/error notification depending on the result of the sync.

From what you said it looks like the NaturalWorkflow is designed just exactly to solve the former case and it can be extended to support the latter as well.

agrare added the wip label Mar 20, 2017

agrare force-pushed the provider_native_crud_state_machine branch 3 times, most recently from 310e18c to 52e4085 Compare March 20, 2017 17:25

agrare requested a review from Fryguy March 20, 2017 18:13

Fryguy requested a review from blomquisg March 20, 2017 21:36

chessbyte assigned blomquisg Mar 20, 2017

chessbyte added enhancement providers labels Mar 20, 2017

agrare force-pushed the provider_native_crud_state_machine branch from 52e4085 to 434a591 Compare March 21, 2017 19:07

agrare changed the title ~~[WIP] Provider native crud state machine~~ Provider native crud state machine Mar 21, 2017

agrare removed the wip label Mar 21, 2017

blomquisg reviewed Mar 22, 2017

View reviewed changes

agrare force-pushed the provider_native_crud_state_machine branch 4 times, most recently from f59205e to db27d48 Compare March 22, 2017 20:05

agrare changed the title ~~Provider native crud state machine~~ Provider native operations state machine Mar 22, 2017

miq-bot added the unmergeable label Mar 27, 2017

agrare force-pushed the provider_native_crud_state_machine branch from db27d48 to 103c4e0 Compare March 28, 2017 21:03

miq-bot removed the unmergeable label Mar 28, 2017

miq-bot added the unmergeable label Apr 5, 2017

agrare force-pushed the provider_native_crud_state_machine branch from 103c4e0 to 454a00b Compare April 5, 2017 12:50

miq-bot removed the unmergeable label Apr 5, 2017

jameswnl reviewed Apr 5, 2017

View reviewed changes

agrare added 9 commits May 2, 2017 10:55

Add notification types for provider native ops

c039100

Wait for refresh and send notification

80cdc8c

Add state-transition diagram

4069c26

Use queue to deliver signals

dbbc89c

Add error handling and set zone/role for queue

b86fd1f

Add spec tests for NativeOperationWorkflow state machine

1956ffa

Make NativeOperationWorkflow#load_transitions protected

201394a

Extract refresh_finished? into its own method

76b255d

Don't nil the status and message when calling finish

1764f21

agrare force-pushed the provider_native_crud_state_machine branch from 3ca48a4 to 1764f21 Compare May 2, 2017 14:55

miq-bot removed the unmergeable label May 2, 2017

Fryguy approved these changes May 2, 2017

View reviewed changes

blomquisg mentioned this pull request May 3, 2017

Tower CUD to invoke targeted refresh #14954

Merged

blomquisg added the fine/no label May 4, 2017

blomquisg merged commit 4872781 into ManageIQ:master May 4, 2017

blomquisg added this to the Sprint 60 Ending May 8, 2017 milestone May 4, 2017

agrare deleted the provider_native_crud_state_machine branch May 4, 2017 19:42

agrare restored the provider_native_crud_state_machine branch May 4, 2017 19:42

agrare deleted the provider_native_crud_state_machine branch May 4, 2017 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provider native operations state machine #14405

Provider native operations state machine #14405

agrare commented Mar 20, 2017 •

edited

Loading

Fryguy commented Mar 20, 2017

blomquisg commented Mar 22, 2017

blomquisg left a comment

blomquisg Mar 22, 2017

agrare Mar 22, 2017

blomquisg Mar 22, 2017

agrare Mar 22, 2017

blomquisg Mar 22, 2017

durandom Mar 22, 2017

agrare Mar 22, 2017

blomquisg Mar 22, 2017

agrare commented Mar 22, 2017

miq-bot commented Mar 27, 2017

miq-bot commented Apr 5, 2017

jameswnl Apr 5, 2017

agrare Apr 5, 2017

miq-bot commented May 2, 2017

Fryguy commented May 2, 2017

durandom commented May 3, 2017

agrare commented May 3, 2017

agrare commented Mar 23, 2018

Glutexo commented Mar 23, 2018

agrare commented Mar 23, 2018

Glutexo commented Mar 23, 2018 •

edited

Loading

		@@ -0,0 +1,129 @@
		class ManageIQ::Providers::NativeCrud < Job

Provider native operations state machine #14405

Provider native operations state machine #14405

Conversation

agrare commented Mar 20, 2017 • edited Loading

Fryguy commented Mar 20, 2017

blomquisg commented Mar 22, 2017

blomquisg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Mar 22, 2017

miq-bot commented Mar 27, 2017

miq-bot commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented May 2, 2017

Fryguy commented May 2, 2017

durandom commented May 3, 2017

agrare commented May 3, 2017

agrare commented Mar 23, 2018

Glutexo commented Mar 23, 2018

agrare commented Mar 23, 2018

Glutexo commented Mar 23, 2018 • edited Loading

agrare commented Mar 20, 2017 •

edited

Loading

Glutexo commented Mar 23, 2018 •

edited

Loading