Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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 CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ Fixed
with items task. (bug fix) #4523
* Fix orquesta workflow bug where context variables are being overwritten on task join.
(bug fix) StackStorm/orquesta#112
* Fix `config_context` renders against incorrect pack
(bug fix) StackStorm/st2#4570
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to "Fix rendering of config_context in orquesta task that references action in different pack (bug fix) #4570"?


2.10.3 - March 06, 2019
-----------------------
Expand Down
8 changes: 6 additions & 2 deletions st2common/st2common/services/workflows.py
Original file line number Diff line number Diff line change
Expand Up @@ -541,9 +541,13 @@ def request_action_execution(wf_ex_db, task_ex_db, st2_ctx, ac_ex_req, delay=Non
# Identify the runner for the action.
runner_type_db = action_utils.get_runnertype_by_name(action_db.runner_type['name'])

# Identify action pack name
action_ref = task_ex_db.task_spec.get('spec').get('action')
Copy link
Contributor

Choose a reason for hiding this comment

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

There's already an action_ref at line 519. They're not the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

You did not address this yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not the same. line 519 is get block of action data from action_execution_d_b. This is get action_ref = pack_name:action_name

Not sure about You did not address this yet. means. Changed if else block to one line.

Copy link
Contributor

Choose a reason for hiding this comment

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

The line numbers may have shifted. But if you go down two line from def request_action_execution(wf_ex_db, task_ex_db, st2_ctx, ac_ex_req, delay=None), there's already an action_ref assigned. Is it different than the action_ref begin assigned in this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if they are not the same, then you just overwrote action_ref that is saved to LiveActionDB below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is different. Changed my variable name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just manually tested this and they are the same value. Can you show me an example where they are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I thought it is reading ActionExecution.action. Put log message and tested. Committed change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Good you finally see it. I thought maybe I'm not clear enough.

pack = action_ref.split('.')[0] if action_ref else st2_ctx.get('pack')

# Set context for the action execution.
ac_ex_ctx = {
'pack': st2_ctx.get('pack'),
'pack': pack,
'user': st2_ctx.get('user'),
'parent': st2_ctx,
'orquesta': {
Expand Down Expand Up @@ -1110,4 +1114,4 @@ def update_execution_records(wf_ex_db, conductor, update_lv_ac_on_states=None,
# Invoke post run on the liveaction for the workflow execution.
if status_changed and wf_lv_ac_db.status in ac_const.LIVEACTION_COMPLETED_STATES:
LOG.info('[%s] Workflow action execution is completed and invoking post run.', wf_ac_ex_id)
runners_utils.invoke_post_run(wf_lv_ac_db)
runners_utils.invoke_post_run(wf_lv_ac_db)
Copy link
Contributor

@m4dcoder m4dcoder Mar 7, 2019

Choose a reason for hiding this comment

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

Remove the carriage return at the end of the line here.

74 changes: 74 additions & 0 deletions st2common/tests/unit/services/test_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@
from st2common.exceptions import action as action_exc
from st2common.models.db import liveaction as lv_db_models
from st2common.models.db import execution as ex_db_models
from st2common.models.db.pack import ConfigDB
Copy link
Contributor

Choose a reason for hiding this comment

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

Please stay consistent with imports for this module. Change to from st2common.models.db import pack as pk_db_models and move this line so the import is alphabetical.

from st2common.persistence import execution as ex_db_access
from st2common.persistence import workflow as wf_db_access
from st2common.persistence.pack import Config
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the pack module is imported above, this import is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to have it in order to save it to DB. Later when line 569 param_utils.render_live_params is called, it read config information from DB.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I see. I misread. You have to change the import here to from st2common.persistence import pack as pk_db_access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

from st2common.services import action as action_service
from st2common.services import workflows as workflow_service
from st2common.transport import liveaction as lv_ac_xport
Expand All @@ -44,8 +46,16 @@
TEST_PACK = 'orquesta_tests'
TEST_PACK_PATH = st2tests.fixturesloader.get_fixtures_packs_base_path() + '/' + TEST_PACK

PACK_7 = 'dummy_pack_7'
PACK_7_PATH = st2tests.fixturesloader.get_fixtures_packs_base_path() + '/' + PACK_7

PACK_20 = 'dummy_pack_20'
PACK_20_PATH = st2tests.fixturesloader.get_fixtures_packs_base_path() + '/' + PACK_20

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the pack orquesta_tests as one of the pack here? Also, why dummy_pack_7 and dummy_pack_20? We can't use the other dummy_packs like dummy_pack_1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can move dummy_pack_20 to orquesta_test2.
The reason I used two dummy pack because I was concerning about integration test.
When install dummy_pack_1, there is a error message Failed to register policies: Failed to register policy "/opt/stackstorm/packs/dummy_pack_1/policies/policy_2.yaml" from pack "dummy_pack_1": Referenced policy_type "action.mock_policy_error" doesnt exist

I almost ran all dummy packs and found these two.

PACKS = [
TEST_PACK_PATH,
PACK_7_PATH,
PACK_20_PATH,
st2tests.fixturesloader.get_fixtures_packs_base_path() + '/core'
]

Expand Down Expand Up @@ -363,3 +373,67 @@ def test_evaluate_action_execution_delay(self):
ac_ex_req = {'action': 'core.noop', 'input': None, 'item_id': 1}
actual_delay = workflow_service.eval_action_execution_delay(task_ex_req, ac_ex_req, True)
self.assertIsNone(actual_delay)

def test_request_action_execution_render(self):
# Manually create ConfigDB
output = 'Testing'
value = {
"config_item_one": output
}
config_db = ConfigDB(pack=PACK_7, values=value)
config = Config.add_or_update(config_db)
self.assertEqual(len(config), 3)

wf_meta = self.get_wf_fixture_meta_data(PACK_20_PATH, 'render_config_context.yaml')

# Manually create the liveaction and action execution objects without publishing.
lv_ac_db = lv_db_models.LiveActionDB(action=wf_meta['name'])
lv_ac_db, ac_ex_db = action_service.create_request(lv_ac_db)

# Request the workflow execution.
wf_def = self.get_wf_def(PACK_20_PATH, wf_meta)
st2_ctx = self.mock_st2_context(ac_ex_db)
wf_ex_db = workflow_service.request(wf_def, ac_ex_db, st2_ctx)
spec_module = specs_loader.get_spec_module(wf_ex_db.spec['catalog'])
wf_spec = spec_module.WorkflowSpec.deserialize(wf_ex_db.spec)

# Pass down appropriate st2 context to the task and action execution(s).
root_st2_ctx = wf_ex_db.context.get('st2', {})
st2_ctx = {
'execution_id': wf_ex_db.action_execution,
'user': root_st2_ctx.get('user'),
'pack': root_st2_ctx.get('pack')
}

# Manually request task execution.
task_route = 0
task_id = 'task1'
task_spec = wf_spec.tasks.get_task(task_id)
task_ctx = {'foo': 'bar'}

task_ex_req = {
'id': task_id,
'route': task_route,
'spec': task_spec,
'ctx': task_ctx,
'actions': [
{'action': 'dummy_pack_7.render', 'input': None}
]
}
workflow_service.request_task_execution(wf_ex_db, st2_ctx, task_ex_req)

# Check task execution is saved to the database.
task_ex_dbs = wf_db_access.TaskExecution.query(workflow_execution=str(wf_ex_db.id))
self.assertEqual(len(task_ex_dbs), 1)
workflow_service.request_task_execution(wf_ex_db, st2_ctx, task_ex_req)

# Manually request action execution
task_ex_db = task_ex_dbs[0]
action_ex_db = workflow_service.request_action_execution(wf_ex_db, task_ex_db, st2_ctx,
task_ex_req['actions'][0])

# Check required attributes.
self.assertIsNotNone(str(action_ex_db.id))
self.assertEqual(task_ex_db.workflow_execution, str(wf_ex_db.id))
expected_parameters = {'value1': output}
self.assertEqual(expected_parameters, action_ex_db.parameters)
3 changes: 2 additions & 1 deletion st2tests/st2tests/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,8 @@ def mock_st2_context(self, ac_ex_db, context=None):
st2_ctx = {
'st2': {
'api_url': api_util.get_full_public_api_url(),
'action_execution_id': str(ac_ex_db.id)
'action_execution_id': str(ac_ex_db.id),
'user': 'stanley'
}
}

Expand Down
2 changes: 2 additions & 0 deletions st2tests/st2tests/fixtures/packs/configs/dummy_pack_7.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
config_item_one: "testing"
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
name: render_config_context
pack: dummy_pack_20
description: Run render config context workflow
runner_type: orquesta
entry_point: workflows/render_config_context.yaml
enabled: true
parameters: {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
version: 1.0
description: Testing config context render".
tasks:
task1:
action: dummy_pack_7.render
output:
- context_value: <% task(task1).result.result.context_value %>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from st2common.runners.base_action import Action


class PrintPythonVersionAction(Action):

def run(self, value1):
return {"context_value": value1}
12 changes: 12 additions & 0 deletions st2tests/st2tests/fixtures/packs/dummy_pack_7/actions/render.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
name: render
runner_type: python-script
description: Action that uses config context
enabled: true
entry_point: render.py
parameters:
value1:
description: Input for action1. Defaults to config_context value.
required: false
type: "string"
default: "{{ config_context.config_item_one }}"
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
config_item_one:
description: "Item use to test config context."
type: "string"
required: true