Skip to content

Add option to specify when to emit st2.generic.actiontrigger #4315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 5, 2018
Merged

Add option to specify when to emit st2.generic.actiontrigger #4315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ab8edb
Add option to specify when to emit actiontrigger
shusugmt Aug 24, 2018
233055a
Fix bug to honor action_sensor.enable config parameter
shusugmt Aug 24, 2018
aa6157c
Merge branch 'master' into selectable-actiontrigger
Kami Sep 4, 2018
dba8b31
Add debug log
shusugmt Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions conf/st2.conf.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
[action_sensor]
# Whether to enable or disable the ability to post a trigger on action.
enable = True
# List of execution statuses that a trigger will be emitted. If not specified, it defaults to action completion.
emit_when = # comma separated list allowed here.

[actionrunner]
# Internal pool size for dispatcher used by regular actions.
Expand Down
37 changes: 23 additions & 14 deletions st2actions/st2actions/notifier/notifier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@

LOG = logging.getLogger(__name__)

ACTION_SENSOR_ENABLED = cfg.CONF.action_sensor.enable
# XXX: Fix this nasty positional dependency.
ACTION_TRIGGER_TYPE = INTERNAL_TRIGGER_TYPES['action'][0]
NOTIFY_TRIGGER_TYPE = INTERNAL_TRIGGER_TYPES['action'][1]
Expand All @@ -79,22 +78,18 @@ def process(self, execution_db):
extra = {'execution': execution_db}
LOG.debug('Processing action execution "%s".', execution_id, extra=extra)

if execution_db.status not in LIVEACTION_COMPLETED_STATES:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but I found previous code a bit easier to read / follow ("return early" vs "nested if", one level less of nesting).

Was there any particular reason why you changed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is inevitable since:

  1. I thought the order of other function calls is important here and shouldn't be changed (to keep this: apply_post_run_policies -> _post_notify_triggers -> _post_generic_trigger)
  2. We always need to call _post_generic_trigger regardless of the status. So If we want to do a "early return" then I need to move that call before if execution_db.status not in LIVEACTION_COMPLETED_STATES: line but that will break the order (_post_generic_trigger will be called before _post_notify_triggers)

msg = 'Skip action execution "%s" because state "%s" is not in a completed state.'
LOG.debug(msg % (str(execution_db.id), execution_db.status), extra=extra)
return

# Get the corresponding liveaction record.
liveaction_db = LiveAction.get_by_id(execution_db.liveaction['id'])

# If the action execution is executed under an orquesta workflow, policies for the
# action execution will be applied by the workflow engine. A policy may affect the
# final state of the action execution thereby impacting the state of the workflow.
if not workflow_service.is_action_execution_under_workflow_context(execution_db):
policy_service.apply_post_run_policies(liveaction_db)
if execution_db.status in LIVEACTION_COMPLETED_STATES:
# If the action execution is executed under an orquesta workflow, policies for the
# action execution will be applied by the workflow engine. A policy may affect the
# final state of the action execution thereby impacting the state of the workflow.
if not workflow_service.is_action_execution_under_workflow_context(execution_db):
policy_service.apply_post_run_policies(liveaction_db)

if liveaction_db.notify is not None:
self._post_notify_triggers(liveaction_db=liveaction_db, execution_db=execution_db)
if liveaction_db.notify is not None:
self._post_notify_triggers(liveaction_db=liveaction_db, execution_db=execution_db)

self._post_generic_trigger(liveaction_db=liveaction_db, execution_db=execution_db)

Expand Down Expand Up @@ -231,11 +226,25 @@ def _get_trace_context(self, execution_id):
return None

def _post_generic_trigger(self, liveaction_db=None, execution_db=None):
if not ACTION_SENSOR_ENABLED:
if not cfg.CONF.action_sensor.enable:
LOG.debug('Action trigger is disabled, skipping trigger dispatch...')
return

execution_id = str(execution_db.id)
extra = {'execution': execution_db}

target_statuses = cfg.CONF.action_sensor.emit_when
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small style thing - I would do target_statuses = cfg.CONF.action_sensor.emit_when or LIVEACTION_COMPLETED_STATES this way we could get rid of else clause.

Besides that, LGTM 👍

if not target_statuses:
if execution_db.status not in LIVEACTION_COMPLETED_STATES:
msg = 'Skip action execution "%s" because state "%s" is not in a completed state.'
LOG.debug(msg % (execution_id, execution_db.status), extra=extra)
return
else:
if execution_db.status not in target_statuses:
msg = 'Skip action execution "%s" because state "%s" is not in %s'
LOG.debug(msg % (execution_id, execution_db.status, target_statuses), extra=extra)
return

payload = {'execution_id': execution_id,
'status': liveaction_db.status,
'start_timestamp': str(liveaction_db.start_timestamp),
Expand Down
64 changes: 64 additions & 0 deletions st2actions/tests/unit/test_notifier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
tests_config.parse_args()

from st2actions.notifier.notifier import Notifier
from st2common.constants.action import LIVEACTION_COMPLETED_STATES
from st2common.constants.action import LIVEACTION_STATUSES
from st2common.constants.triggers import INTERNAL_TRIGGER_TYPES
from st2common.models.api.action import LiveActionAPI
from st2common.models.db.action import ActionDB
Expand Down Expand Up @@ -201,3 +203,65 @@ def test_notify_triggers_jinja_patterns(self, dispatch):
dispatch.assert_called_once_with('core.st2.generic.notifytrigger', payload=exp,
trace_context={})
notifier.process(execution)

@mock.patch('oslo_config.cfg.CONF.action_sensor', mock.MagicMock(
emit_when=[]))
@mock.patch.object(Notifier, '_get_runner_ref', mock.MagicMock(
return_value='run-local-cmd'))
@mock.patch.object(Notifier, '_get_trace_context', mock.MagicMock(
return_value={}))
@mock.patch('st2common.transport.reactor.TriggerDispatcher.dispatch')
def test_post_generic_trigger(self, dispatch):
for status in LIVEACTION_STATUSES:
liveaction_db = LiveActionDB(action='core.local')
liveaction_db.status = status
execution = MOCK_EXECUTION
execution.liveaction = vars(LiveActionAPI.from_model(liveaction_db))
execution.status = liveaction_db.status

notifier = Notifier(connection=None, queues=[])
notifier._post_generic_trigger(liveaction_db, execution)

if status in LIVEACTION_COMPLETED_STATES:
exp = {'status': status,
'start_timestamp': str(liveaction_db.start_timestamp),
'result': {}, 'parameters': {},
'action_ref': u'core.local',
'runner_ref': 'run-local-cmd',
'execution_id': str(MOCK_EXECUTION.id),
'action_name': u'core.local'}
dispatch.assert_called_with('core.st2.generic.actiontrigger',
payload=exp, trace_context={})

self.assertEqual(dispatch.call_count, len(LIVEACTION_COMPLETED_STATES))

@mock.patch('oslo_config.cfg.CONF.action_sensor', mock.MagicMock(
emit_when=['scheduled', 'pending', 'abandoned']))
@mock.patch.object(Notifier, '_get_runner_ref', mock.MagicMock(
return_value='run-local-cmd'))
@mock.patch.object(Notifier, '_get_trace_context', mock.MagicMock(
return_value={}))
@mock.patch('st2common.transport.reactor.TriggerDispatcher.dispatch')
def test_post_generic_trigger_with_emit_condition(self, dispatch):
for status in LIVEACTION_STATUSES:
liveaction_db = LiveActionDB(action='core.local')
liveaction_db.status = status
execution = MOCK_EXECUTION
execution.liveaction = vars(LiveActionAPI.from_model(liveaction_db))
execution.status = liveaction_db.status

notifier = Notifier(connection=None, queues=[])
notifier._post_generic_trigger(liveaction_db, execution)

if status in ['scheduled', 'pending', 'abandoned']:
exp = {'status': status,
'start_timestamp': str(liveaction_db.start_timestamp),
'result': {}, 'parameters': {},
'action_ref': u'core.local',
'runner_ref': 'run-local-cmd',
'execution_id': str(MOCK_EXECUTION.id),
'action_name': u'core.local'}
dispatch.assert_called_with('core.st2.generic.actiontrigger',
payload=exp, trace_context={})

self.assertEqual(dispatch.call_count, 3)
4 changes: 4 additions & 0 deletions st2common/st2common/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,10 @@ def register_opts(ignore_errors=False):
cfg.BoolOpt(
'enable', default=True,
help='Whether to enable or disable the ability to post a trigger on action.'),
cfg.ListOpt(
'emit_when', default=[],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For explicitness sake, you could perhaps default this option to LIVEACTION_COMPLETED_STATES.

This way you could also simple the code here a bit - https://github.com/StackStorm/st2/pull/4315/files#diff-70b51c2693ac263742fc28ad30ebc932R234.

Copy link
Contributor Author

@shusugmt shusugmt Sep 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For explicitness sake, you could perhaps default this option to LIVEACTION_COMPLETED_STATES.

If we specify the list here explicitly, then we need to follow LIVEACTION_COMPLETED_STATES changes that may happen in the future. So I thought this way is better but I'm OK with either option. What do you think?

This way you could also simple the code here a bit

Anyways that test case still needs to be there because users might have chance to specify a null or a blank list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but I don't think that should be a big deal because any change to that constant would also mean user would need to update and restart all the services so the change would be picked up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge this into master and open a small PR with changelog entry and this change here which will allow us to slightly simplify the if statement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And another reason why I prefer to use a default value here is because it's more explicit.

If this option contain no default value here, user needs to dig through the code and documentation to figure out what a default value of None indicates.

help='List of execution statuses that a trigger will be emitted. '
'If not specified, it defaults to action completion.'),
]

do_register_opts(action_sensor_opts, group='action_sensor')
Expand Down