Bug 1368058 - The counter ae_state_retries is not incremented if $evm.root['ae_result'] = 'retry' is set in a state machine on_exit method
Summary: The counter ae_state_retries is not incremented if $evm.root['ae_result'] = '...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat CloudForms Management Engine
Classification: Red Hat
Component: Automate
Version: 5.6.0
Hardware: Unspecified
OS: Unspecified
high
high
Target Milestone: GA
: 5.8.0
Assignee: mkanoor
QA Contact: Milan Falešník
URL:
Whiteboard: automate
Depends On:
Blocks: 1368176 1413769
TreeView+ depends on / blocked
 
Reported: 2016-08-18 08:59 UTC by Peter McGowan
Modified: 2017-06-12 16:45 UTC (History)
6 users (show)

Fixed In Version: 5.8.0.0
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1368176 1413769 (view as bug list)
Environment:
Last Closed: 2017-06-12 16:45:41 UTC
Category: ---
Cloudforms Team: ---
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)
Reproducer domain (4.57 KB, application/zip)
2017-01-25 12:14 UTC, Milan Falešník
no flags Details

Description Peter McGowan 2016-08-18 08:59:09 UTC
Description of problem:
If I set $evm.root['ae_result'] = 'retry' in an on_exit method of a state machine, the state is retried as expected, but the ae_state_retries counter is not incremented.

Version-Release number of selected component (if applicable):
5.6.0.13

How reproducible:
Every time

Steps to Reproduce:
1. Create a state machine that has an on_error method that sets $evm.root['ae_result'] = 'retry' and a suitable $evm.root['ae_retry_interval'].

Actual results:
The state is retried but the ae_state_retries counter stays at 0

Expected results:
The ae_state_retries counter should be incremented for each retry.

Additional info:

in miq_ae_state_machine it seems that increment_state_retries is only called after process_state_relationship and not process_state_method(f, 'on_exit').

For a usability point of view we should be able to issue a state retry from any of the ae_state_step methods.

Comment 2 mkanoor 2016-08-18 14:17:38 UTC
Peter,
The retry_logic was meant only for the main state and not for the on_entry, on_exit or on_error. The problem I guess is because we allow the scripts to directly set the root['ae_result'], which gets processed differently.

I think for each of the methods we need to validate what they can set 

on_entry -> ae_result => ok | error | skip, ae_next_state => state_name

on_exit  -> ae_result => ok | error 

on_error -> ae_result -> ok | error | continue, ae_next_state => state_name

main state -> ae_result -> ok| error | retry | restart, ae_next_state => state_name

The problem with this enforcement is some of the older models have the actual state embedded in the on_entry, because we lacked the METHOD::

If we allow any method to set any of the variables it could become chaotic and indeterministic, so we have to be careful with this change.

Thanks,
Madhu

Comment 3 Peter McGowan 2016-08-18 14:35:06 UTC
I agree that we should change the older models such as ProvisionRequestApproval to use the METHOD:: style.

I think that on_exit is the natural place to perform state post-processing, and a valid outcome of post processing logic might be that we need to retry the state.

From a usability point of view it would be nice to have the following options:

on_entry -> ae_result => ok | error | skip, ae_next_state => state_name

on_exit  -> ae_result => ok | error | retry

on_error -> ae_result -> abort | continue, ae_next_state => state_name

main state -> ae_result -> ok | error | retry | restart, ae_next_state => state_name

Peter

Comment 6 CFME Bot 2017-01-03 21:36:03 UTC
New commit detected on ManageIQ/manageiq/master:
https://github.com/ManageIQ/manageiq/commit/098176af9e7cee3ca57fa5f8babe46ad4a26214f

commit 098176af9e7cee3ca57fa5f8babe46ad4a26214f
Author:     Madhu Kanoor <mkanoor>
AuthorDate: Tue Jan 3 13:46:12 2017 -0500
Commit:     Madhu Kanoor <mkanoor>
CommitDate: Tue Jan 3 13:46:12 2017 -0500

    Increment the ae_state_retries when on_exit sets retry
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1368058

 .../engine/miq_ae_state_machine.rb                 |  8 +++++++-
 .../engine/miq_ae_state_machine_steps_spec.rb      | 24 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

Comment 9 Milan Falešník 2017-01-25 12:14:53 UTC
Created attachment 1244231 [details]
Reproducer domain

To reproduce:

1) Import the attached file, it will create a domain called OnExitRetry
2) Enable the domain
3) Go to Automate / Simulation
4) Simulate Request with instance OnExitRetry, execute methods
5) Click submit, open the tree on right and expand ae_state_retries

It should be 1 by now and subsequent clicks on Retry should raise the number if it works properly.

Comment 12 Milan Falešník 2017-04-04 12:49:44 UTC
Verified in 5.8.0.8, the ae_state_retries increases.


Note You need to log in before you can comment on or make changes to this bug.