config_context renders against incorrect pack #4570

jinpingh · 2019-02-28T18:46:52Z

Fixes #4567 config_context renders against incorrect pack
Root cause:
When a action calls other action which belongs to the different pack, during render_live_params, only parent pack configuration is retrieved.

Fixes:
Passes action reference to render_live_params, if action parent pack is different with child action pack, the configuration for child pack is retired and rendered.

Root cause: When a action calls other action which belongs to the different pack, during `render_live_params`, only parent pack configuration is retrieved. Fixes: Passes action reference to `render_live_params`, if action parent pack is different with child action pack, the configuration for child pack is retired and rendered.

m4dcoder

If the pack name being passed into the render_live_params is not correct, why not assign the correct pack at https://github.com/StackStorm/st2/blob/master/st2common/st2common/services/workflows.py#L546?

jinpingh · 2019-02-28T19:17:50Z

If the pack name being passed into the render_live_params is not correct, why not assign the correct pack at https://github.com/StackStorm/st2/blob/master/st2common/st2common/services/workflows.py#L546?

Running task is belong to the pack1 and it is passed. Action execution is belong to the different pack, e.g. pack2, in case both pack information needed on pack2 action.

jinpingh · 2019-02-28T19:30:07Z

If the pack name being passed into the render_live_params is not correct, why not assign the correct pack at https://github.com/StackStorm/st2/blob/master/st2common/st2common/services/workflows.py#L546?

I am thinking if pack2 action needs both pack1 and pack2 information, it is good to render both.

m4dcoder · 2019-02-28T19:34:25Z

What's the expected behavior from our current docs?

jinpingh · 2019-02-28T19:36:44Z

What's the expected behavior from our current docs?

Let me google. If it is not supported, will change code to pass right pack name

jinpingh · 2019-02-28T21:28:01Z

#4570 (comment)
Googed Stackstorm documentation and did not see anywhere mentioned that parent pack is supported in the child action execution.

Googed Stackstorm documentation and did not see anywhere mentioned that this is supported. Fix typo for `cancellation`

m4dcoder

Also, where are the unit and integration tests?

st2common/st2common/services/workflows.py

m4dcoder · 2019-02-28T21:41:49Z

st2common/st2common/services/workflows.py

    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')


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

You did not address this yet.

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.

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.

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

Yes, it is different. Changed my variable name.

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

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

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

st2common/st2common/services/workflows.py

m4dcoder · 2019-02-28T21:42:49Z

st2common/st2common/services/workflows.py

            'task_name': task_ex_db.task_name,
            'task_id': task_ex_db.task_id
-        }
+        },


Why do we need a comma here?

Did run nosetests --nocapture st2common/tests/unit/test_param_utils.py and all passed

m4dcoder · 2019-02-28T22:06:36Z

Did run nosetests --nocapture st2common/tests/unit/test_param_utils.py and all passed

I assume you are responding to my questions about unit/integration tests. Can you tell me what use case are we fixing here? Where is the unit test and integration test that cover this use case?

jinpingh · 2019-02-28T22:45:09Z

Did run nosetests --nocapture st2common/tests/unit/test_param_utils.py and all passed

I assume you are responding to my questions about unit/integration tests. Can you tell me what use case are we fixing here? Where is the unit test and integration test that cover this use case?

Tested test_param_utils.py and forgot param.py is not going to change. I am working on new testcase(s) in st2common/tests/unit/services/test_workflow.py. Please let me know if this is not right place.

m4dcoder · 2019-02-28T22:54:24Z

For the unit test, st2common/tests/unit/services/test_workflow.py looks like a good place to put the test.

For the integration test, I am also expecting a new test at https://github.com/StackStorm/st2/blob/master/st2tests/integration/orquesta/test_wiring.py. You will need st2 to be running on your dev system and then run make orquesta-itests to test.

m4dcoder · 2019-02-28T23:30:55Z

Also, please remember to add an entry in the CHANGELOG.

jinpingh · 2019-03-07T05:06:40Z

For the unit test, st2common/tests/unit/services/test_workflow.py looks like a good place to put the test.
Done for unit test

jinpingh · 2019-03-07T05:15:28Z

For the integration test, I am also expecting a new test at https://github.com/StackStorm/st2/blob/master/st2tests/integration/orquesta/test_wiring.py. You will need st2 to be running on your dev system and then run make orquesta-itests to test.

For integration test, from st2test framework, it setup base_path directory as st2/st2tests/st2tests/fixtures/packs/ and database name to st2-test.

Since orquesta integration test is calling st2client to do Workflow executions, so packs have to be installed before execution. Implemented code to installed two packs in the st2-test DB
After successfully installed two packs from st2/st2tests/st2tests/fixtures/packs/, observed that orquesta_runner is still looking for actions, workflow and virtuaenv from /opt/stackstorm/packs/

traceback:   File "/home/vagrant/st2/st2actions/st2actions/container/base.py", line 127, in _do_run
    (status, result, context) = runner.run(action_params)
  File "/home/vagrant/st2/contrib/runners/orquesta_runner/orquesta_runner/orquesta_runner.py", line 86, in run
    wf_def = self.get_workflow_definition(self.entry_point)
  File "/home/vagrant/st2/contrib/runners/orquesta_runner/orquesta_runner/orquesta_runner.py", line 50, in get_workflow_definition
    with open(entry_point, 'r') as def_file:

Two suggestions:

Manually install a pack (second pack's workflow/action can be merged to examples), then run test. This only need to add a small function to test_wiring.py
Implemented code:
a. Copy two packs to /opt/stackstorm directory
b. Install these two packs to st2 DB.
c. Run test case.

Any suggestions?

m4dcoder · 2019-03-07T05:26:39Z

I need to see the unit and integration tests in this PR to give you any real feedback. When this PR is merged, circle ci will run make orquesta-itests to validate. You can start by taking a look at https://github.com/StackStorm/st2/blob/master/.circleci/config.yml and follow the integration test to see how the environment is setup and what options there are to install this other pack.

# Conflicts: # st2common/st2common/services/workflows.py

Added new unit test case for the fix. Modified two dummy packs that unit test can run. Fix code review comments. Updated CHANGELOG.txt

m4dcoder · 2019-03-07T19:50:13Z

st2common/tests/unit/services/test_workflow.py

 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


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.

m4dcoder · 2019-03-07T19:50:24Z

st2common/st2common/services/workflows.py

    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')


You did not address this yet.

m4dcoder · 2019-03-07T19:51:02Z

st2common/tests/unit/services/test_workflow.py

+from st2common.models.db.pack import ConfigDB
 from st2common.persistence import execution as ex_db_access
 from st2common.persistence import workflow as wf_db_access
+from st2common.persistence.pack import Config


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

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.

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

m4dcoder · 2019-03-07T19:53:26Z

st2common/tests/unit/services/test_workflow.py

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


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?

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.

m4dcoder · 2019-03-07T21:40:10Z

st2common/st2common/services/workflows.py

    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)


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

Remove action and workflow from dummy_pack_20 to orquesta_tests Rename action_ref to spec_action Rename import names for test_workflow.py and reflect changes for dummy_pack_20

In order to run integration test, test used packs has to be installed first. Install `orquesta_tests` and `dummy_pack_7` packs in `` DB Create `virtualenv` for packs Add new testcase in test_wiring.py

jinpingh · 2019-03-08T16:57:10Z

I need to see the unit and integration tests in this PR to give you any real feedback. When this PR is merged, circle ci will run make orquesta-itests to validate. You can start by taking a look at https://github.com/StackStorm/st2/blob/master/.circleci/config.yml and follow the integration test to see how the environment is setup and what options there are to install this other pack.

From st2cd https://github.com/StackStorm/st2cd/blob/master/actions/setup_e2e_tests.sh, where examples pack is installed. We can add one more pack here:

sudo cp -R /usr/share/doc/st2/examples /opt/stackstorm/packs/
st2 run packs.setup_virtualenv packs=examples,tests,asserts,fixtures,webui
st2ctl reload --register-all

Will find out how file is located at /usr/share/doc/st2. Looking at https://github.com/StackStorm/st2/blob/master/.circleci/config.yml

m4dcoder · 2019-03-08T17:37:33Z

st2tests/integration/orquesta/test_wiring.py

        self.assertDictEqual(ex.result, expected_result)
+
+    def test_config_context_renders(self):
+        packs = ["orquesta_tests", "dummy_pack_7"]


These are packs for unit tests. We have separate ones for integration tests. I use the examples pack. But there are other packs that st2 CI uses. Looking at how the st2 environment is setup in CI and reusing existing test packs and installing those test packs on setup is simpler than writing an entire InstallFixturesPacks class here.

These are packs for unit tests. We have separate ones for integration tests. I use the examples pack. But there are other packs that st2 CI uses. Looking at how the st2 environment is setup in CI and reusing existing test packs and installing those test packs on setup is simpler than writing an entire InstallFixturesPacks class here.

Submitted code. Want to make sure I am in the right track. Did look at existing pack to replace dummy_pack_7 and still did not get the right one. I think we should create a new pack in the case integration test like this that needs too packs with complicated setup. Thanks!

m4dcoder · 2019-03-08T17:39:59Z

st2tests/st2tests/base.py

            self._register_pack_configs()


+class InstallFixturesPacks(DbTestCase):


Please look for a simpler alternative first thru the environment setup in circleci. May be simpler than writing a brand new class to do pack and venv setup here.

m4dcoder · 2019-03-11T20:55:49Z

st2tests/tests/actions/render.yaml

@@ -0,0 +1,12 @@
+---


The tests pack I'm asking you to modify is located @ https://github.com/StackStorm/st2tests/tree/master/packs/tests. It is a separate repo. You're adding files in the wrong place.

Sorry about this, did not realize there is a different repo for st2tests. On cicd for integration test, does tests pack installed on the same directory as https://github.com/StackStorm/st2/tree/master/st2tests/tests? Because it needs to be copied to /opt/stackstrom/packs, need to know the source location.

Since i don't understand how https://github.com/StackStorm/st2tests/tree/master/packs/tests is used, for earlier unit test submission, dummy_pack_7 is used for testing, should we change it to the same tests pack?

m4dcoder · 2019-03-11T20:55:56Z

st2tests/tests/actions/render.py

@@ -0,0 +1,7 @@
+from st2common.runners.base_action import Action


The tests pack I'm asking you to modify is located @ https://github.com/StackStorm/st2tests/tree/master/packs/tests. It is a separate repo. You're adding files in the wrong place.

m4dcoder · 2019-03-11T20:56:08Z

st2tests/tests/config.schema.yaml

@@ -0,0 +1,5 @@
+---


The tests pack I'm asking you to modify is located @ https://github.com/StackStorm/st2tests/tree/master/packs/tests. It is a separate repo. You're adding files in the wrong place.

m4dcoder

Thank you for making progress with this. There're still some more work and things to correct.

m4dcoder · 2019-03-12T17:51:03Z

@jinpingh I just want to let you know that you are doing great here. We are near the finish line for this PR. There are a number of things that is new here (registering pack besides examples pack for integration test) that even if I am the developer working on this, it would involve me doing exploring and trial and error. So I expected back and forth with this PR. The feedbacks on the review has more to do with helping you gain knowledge and get into the habit of writing unit tests and integration tests then anything else. Thank you for your diligence. So take a breather and continue the good work.

…pack Add new action to `tests` pack for render config context integration test. This is second part of code changes. Please see StackStorm/st2#4570

1. Move tests pack action to https://github.com/StackStorm/st2tests/tree/master/packs/tests Add a default value to config.schema.yaml and renamed action `render` to `render_config_context` Please see StackStorm/st2tests#150 2. Made change in CHANGELOG.rst 3. Rename copy_examples to copy_test_packs in `tools/launchdev.sh` 4. Handle gracefully when authentication is on for setting up virtualenv. 5. Renamed action `render` to `render_config_context` for unit test dummy_pack_7

jinpingh · 2019-03-12T21:49:55Z

tools/launchdev.sh

        cp -Rp ./contrib/examples $PACKS_BASE_DIR
-        cp -Rp ./st2tests/tests $PACKS_BASE_DIR
-        cp -p ./st2tests/tests/configs/tests.yaml $CONFIG_BASE_DIR
+        cp -Rp ./st2tests/packs/tests $PACKS_BASE_DIR


Please let me know if this is the right path ./st2tests/packs/tests

The tests pack is in a completely different repo https://github.com/StackStorm/st2tests/. You have to git clone https://github.com/StackStorm/st2tests.git to a temporary location and then copy the directory ./st2tests/packs/tests/ from the temporary location to /opt/stackstorm/packs.

m4dcoder · 2019-03-12T22:08:22Z

tools/launchdev.sh

+                    break
+                fi
+            fi
+        done < "$ST2_CONF"


Instead of this while loop, can we just run st2 run packs.setup_virtualenv packs=tests and if it returns ERROR: 401 Client Error: Unauthorized, you echo the warning?

I thought just run st2 run packs.setup_virtualenv packs=tests and if it returns ERROR: 401 Client Error: Unauthorized is easy way but not graceful way.

It's ok with me.

Are you going to change this?

Are you going to change this?

I am not going to change, but if this is too much, i can change it.

Yeah, I prefer simplicity here. If it returns an auth error, then warn users. This is a dev launch script, don't need to go thru the hassle to parse for the auth lines in the config file.

Kami · 2019-03-13T15:56:16Z

Just adding this info here for completeness. It looks like this is related to - #4531.

There I fixed the original issue, but I didn't correctly handle child workflow action which this PR fixes 👍

m4dcoder

Can you rename the commit for Clone https://github.com/StackStorm/st2tests.git since tests pack is … to Add step to clone st2tests repo and copy tests pack?

Clone st2tests repo to /tmp directory Copy st2tests/tests pack to /opt/stackstorm/packs Remove /tmp/st2tests directory

m4dcoder · 2019-03-13T17:44:13Z

@jinpingh We are almost done here. So after these minor changes, get everything green before asking for review again.

If `st2 run packs.setup_virtualenv packs=tests` returns an error, then warn users

m4dcoder · 2019-03-13T18:38:29Z

tools/launchdev.sh

+    if [ "$copy_test_packs" = true ]; then
+        st2 run packs.setup_virtualenv packs=tests
+        if [ $? != 0 ]; then
+            echo "Warning: Please setup virtualenv for pack \"tests\" before run integration test"


Please change the message to Unable to setup virtualenv for the \"tests\" pack. Please setup virtualenv for the \"tests\" pack before running integration tests.

Change the message to Unable to setup virtualenv for the \"tests\" pack. Please setup virtualenv for the \"tests\" pack before running integration tests.

…m/StackStorm/st2 into issue-4567/config_context_renders

Kami · 2019-03-13T18:51:23Z

Just a heads up - I didn't do extensive end to end testing (e.g. multiple levels of nested workflows and executions), but I tested it with example packs from https://github.com/bigmstone/packConfig and it appears to be working fine.

m4dcoder

@jinpingh Thank you for the hard work. I hope you learned a lot thru this experience.
Thanks for help and valuable inputs. Finally i can close this PR :) ✌

Kami · 2019-03-14T07:48:47Z

Since it got merged in time, I will also cherry pick this changes in a branch for v2.10.4 release 👍

jinpingh requested review from bigmstone and m4dcoder February 28, 2019 18:46

m4dcoder requested changes Feb 28, 2019

View reviewed changes

Remove render for parent pack configuration.

a97fd8e

Googed Stackstorm documentation and did not see anywhere mentioned that this is supported. Fix typo for `cancellation`

m4dcoder requested changes Feb 28, 2019

View reviewed changes

Kami added this to the 3.0.0 milestone Mar 3, 2019

jinpingh added 2 commits March 7, 2019 10:33

Merge branch 'master' into issue-4567/config_context_renders

5c64ca0

# Conflicts: # st2common/st2common/services/workflows.py

New unit test for config_context renders against incorrect pack

62a51fb

Added new unit test case for the fix. Modified two dummy packs that unit test can run. Fix code review comments. Updated CHANGELOG.txt

m4dcoder requested changes Mar 7, 2019

View reviewed changes

jinpingh closed this Mar 7, 2019

jinpingh reopened this Mar 7, 2019

m4dcoder reviewed Mar 7, 2019

View reviewed changes

jinpingh added 3 commits March 7, 2019 14:59

Fix review comments for unit test.

0e780ad

Remove action and workflow from dummy_pack_20 to orquesta_tests Rename action_ref to spec_action Rename import names for test_workflow.py and reflect changes for dummy_pack_20

Remove code for getting action reference from spc.action

0c41155

Integration test for config_context renders against incorrect pack

f76e6dd

In order to run integration test, test used packs has to be installed first. Install `orquesta_tests` and `dummy_pack_7` packs in `` DB Create `virtualenv` for packs Add new testcase in test_wiring.py

m4dcoder requested changes Mar 8, 2019

View reviewed changes

m4dcoder reviewed Mar 11, 2019

View reviewed changes

m4dcoder requested changes Mar 11, 2019

View reviewed changes

jinpingh mentioned this pull request Mar 12, 2019

config_context renders against incorrect pack StackStorm/st2tests#150

Merged

jinpingh commented Mar 12, 2019

View reviewed changes

m4dcoder reviewed Mar 12, 2019

View reviewed changes

m4dcoder requested changes Mar 13, 2019

View reviewed changes

Add step to clone st2tests repo and copy tests pack

eaa3858

Clone st2tests repo to /tmp directory Copy st2tests/tests pack to /opt/stackstorm/packs Remove /tmp/st2tests directory

Remove unused import: mock

c1f0724

jinpingh force-pushed the issue-4567/config_context_renders branch from f9d2f47 to c1f0724 Compare March 13, 2019 17:45

jinpingh and others added 2 commits March 13, 2019 11:30

Remove authentication setup checking for simplicity

82fcc34

If `st2 run packs.setup_virtualenv packs=tests` returns an error, then warn users

Merge branch 'master' into issue-4567/config_context_renders

3a8598e

m4dcoder requested changes Mar 13, 2019

View reviewed changes

jinpingh added 2 commits March 13, 2019 11:43

Change warning message.

c81e6d1

Change the message to Unable to setup virtualenv for the \"tests\" pack. Please setup virtualenv for the \"tests\" pack before running integration tests.

Merge branch 'issue-4567/config_context_renders' of https://github.co…

30bd1a0

…m/StackStorm/st2 into issue-4567/config_context_renders

m4dcoder approved these changes Mar 13, 2019

View reviewed changes

Merge branch 'master' into issue-4567/config_context_renders

2cbe564

jinpingh merged commit 7f6592d into master Mar 13, 2019

jinpingh deleted the issue-4567/config_context_renders branch March 13, 2019 20:56

Kami modified the milestones: 3.0.0, 2.10.4 Mar 14, 2019

Kami mentioned this pull request Mar 14, 2019

Changes for v2.10.4 release #4594

Merged


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

		self._register_pack_configs()


		class InstallFixturesPacks(DbTestCase):

		@@ -0,0 +1,7 @@
		from st2common.runners.base_action import Action

Uh oh!

config_context renders against incorrect pack #4570

config_context renders against incorrect pack #4570

Uh oh!

Conversation

jinpingh commented Feb 28, 2019

Uh oh!

m4dcoder left a comment

Choose a reason for hiding this comment

Uh oh!

jinpingh commented Feb 28, 2019

Uh oh!

jinpingh commented Feb 28, 2019

Uh oh!

m4dcoder commented Feb 28, 2019

Uh oh!

jinpingh commented Feb 28, 2019

Uh oh!

jinpingh commented Feb 28, 2019

Uh oh!

m4dcoder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4dcoder commented Feb 28, 2019

Uh oh!

jinpingh commented Feb 28, 2019

Uh oh!

m4dcoder commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m4dcoder commented Feb 28, 2019

Uh oh!

jinpingh commented Mar 7, 2019

Uh oh!

jinpingh commented Mar 7, 2019

Uh oh!

m4dcoder commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4dcoder Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

m4dcoder commented Feb 28, 2019 •

edited

Loading

m4dcoder Mar 7, 2019 •

edited

Loading

jinpingh commented Mar 8, 2019 •

edited

Loading