Common pack lib for sensors and actions #3658

lakshmi-kannan · 2017-08-08T16:40:13Z

closes: #3490, closes: #2220

Example

Common lib code in /opt/stackstorm/packs/examples/lib/

(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯ cat /opt/stackstorm/packs/examples/lib/base.py                 issues_3490/pack_common_code ✭ ✱ ◼
import os

def get_environ(env_var):
    return os.environ.get(env_var, None)

Using the common lib code in an action

(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯ cat /opt/stackstorm/packs/examples/actions/pythonactions/isprime.py
import math
import os

from base import get_environ
from st2common.runners.base_action import Action


class PrimeCheckerAction(Action):
    def run(self, value=0):
        self.logger.info(get_environ('PYTHONPATH'))
        self.logger.debug('value=%s' % (value))
        if math.floor(value) != value:
            raise ValueError('%s should be an integer.' % value)
        if value < 2:
            return False
        for test in range(2, int(math.floor(math.sqrt(value)))+1):
            if value % test == 0:
                return False
        return True

if __name__ == '__main__':
    checker = PrimeCheckerAction()
    for i in range(0, 10):
        print '%s : %s' % (i, checker.run(value=1))
(virtualenv)vagrant@st2dev /m/s/s/st2 ❯❯❯

TODO

Examples
Docs Sharing code between sensors and actions st2docs#576
Unit tests No unit tests possible.
Integration tests in st2tests Add integration tests for python sensors and actions st2tests#111
Changelog

Feedback needed

Import style

As you can see in the action consuming the lib, the import line looks so

from base import get_environ

I want all the feature requesters like @andrew-regan @ytjohn @nmaludy to weigh in on if this works for them or do they need something like

from lib.base import get_environ

Do you want to allow lib code to contain dependencies from pack requirements?

I think this should work the way we have done sandboxing but I want to know if this is something you really want it to work?

Anything else?

arm4b · 2017-08-08T18:28:00Z

st2common/st2common/util/pack.py

+    and actions are placed.
+
+    For example, if the pack is at /opt/stackstorm/packs/my_pack, you can place
+    common library code for actions and sensors in /opt/stackstorm/packs/my_pack/lib/.


nmaludy · 2017-08-08T23:18:40Z

@lakshmi-kannan Great work!

Import Style

I prefer lib.base to base because it seems more "explicit" to me. However, i'm not sure how this will play out for existing packs that have python files in actions/lib/ and sensors/lib/?

With this import style, will python actions be able to reference this directory like so : entry_point: ../lib/operation.py?

Dependencies in requirements

Yes, i plan on having common code reference dependencies defined in the pack's requirements.txt.
My use case for this lib/ directory are "utility" functions that aggregate several common API call chains or transform input/output datatypes in the underlying integration library.

Thank you very much for this feature!

Kami · 2017-08-09T07:48:51Z

I would also prefer lib.base over base, but this might be harder to achieve because this means we would need to add <pack path>/actions/ to PYTHONPATH and make sure actions/ directory is also a package (contains __init__.py).

Kami · 2017-08-09T07:49:42Z

contrib/runners/python_runner/python_runner.py

+                                                      inherit_parent_virtualenv=True)
+        pack_common_libs_path = get_pack_common_libs_path(pack_db=pack_db)
+
+        env['PYTHONPATH'] = pack_common_libs_path + ':' + sandbox_python_path


Just thinking out loud - is adding it to the front of the python path the right thing to do? :)

Kami · 2017-08-09T07:50:02Z

st2common/st2common/bootstrap/base.py

@@ -174,6 +174,7 @@ def _register_pack_db(self, pack_name, pack_dir):
        # Include a list of pack files
        pack_file_list = get_file_list(directory=pack_dir, exclude_patterns=EXCLUDE_FILE_PATTERNS)
        content['files'] = pack_file_list
+        content['pack'] = pack_dir


Should probably be pack -> path :D

Yeah, I've the fix locally. Haven't pushed yet.

Kami · 2017-08-09T07:51:06Z

st2common/st2common/util/pack.py

+
+    :rtype: ``str``
+    """
+    pack_dir = getattr(pack_db, 'path', None)


I guess having this path here static is fine, although in other places we dynamically resolve entry point, etc.

lakshmi-kannan · 2017-08-10T00:12:18Z

I prefer lib.base to base because it seems more "explicit" to me. However, i'm not sure how this will play out for existing packs that have python files in actions/lib/ and sensors/lib/

@nmaludy That's why supporting it would be a pain. Plus, for remote SSH actions, we copy over actions/lib. I don't want people to get confused with that lib with this lib dir :). I'll document this but still this is going to be a source of confusions. Therefore, I am going to not support lib.base but allow modules in ${pack_dir}/lib/ to be imported by name like import foo (and not import lib.foo)

With this import style, will python actions be able to reference this directory like so : entry_point: ../lib/operation.py?

This would be strongly discouraged although this might work due to our implementation. "lib" folder is supposed to contain only common code that is then used in pythonic sensors and actions. Lib cannot contain actual st2 content.

I am debating if I should rename lib to something like "common" to avoid confusions down the road.

nmaludy · 2017-08-10T00:23:59Z

@lakshmi-kannan makes total sense as to the supportability and backwards compatibility of existing code.

Although I prefer the name lib/, I like the idea of renaming it to common/ to avoid confusion.

When you say "lib cannot contain actual st2 content", does this mean it cannot contain action and sensor code? Will the code in lib/ still be able to access st2 functions like the pack's config and the key/value store?

One final question, will lib/ require an init.py file to work?

lakshmi-kannan · 2017-08-10T00:33:49Z

When you say "lib cannot contain actual st2 content", does this mean it cannot contain action and sensor code? Will the code in lib/ still be able to access st2 functions like the pack's config and the key/value store?

Code in lib/ cannot call functions in action or sensor. This would be poor code design IMO. Code in lib/ can be called by both sensor and action. That's why it's called "lib". I assume data mangling functions, common client library code not pushed to pypi etc, common utility functions will go in files in lib. We will not support calling sensor or action code in lib. We can never do that with the way we isolate the sanboxed env for actions and sensors.

Will the code in lib/ still be able to access st2 functions like the pack's config and the key/value store?

I can see this can be useful down the road but the initial pass will not address this. This is a complete feature by itself and I won't take it on in my first pass here.

One final question, will lib/ require an init.py file to work?

Yes, I answered this already somewhere but yes, lib should a python package. Otherwise, imports won't work.

* master: Remove debugging 'docker ps' Add clarifying comments about the Docker cleanup Remove any created Docker volumes before and after build

arm4b · 2017-08-18T22:21:13Z

@Kami @lakshmi-kannan are we going to include this change as part of 2.4 or 2.5?

Kami · 2017-09-13T19:46:21Z

@lakshmi-kannan I'm fine if we can document it.

And ideally, if we can also make it fail loudly in scenarios like that (but that might not be possible, at least easily - would need to dig in to say for sure though).

lakshmi-kannan · 2017-09-13T19:49:18Z

@Kami Here is an example of how it breaks

result_task: test_stdout_python_action
result:
  exit_code: 1
  result: None
  stderr: "Traceback (most recent call last):\n  File \"/opt/stackstorm/virtualenvs/fixtures/lib/python2.7/site.py\", line 703, in <module>\n    main()\n  File \"/opt/stackstorm/virtualenvs/fixtures/lib/python2.7/site.py\", line 670, in main\n    virtual_install_main_packages()\n  File \"/opt/stackstorm/virtualenvs/fixtures/lib/python2.7/site.py\", line 553, in virtual_install_main_packages\n    f = open(os.path.join(os.path.dirname(__file__), 'orig-prefix.txt'))\nAttributeError: 'module' object has no attribute 'path'\n"
  stdout: ''
error: Traceback (most recent call last):
  File "/opt/stackstorm/virtualenvs/fixtures/lib/python2.7/site.py", line 703, in <module>
    main()
  File "/opt/stackstorm/virtualenvs/fixtures/lib/python2.7/site.py", line 670, in main
    virtual_install_main_packages()
  File "/opt/stackstorm/virtualenvs/fixtures/lib/python2.7/site.py", line 553, in virtual_install_main_packages
    f = open(os.path.join(os.path.dirname(__file__), 'orig-prefix.txt'))
AttributeError: 'module' object has no attribute 'path'

If we want to do runtime check, this is going to be hard. Maybe we can add this to the linter to stop people from doing this?

Kami · 2017-09-14T14:30:12Z

I think having a linter or similar would be hard, because we would probably manually keep a list of the "reserved" / system module names.

So I'm not aware of any easy solution besides namespacing / prefixing user modules with lib. or similar.

The more problematic thing is that it's not just stdlib package / module names, but any package / module which is available in the virtualenv which could make it quite annoying and action developers would need to get quite creative with the names.

lakshmi-kannan · 2017-09-14T16:49:15Z

@Kami and I discussed this in slack. In conclusion, a lot of packs already have a 'lib' folder and do imports like from lib.action import foo. By merging this PR, we will break things I think. It's better to not merge this until we can confirm we won't break existing packs that use "lib".

Kami · 2017-09-14T17:28:54Z

Yeah, sadly this feature is a lot more complicated then we would hope for - if we continued with the current approach it has a potential to break existing code (potentially in obscure manner which would cause unnecessary confusion and bad UX).

Only good option we can think of is namespacing the imports - e.g. something like from <pack name>.lib.<module name> import foo and / or from <pack name>/actions/lib/<module name> import foo.

The problem with the name spacing is that they are two ways to achieve it:

Some PATH hackery so it only works inside st2 even if pack directory is not a package
Making sure pack root directory and <pack root>/lib/ directories are packages (contain __init__.py file)

I'm strongly against the first approach. The code should also work if user runs it outside StackStorm environment (possible to re-use it, easier to develop and troubleshoot, etc.).

Second approach would mean it only works with new code and packs which are also Python packages. I'm fine with that, the problem is that now package root would also need to contain __init__py and I'm not a big fan of that - packs can contain actions for different runners and this would make it somewhat biased against the Python runner.

lakshmi-kannan · 2017-09-21T15:31:45Z

@Kami I created this pack to test our theory that these changes might break if actions and sensors have "lib" folder inside them. I was thinking it shouldn't and it looks like it doesn't. Here is a sample pack https://github.com/lakshmi-kannan/lib_test with a pack level lib folder and a lib folder inside sensors/ and actions/. Both actions and sensors import from pack lib and lib folders inside actions/ and sensors/ respectively. The sensor runs fine and also the actions. I verified that there are no import issues. I'll scan exchange and see if there are any other type of imports I need to test. Also, I'll port these tests to st2tests. Summary: Looks like there is still hope!

Kami · 2017-10-20T17:53:47Z

conf/st2.dev.conf

@@ -106,3 +106,6 @@ pack_group = stanley
 [mistral]
 v2_base_url = http://127.0.0.1:8989/v2
 jitter_interval = 0
+
+[packs]
+enable_common_libs = True


We have [system] where we have other packs related options there so there are some inconsistencies.

Anyway, I'm fine with this for now.

Kami · 2017-10-20T17:54:01Z

conf/st2.conf.sample

@@ -292,3 +292,6 @@ local_timezone = America/Los_Angeles
 # Base https URL to access st2 Web UI. This is used to constructhistory URLs that are sent out when chatops is used to kick off executions.
 webui_base_url = https://localhost

+[packs]
+# Enable/Disable support for pack common libs


Needs some clarification and link to the docs.

* master: (68 commits) Re-generate requirements.txt file. Add missing empty requirements.txt files. Update runner stevedore metadata and use actual runner name from runner.yaml for stevedore extension / driver name instead of using the Python module name. Add a changelog entry. Add changelog entry. Upgrade various Python deps to latest stable versions. Move import to reduce import time. Fix performance regression and move imports in-line to avoid import overhead (400+ms). Add functions for retrieving a list of dynamically registered runners. Add BACKENDS_NAMESPACE constant so it's consistent with auth backends. Add missing requirements.txt to action chain runner. Add "get_metadata()" function to each runner module, make sure we include runner.yaml as part of module data, add missing __all__ definitions. Add stevedore entrypoint metadata to each runner Python package setup.py file. Update changelog. Add changelog entry for #3802. Add changelog entry for #3810. Add a docstring. Mock action has no _log_level property, make code more robust. Fix lint. Add a changelog entry. ...

Kami · 2017-11-08T14:46:51Z

contrib/runners/python_runner/python_runner.py

@@ -105,7 +109,8 @@ def pre_run(self):
    def run(self, action_parameters):
        LOG.debug('Running pythonrunner.')
        LOG.debug('Getting pack name.')
-        pack = self.get_pack_name()
+        pack = self.get_pack_ref()
+        pack_db = Pack.get_by_ref(pack)


I guess there could be some races now - e.g. when pack is being removed and the model is removed first, but then again, we can't really guarantee proper behavior during pack removal process anyway.

Kami · 2017-11-08T14:52:27Z

contrib/runners/python_runner/python_runner.py

+        pack_common_libs_path = get_pack_common_libs_path(pack_db=pack_db)
+
+        if self._enable_common_pack_libs and pack_common_libs_path:
+            env['PYTHONPATH'] = pack_common_libs_path + ':' + sandbox_python_path


Can you please also add some unit tests which assert on the actual PYTHONPATH for various scenarios (libs path is enabled, disabled, etc.)?

If they are already there, please ignore my comment, but I didn't see any. I only see st2tests test, but it only seems to be testing one code path (common libs enabled). Should be straight forward to add a unit test - Python runner action can just print PYTHONPATH and we can assert on that.

Fixed in ce6fb89

* master: Use Python's built-in '==' instead of rolling our own dictionary equality check function Add changelog entry for #3833. Add a unit test which verifies that runner container correctly scopes temporary auth token to the user which ran the action. Add tests for filtering with the search operator Add tests for new payload module Correct a few failure messages in test assertions Add tests for operators where criteria_pattern is None Add tests for get_operator and get_allowed_operators Add unit tests for search operator Add a 'search' operator to the rules engine, and move PayloadLookup into its own module: st2common.util.payload

* master: (33 commits) Fix "liveactions" vs "executions" manager naming mismatch & add example Add return, remove now obsolete tests. Update st2 whoami CLI command to also include auth token expire time. Update /api/v1/user API endpoint to also include information about token expiration time in case user authenticated via auth token. Use numeric constants. Update pack.yaml Minor edit to changelog entry Add "datastore_service" property to sensor service so it has a consistent api with the action service. Fix lint. Update 'st2 whoami' CLI command to utilize new /api/v1/user API endpoint. Update CHANGELOG.rst Add clarifying comment. Move sleep to the right place. Add sleep to avoid race. Add changelog entry. Add tests for the mock service methods. Add method wrappers on the action and sensor service. Also add get_user_info method to the corresponding mock class. Add new get_user_info method to datastore service. Change controller and API endpoint path from whoami to user. ...

* master: (25 commits) Add script for listing available / installed action runners. Add period at the end of sentence in CHANGELOG Update copyright date. To avoid package naming conflicts, rename inquirery runner module and package from just "inquirer" to "inquirer". Add "stackstorm-runner-" prefix to all the Package names for action runners. Update changelog entry. Update more of the affected tests to use new, non-deprecated paths. Fix some of the affected tests. Add make target for running just the runners tests. Update affected runner tests. Update affected tests. Update __all__. Update runner registration code so it also supports new runner module locations. Throw a more user friendly exception. Make sure we correctly include runner.yaml file in the final package. Make all action runners proper Python packages. Soft-retrieve schema, and check to ensure it's not empty before masking Force key lookup for schema in mask_secrets Ensure executions API properly masks inquiry responses Use MASKED_ATTRIBUTE_VALUE in lieu of string literals ...

Python and other runners were refactored to be fully standalone and not require db connection, but #3658 PR inadvertently introduced DB dependency. This PR works around that by only retrieving PackDB object from the database if common libs functionality is enabled and ignore database related errors. Note: Common lib functionality is a new feature and is not needed on AWS Lambada.

Initial checkpoint

38003f9

arm4b reviewed Aug 8, 2017

View reviewed changes

arm4b mentioned this pull request Aug 8, 2017

Feature Request: Standard mechanism to share python code within a pack #2220

Closed

Kami assigned Kami and unassigned Kami Aug 8, 2017

Kami self-requested a review August 8, 2017 21:55

Kami reviewed Aug 9, 2017

View reviewed changes

Fix the action runner to work with libs

afd1c65

Lakshmi Kannan added 2 commits August 10, 2017 10:13

Add a sample sensor to test common lib for sensor

7749edd

Edit sample sensor to use common lib

636d5a6

lakshmi-kannan mentioned this pull request Aug 10, 2017

Sensors in "Examples" pack should do something meaningful #3663

Closed

Lint fixes

eb184bf

lakshmi-kannan mentioned this pull request Aug 11, 2017

Add integration tests for python sensors and actions StackStorm/st2tests#111

Merged

s/base.py/environ.py in examples pack

2e13fa8

lakshmi-kannan mentioned this pull request Aug 11, 2017

Sharing code between sensors and actions StackStorm/st2docs#576

Merged

Lakshmi Kannan added 5 commits August 14, 2017 07:23

Add a logging sample

73433e4

Add unit tests for pack db models

3e75a66

Add tests for get_pack_common_libs_path

208aa2a

Merge branch 'master' into issues_3490/pack_common_code

71e863e

* master: Remove debugging 'docker ps' Add clarifying comments about the Docker cleanup Remove any created Docker volumes before and after build

Update CHANGELOG

6bc0fcc

lakshmi-kannan changed the title ~~WIP: Common pack lib for sensors and actions~~ Common pack lib for sensors and actions Aug 14, 2017

lakshmi-kannan added the RFR label Aug 14, 2017

Kami approved these changes Sep 14, 2017

View reviewed changes

Expose config to enable/disable common libs support

296ad61

Kami reviewed Oct 20, 2017

View reviewed changes

Lakshmi Kannan added 2 commits October 24, 2017 07:50

Explanation for enable_common_libs config setting

af7bec3

Update CHANGELOG

210ae86

humblearner modified the milestones: 2.5.0, 2.6.0 Oct 24, 2017

Lakshmi Kannan added 3 commits November 7, 2017 08:05

Use make configgen to generate st2.conf.sample

8889072

Fix lint (remove blank line)

e829a93

Kami reviewed Nov 8, 2017

View reviewed changes

Lakshmi Kannan added 5 commits November 8, 2017 12:04

Add unit tests to validate lib path

ce6fb89

Validation path is different in travis

5b88cff

lakshmi-kannan merged commit dcdf4c3 into master Nov 15, 2017

cognifloyd mentioned this pull request Nov 20, 2017

Feature Request: Add mutable abs_repo_base to packs actions #3724

Open

Kami mentioned this pull request Dec 6, 2017

Remove MongoDB database dependency from Python runner #3900

Merged

arm4b deleted the issues_3490/pack_common_code branch May 15, 2018 17:26

cognifloyd removed the RFR label Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common pack lib for sensors and actions #3658

Common pack lib for sensors and actions #3658

lakshmi-kannan commented Aug 8, 2017 •

edited

Loading

arm4b Aug 8, 2017

andrew-regan Aug 8, 2017

nmaludy commented Aug 8, 2017

Kami commented Aug 9, 2017

Kami Aug 9, 2017

Kami Aug 9, 2017

lakshmi-kannan Aug 9, 2017

Kami Aug 9, 2017

lakshmi-kannan commented Aug 10, 2017 •

edited

Loading

nmaludy commented Aug 10, 2017

lakshmi-kannan commented Aug 10, 2017

arm4b commented Aug 18, 2017

Kami commented Sep 13, 2017

lakshmi-kannan commented Sep 13, 2017

Kami commented Sep 14, 2017

lakshmi-kannan commented Sep 14, 2017

Kami commented Sep 14, 2017

lakshmi-kannan commented Sep 21, 2017

Kami Oct 20, 2017

Kami Oct 20, 2017

Kami Nov 8, 2017

Kami Nov 8, 2017

lakshmi-kannan Nov 8, 2017

Common pack lib for sensors and actions #3658

Common pack lib for sensors and actions #3658

Conversation

lakshmi-kannan commented Aug 8, 2017 • edited Loading

Example

Common lib code in /opt/stackstorm/packs/examples/lib/

Using the common lib code in an action

TODO

Feedback needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmaludy commented Aug 8, 2017

Import Style

Dependencies in requirements

Kami commented Aug 9, 2017

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

lakshmi-kannan commented Aug 10, 2017 • edited Loading

nmaludy commented Aug 10, 2017

lakshmi-kannan commented Aug 10, 2017

arm4b commented Aug 18, 2017

Kami commented Sep 13, 2017

lakshmi-kannan commented Sep 13, 2017

Kami commented Sep 14, 2017

lakshmi-kannan commented Sep 14, 2017

Kami commented Sep 14, 2017

lakshmi-kannan commented Sep 21, 2017

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

lakshmi-kannan commented Aug 8, 2017 •

edited

Loading

lakshmi-kannan commented Aug 10, 2017 •

edited

Loading