Add event state machine for refresh. #243

lfu · 2018-01-19T16:03:12Z

This event state machine replaces the builtin method event_action_refresh_sync that waits for the completion of refresh and hence blocks the worker.

Part of ManageIQ/manageiq#16868.
https://bugzilla.redhat.com/show_bug.cgi?id=1534631

@miq-bot add_label bug, fine/yes, gaprindashvili/yes

cc @mkanoor @gmcculloug @tinaafitz

lfu · 2018-01-19T16:05:02Z

gmcculloug · 2018-01-19T19:28:52Z

...ent/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/target_refresh.rb

+                target = @handle.object["refresh_target"]
+                @handle.log(:info, "Refresh target: [#{target}]")
+                raise "Refresh target not found" if target.nil?
+                target


You don't need to do this but since you used tap above this feels like another example.

@handle.object["refresh_target"].tap do |target| @handle.log(:info, "Refresh target: [#{target}]") raise "Refresh target not found" if target.nil? end

gmcculloug · 2018-01-19T19:29:14Z

...nt/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/check_refreshed.rb

+                  @handle.log(:error, "Refresh task with id : #{task_id} not found")
+                  exit(MIQ_ERROR)
+                end
+                task


Same here with tap.

gmcculloug · 2018-01-19T19:36:41Z

...nt/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/check_refreshed.rb

+              end
+
+              def refresh_task
+                task_id = @handle.get_state_var(:task_id).try(:first)


Suggest using a more descriptive key name in place of :task_id, like :refresh_task_id.

gmcculloug · 2018-01-19T19:38:07Z

...nt/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/check_refreshed.rb

+
+              def check_status(task)
+                case task.state
+                when 'Finished'


Move this string into a constant: STATE_FINISHED = 'Finished'.freeze

mkanoor

@lfu For master all the methods would have to be builtin methods and can't be inline methods. These changes are appropriate for G and F release but not for master. For master they need to be builtin methods.

mkanoor · 2018-01-19T21:05:29Z

@lfu
The MiqTask is pretty generic so we should write the builtin method in a way that it can be reused by other methods that create a MiqTask. The task_id can be either passed into the builtin method or it could directly access some well know state var

lfu · 2018-01-19T22:51:37Z

@mkanoor ids of MiqTask is returned by EmsRefresh.queue_refresh. Event handler in this case does not create the task.

jrafanie · 2018-01-22T20:32:19Z

@lfu can you add this BZ to the PR? Thanks!

https://bugzilla.redhat.com/show_bug.cgi?id=1534631

It seems to go together with my change for the authentications.

mkanoor

For future release we might want to use builtin methods.

mkanoor · 2018-01-22T21:06:33Z

...nt/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/check_refreshed.rb

+              end
+
+              def fetch_task(task_id)
+                @handle.object.attributes.sort.each { |k, v| @handle.log(:info, "#{k}: #{v}") }


@lfu
Is this just for logging purpose? Do we need it?

@mkanoor This is for information purpose. It would be useful if anything goes wrong. I would prefer to print it out for our good.

We can get this information from the automation URL in the log already (in a less user-friendly format). This adds more clutter to an already verbose log. I would advise dropping it.

mkanoor · 2018-01-22T21:08:09Z

...nt/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/check_refreshed.rb

+
+              def check_status(task)
+                case task.state
+                when STATE_FINISHED


@lfu What about the error scenario when the task is finished but has an error? will that get flagged as a retry

@mkanoor A MiqTask's state may be one of Initialized, Queued, Active and Finished.

@mkanoor There is a separate status column for capturing that. app/models/miq_task.rb#L8-L12

tinaafitz

@lfu Looks good.

mkanoor · 2018-01-22T22:50:13Z

...nt/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/check_refreshed.rb

+              def check_status(task)
+                case task.state
+                when STATE_FINISHED
+                  @handle.root['ae_result'] = 'ok'


@lfu If the State == "Finished" but Status != "Ok" we should set the ae_result as error and fail the state machine. Otherwise if there is a password failure during refresh we will continue processing the next state which is going to run the policy action.

lfu · 2018-01-23T00:39:05Z

...nt/automate/ManageIQ/System/Event/StateMachines/Refresh.class/__methods__/check_refreshed.rb

+
+              def fetch_task(task_id)
+                @handle.vmdb(:miq_task).find(task_id)
+              rescue MiqAeException::ServiceNotFound


@mkanoor @tinaafitz @gmcculloug Please review this change.
@handle.vmdb(:miq_task).find(task_id) would throw an exception instead of returning nil when task is not found.

Use @handle.vmdb(:miq_task).find_by(:id => task_id)

https://bugzilla.redhat.com/show_bug.cgi?id=1534631

lfu · 2018-01-23T21:41:26Z

End-to-end testing worked well.

https://bugzilla.redhat.com/show_bug.cgi?id=1534631

miq-bot · 2018-01-24T14:52:19Z

Checked commits lfu/manageiq-content@1535271~...56ad2bd with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
10 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

gmcculloug · 2018-01-24T15:13:43Z

@tinaafitz Can you look this over again as some things have changed since you approved.

tinaafitz

@lfu Looks good

lfu · 2018-01-24T16:11:11Z

Change in ManageIQ/manageiq#16868 caused the Travis failure.

mfalesni · 2018-02-01T14:52:07Z

What is the best way to test this change? Where does the state machine get called so I can see how does it work?

lfu · 2018-02-01T16:09:33Z

@mfalesni The state machine is called by reconfigvm_task_complete event.
Run Vm Reconfigure task via UI which would call the state machine at the end of the process. It would be even better if somehow the provider refresh takes longer time to get done.

Add event state machine for refresh. (cherry picked from commit e0ec800) https://bugzilla.redhat.com/show_bug.cgi?id=1550725

simaishi · 2018-03-01T21:55:37Z

Fine backport details:

$ git log -1
commit 473466ed60cf61e69ac7f1cad7ea9b5d08958cbd
Author: Greg McCullough <[email protected]>
Date:   Thu Jan 25 09:02:48 2018 -0500

    Merge pull request #243 from lfu/event_action_refresh_sync
    
    Add event state machine for refresh.
    (cherry picked from commit e0ec80069f3a5e2c75079fa519e9dfbf18e6771d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1550725

Follow up of ManageIQ/manageiq-content#243. https://bugzilla.redhat.com/show_bug.cgi?id=1550725

Add event state machine for refresh. (cherry picked from commit e0ec800) https://bugzilla.redhat.com/show_bug.cgi?id=1550724

simaishi · 2018-03-06T21:52:24Z

Gaprindashvili backport details:

$ git log -1
commit 068779473d7cbc266754277d9b14793df00dc90c
Author: Greg McCullough <[email protected]>
Date:   Thu Jan 25 09:02:48 2018 -0500

    Merge pull request #243 from lfu/event_action_refresh_sync
    
    Add event state machine for refresh.
    (cherry picked from commit e0ec80069f3a5e2c75079fa519e9dfbf18e6771d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1550724

Follow up of ManageIQ/manageiq-content#243. https://bugzilla.redhat.com/show_bug.cgi?id=1550725

miq-bot added bug fine/yes labels Jan 19, 2018

lfu force-pushed the event_action_refresh_sync branch from 4daa344 to b58ef1e Compare January 19, 2018 16:11

gmcculloug requested a review from mkanoor January 19, 2018 16:27

gmcculloug self-assigned this Jan 19, 2018

lfu force-pushed the event_action_refresh_sync branch 3 times, most recently from 162b676 to ddda53e Compare January 19, 2018 18:39

lfu changed the title ~~Initial commit of event state machine for refresh.~~ Add event state machine for refresh. Jan 19, 2018

lfu force-pushed the event_action_refresh_sync branch from ddda53e to a44a5b3 Compare January 19, 2018 18:53

gmcculloug reviewed Jan 19, 2018

View reviewed changes

mkanoor suggested changes Jan 19, 2018

View reviewed changes

lfu force-pushed the event_action_refresh_sync branch from a44a5b3 to 0817147 Compare January 19, 2018 21:52

mkanoor approved these changes Jan 22, 2018

View reviewed changes

mkanoor reviewed Jan 22, 2018

View reviewed changes

tinaafitz approved these changes Jan 22, 2018

View reviewed changes

lfu force-pushed the event_action_refresh_sync branch 2 times, most recently from 335d1d0 to a40dc43 Compare January 22, 2018 22:48

mkanoor reviewed Jan 22, 2018

View reviewed changes

lfu force-pushed the event_action_refresh_sync branch 2 times, most recently from fafe199 to 51c2755 Compare January 23, 2018 00:35

lfu commented Jan 23, 2018

View reviewed changes

lfu force-pushed the event_action_refresh_sync branch from 51c2755 to b69ebd1 Compare January 23, 2018 14:07

Add event state machine for refresh.

1535271

https://bugzilla.redhat.com/show_bug.cgi?id=1534631

lfu force-pushed the event_action_refresh_sync branch from 47d7bb9 to 97da41d Compare January 23, 2018 21:23

lfu closed this Jan 24, 2018

lfu reopened this Jan 24, 2018

Add test cases for event state machines.

56ad2bd

https://bugzilla.redhat.com/show_bug.cgi?id=1534631

lfu force-pushed the event_action_refresh_sync branch from 97da41d to 56ad2bd Compare January 24, 2018 14:42

tinaafitz approved these changes Jan 24, 2018

View reviewed changes

gmcculloug approved these changes Jan 25, 2018

View reviewed changes

gmcculloug merged commit e0ec800 into ManageIQ:master Jan 25, 2018

gmcculloug added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 25, 2018

simaishi pushed a commit that referenced this pull request Mar 1, 2018

Merge pull request #243 from lfu/event_action_refresh_sync

473466e

Add event state machine for refresh. (cherry picked from commit e0ec800) https://bugzilla.redhat.com/show_bug.cgi?id=1550725

simaishi added fine/backported and removed fine/yes labels Mar 1, 2018

lfu mentioned this pull request Mar 2, 2018

[FINE] Remove the options hash which is new in version 5.9. #256

Merged

lfu added a commit to lfu/manageiq that referenced this pull request Mar 2, 2018

Add service model for MiqTask which is needed by event state machine.

f4e1a2a

Follow up of ManageIQ/manageiq-content#243. https://bugzilla.redhat.com/show_bug.cgi?id=1550725

lfu added a commit to lfu/manageiq that referenced this pull request Mar 2, 2018

Add service model for MiqTask which is needed by event state machine.

4e077e5

Follow up of ManageIQ/manageiq-content#243. https://bugzilla.redhat.com/show_bug.cgi?id=1550725

lfu mentioned this pull request Mar 2, 2018

[FINE] Add service model for MiqTask which is needed by event state machine. ManageIQ/manageiq#17081

Merged

simaishi pushed a commit that referenced this pull request Mar 6, 2018

Merge pull request #243 from lfu/event_action_refresh_sync

0687794

Add event state machine for refresh. (cherry picked from commit e0ec800) https://bugzilla.redhat.com/show_bug.cgi?id=1550724

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Mar 6, 2018

d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018

Add service model for MiqTask which is needed by event state machine.

1a73279

Follow up of ManageIQ/manageiq-content#243. https://bugzilla.redhat.com/show_bug.cgi?id=1550725

lfu deleted the event_action_refresh_sync branch September 29, 2018 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add event state machine for refresh. #243

Add event state machine for refresh. #243

lfu commented Jan 19, 2018 •

edited

Loading

lfu commented Jan 19, 2018 •

edited

Loading

gmcculloug Jan 19, 2018

gmcculloug Jan 19, 2018

gmcculloug Jan 19, 2018

gmcculloug Jan 19, 2018

mkanoor left a comment

mkanoor commented Jan 19, 2018

lfu commented Jan 19, 2018 •

edited

Loading

jrafanie commented Jan 22, 2018

mkanoor left a comment

mkanoor Jan 22, 2018

lfu Jan 22, 2018

gmcculloug Jan 22, 2018

mkanoor Jan 22, 2018

lfu Jan 22, 2018

gmcculloug Jan 22, 2018

tinaafitz left a comment

mkanoor Jan 22, 2018

lfu Jan 23, 2018

gmcculloug Jan 23, 2018

lfu commented Jan 23, 2018

miq-bot commented Jan 24, 2018

gmcculloug commented Jan 24, 2018

tinaafitz left a comment

lfu commented Jan 24, 2018

mfalesni commented Feb 1, 2018

lfu commented Feb 1, 2018

simaishi commented Mar 1, 2018

simaishi commented Mar 6, 2018

Add event state machine for refresh. #243

Add event state machine for refresh. #243

Conversation

lfu commented Jan 19, 2018 • edited Loading

lfu commented Jan 19, 2018 • edited Loading

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

mkanoor left a comment

Choose a reason for hiding this comment

mkanoor commented Jan 19, 2018

lfu commented Jan 19, 2018 • edited Loading

jrafanie commented Jan 22, 2018

mkanoor 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

tinaafitz 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

lfu commented Jan 23, 2018

miq-bot commented Jan 24, 2018

gmcculloug commented Jan 24, 2018

tinaafitz left a comment

Choose a reason for hiding this comment

lfu commented Jan 24, 2018

mfalesni commented Feb 1, 2018

lfu commented Feb 1, 2018

simaishi commented Mar 1, 2018

simaishi commented Mar 6, 2018

lfu commented Jan 19, 2018 •

edited

Loading

lfu commented Jan 19, 2018 •

edited

Loading

lfu commented Jan 19, 2018 •

edited

Loading