Merge pull request #4713 from StackStorm/various_misc_fixes

Various changes and fixes which make StackStorm code more re-usable in different contexts

Author: m4dcoder

CHANGELOG.rst

@@ -11,6 +11,10 @@ Changed
 * Update requests library to latest version (2.22.0) in requirements. (improvement) #4680
 * Disallow "decrypt_kv" filter to be specified in the config for values that are marked as
   "secret: True" in the schema. (improvement) #4709
+* Upgrade ``tooz`` library to latest stable version (1.65.0) so it uses latest version of
+  ``grpcio`` library. (improvement) #4713
+* Update ``st2-pack-install`` and ``st2-pack-download`` CLI command so it supports installing
+  packs from local directories which are not git repositories. (improvement) #4713
 
 Fixed
 ~~~~~
@@ -19,6 +23,14 @@ Fixed
 * Allow tasks defined in the same task transition with ``fail`` to run for orquesta. (bug fix)
 * Fix workflow service to handle unexpected coordinator and database errors. (bug fix) #4704 #4705
 * Fix filter ``to_yaml_string`` to handle mongoengine base types for dict and list. (bug fix) #4700
+* Fix timeout handling in the Python runner. In some scenarios where action would time out before
+  producing any output (stdout, stder), timeout was not correctly propagated to the user. (bug fix)
+  #4713
+* Update ``st2common/setup.py`` file so it correctly declares all the dependencies and script
+  files it provides. This way ``st2-pack-*`` commands can be used in a standalone fashion just by
+  installing ``st2common`` Python package and nothing else. (bug fix) #4713
+* Fix ``st2-pack-download`` command so it works in the environments where ``sudo`` binary is not
+  available (e.g. Docker). (bug fix) #4713
 
 3.0.1 - May 24, 2019
 --------------------

contrib/packs/tests/fixtures/stackstorm-test4/pack.yaml

@@ -0,0 +1,11 @@
+---
+name : test4
+description : st2 pack to test package management pipeline
+keywords:
+  - some
+  - search
+  - another
+  - terms
+version : "0.4.0"
+author : st2-dev
+email : [email protected]

contrib/packs/tests/test_action_download.py

@@ -35,6 +35,8 @@
 
 from pack_mgmt.download import DownloadGitRepoAction
 
+BASE_DIR = os.path.dirname(os.path.abspath(__file__))
+
 PACK_INDEX = {
     "test": {
         "version": "0.4.0",
@@ -67,6 +69,18 @@
 }
 
 
+original_is_dir_func = os.path.isdir
+
+
+def mock_is_dir_func(path):
+    """
+    Mock function which returns True if path ends with .git
+    """
+    if path.endswith('.git'):
+        return True
+    return original_is_dir_func(path)
+
+
 @mock.patch.object(pack_service, 'fetch_pack_index', mock.MagicMock(return_value=(PACK_INDEX, {})))
 class DownloadGitRepoActionTestCase(BaseActionTestCase):
     action_cls = DownloadGitRepoAction
@@ -79,8 +93,9 @@ def setUp(self):
         self.addCleanup(clone_from.stop)
         self.clone_from = clone_from.start()
 
+        self.expand_user_path = tempfile.mkdtemp()
         expand_user = mock.patch.object(os.path, 'expanduser',
-                                        mock.MagicMock(return_value=tempfile.mkdtemp()))
+                                        mock.MagicMock(return_value=self.expand_user_path))
 
         self.addCleanup(expand_user.stop)
         self.expand_user = expand_user.start()
@@ -522,16 +537,38 @@ def fake_commit(arg_ref):
             result = action.run(packs=packs, abs_repo_base=self.repo_base)
             self.assertEqual(result, {'test': 'Success.'})
 
+    @mock.patch('os.path.isdir', mock_is_dir_func)
     def test_run_pack_dowload_local_git_repo_detached_head_state(self):
         action = self.get_action_instance()
 
         type(self.repo_instance).active_branch = \
             mock.PropertyMock(side_effect=TypeError('detached head'))
 
-        result = action.run(packs=['file:///stackstorm-test'], abs_repo_base=self.repo_base)
+        pack_path = os.path.join(BASE_DIR, 'fixtures/stackstorm-test')
+
+        result = action.run(packs=['file://%s' % (pack_path)], abs_repo_base=self.repo_base)
         self.assertEqual(result, {'test': 'Success.'})
 
         # Verify function has bailed out early
         self.repo_instance.git.checkout.assert_not_called()
         self.repo_instance.git.branch.assert_not_called()
         self.repo_instance.git.checkout.assert_not_called()
+
+    def test_run_pack_download_local_directory(self):
+        action = self.get_action_instance()
+
+        # 1. Local directory doesn't exist
+        expected_msg = r'Local pack directory ".*" doesn\'t exist'
+        self.assertRaisesRegexp(ValueError, expected_msg, action.run,
+                                packs=['file://doesnt_exist'], abs_repo_base=self.repo_base)
+
+        # 2. Local pack which is not a git repository
+        pack_path = os.path.join(BASE_DIR, 'fixtures/stackstorm-test4')
+
+        result = action.run(packs=['file://%s' % (pack_path)], abs_repo_base=self.repo_base)
+        self.assertEqual(result, {'test4': 'Success.'})
+
+        # Verify pack contents have been copied over
+        destination_path = os.path.join(self.repo_base, 'test4')
+        self.assertTrue(os.path.exists(destination_path))
+        self.assertTrue(os.path.exists(os.path.join(destination_path, 'pack.yaml')))

contrib/runners/local_runner/local_runner/base.py

@@ -22,7 +22,6 @@
 
 import six
 from oslo_config import cfg
-from eventlet.green import subprocess
 from six.moves import StringIO
 
 from st2common.constants import action as action_constants
@@ -35,6 +34,7 @@
 from st2common.util.green import shell
 from st2common.util.shell import kill_process
 from st2common.util import jsonify
+from st2common.util import concurrency
 from st2common.services.action import store_execution_output_data
 from st2common.runners.utils import make_read_and_store_stream_func
 
@@ -138,13 +138,15 @@ def _run(self, action):
         read_and_store_stderr = make_read_and_store_stream_func(execution_db=self.execution,
             action_db=self.action, store_data_func=store_execution_stderr_line)
 
+        subprocess = concurrency.get_subprocess_module()
+
         # If sudo password is provided, pass it to the subprocess via stdin>
         # Note: We don't need to explicitly escape the argument because we pass command as a list
         # to subprocess.Popen and all the arguments are escaped by the function.
         if self._sudo_password:
             LOG.debug('Supplying sudo password via stdin')
-            echo_process = subprocess.Popen(['echo', self._sudo_password + '\n'],
-                                            stdout=subprocess.PIPE)
+            echo_process = concurrency.subprocess_popen(['echo', self._sudo_password + '\n'],
+                                                        stdout=subprocess.PIPE)
             stdin = echo_process.stdout
         else:
             stdin = None

contrib/runners/local_runner/tests/integration/test_localrunner.py

@@ -152,8 +152,8 @@ def test_sudo_and_env_variable_preservation(self):
         self.assertEquals(status, action_constants.LIVEACTION_STATUS_SUCCEEDED)
         self.assertEqual(result['stdout'].strip(), 'root\nponiesponies')
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
-    @mock.patch('st2common.util.green.shell.eventlet.spawn')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
+    @mock.patch('st2common.util.concurrency.spawn')
     def test_action_stdout_and_stderr_is_stored_in_the_db(self, mock_spawn, mock_popen):
         # Feature is enabled
         cfg.CONF.set_override(name='stream_output', group='actionrunner', override=True)
@@ -210,8 +210,8 @@ def test_action_stdout_and_stderr_is_stored_in_the_db(self, mock_spawn, mock_pop
         self.assertEqual(output_dbs[1].data, mock_stderr[1])
         self.assertEqual(output_dbs[2].data, mock_stderr[2])
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
-    @mock.patch('st2common.util.green.shell.eventlet.spawn')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
+    @mock.patch('st2common.util.concurrency.spawn')
     def test_action_stdout_and_stderr_is_stored_in_the_db_short_running_action(self, mock_spawn,
                                                                                mock_popen):
         # Verify that we correctly retrieve all the output and wait for stdout and stderr reading
@@ -331,7 +331,7 @@ def test_shell_command_sudo_password_is_passed_to_sudo_binary(self):
             self.assertEquals(result['stdout'], sudo_password)
 
         # Verify new process which provides password via stdin to the command is created
-        with mock.patch('eventlet.green.subprocess.Popen') as mock_subproc_popen:
+        with mock.patch('st2common.util.concurrency.subprocess_popen') as mock_subproc_popen:
             index = 0
             for sudo_password in sudo_passwords:
                 runner = self._get_runner(action_db, cmd=cmd)
@@ -510,8 +510,8 @@ def test_script_with_parameters_parameter_serialization(self):
         output_dbs = ActionExecutionOutput.query(output_type='stderr')
         self.assertEqual(len(output_dbs), 0)
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
-    @mock.patch('st2common.util.green.shell.eventlet.spawn')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
+    @mock.patch('st2common.util.concurrency.spawn')
     def test_action_stdout_and_stderr_is_stored_in_the_db(self, mock_spawn, mock_popen):
         # Feature is enabled
         cfg.CONF.set_override(name='stream_output', group='actionrunner', override=True)

contrib/runners/python_runner/python_runner/python_runner.py

@@ -318,15 +318,17 @@ def _get_output_values(self, exit_code, stdout, stderr, timed_out):
             action_result = split[1].strip()
             stdout = split[0] + split[2]
         else:
+            # Timeout or similar
             action_result = None
 
-        # Parse the serialized action result object
-        try:
-            action_result = json.loads(action_result)
-        except Exception as e:
-            # Failed to de-serialize the result, probably it contains non-simple type or similar
-            LOG.warning('Failed to de-serialize result "%s": %s' % (str(action_result),
-                                                                    six.text_type(e)))
+        # Parse the serialized action result object (if available)
+        if action_result:
+            try:
+                action_result = json.loads(action_result)
+            except Exception as e:
+                # Failed to de-serialize the result, probably it contains non-simple type or similar
+                LOG.warning('Failed to de-serialize result "%s": %s' % (str(action_result),
+                                                                        six.text_type(e)))
 
         if action_result:
             if isinstance(action_result, dict):

contrib/runners/python_runner/tests/unit/test_pythonrunner.py

@@ -251,7 +251,7 @@ def test_simple_action_no_entry_point(self):
         expected_msg = 'Action .*? is missing entry_point attribute'
         self.assertRaisesRegexp(Exception, expected_msg, runner.run, {})
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
     def test_action_with_user_supplied_env_vars(self, mock_popen):
         env_vars = {'key1': 'val1', 'key2': 'val2', 'PYTHONPATH': 'foobar'}
 
@@ -275,8 +275,8 @@ def test_action_with_user_supplied_env_vars(self, mock_popen):
             else:
                 self.assertEqual(actual_env[key], value)
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
-    @mock.patch('st2common.util.green.shell.eventlet.spawn')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
+    @mock.patch('st2common.util.concurrency.spawn')
     def test_action_stdout_and_stderr_is_not_stored_in_db_by_default(self, mock_spawn, mock_popen):
         # Feature should be disabled by default
         values = {'delimiter': ACTION_OUTPUT_RESULT_DELIMITER}
@@ -344,8 +344,8 @@ def test_action_stdout_and_stderr_is_not_stored_in_db_by_default(self, mock_spaw
         output_dbs = ActionExecutionOutput.get_all()
         self.assertEqual(len(output_dbs), 0)
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
-    @mock.patch('st2common.util.green.shell.eventlet.spawn')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
+    @mock.patch('st2common.util.concurrency.spawn')
     def test_action_stdout_and_stderr_is_stored_in_the_db(self, mock_spawn, mock_popen):
         # Feature is enabled
         cfg.CONF.set_override(name='stream_output', group='actionrunner', override=True)
@@ -406,7 +406,7 @@ def test_action_stdout_and_stderr_is_stored_in_the_db(self, mock_spawn, mock_pop
         self.assertEqual(output_dbs[1].data, mock_stderr[1])
         self.assertEqual(output_dbs[2].data, mock_stderr[2])
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
     def test_stdout_interception_and_parsing(self, mock_popen):
         values = {'delimiter': ACTION_OUTPUT_RESULT_DELIMITER}
 
@@ -478,7 +478,7 @@ def test_stdout_interception_and_parsing(self, mock_popen):
         self.assertEqual(output['exit_code'], 0)
         self.assertEqual(status, 'succeeded')
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
     def test_common_st2_env_vars_are_available_to_the_action(self, mock_popen):
         mock_process = mock.Mock()
         mock_process.communicate.return_value = ('', '')
@@ -495,7 +495,7 @@ def test_common_st2_env_vars_are_available_to_the_action(self, mock_popen):
         actual_env = call_kwargs['env']
         self.assertCommonSt2EnvVarsAvailableInEnv(env=actual_env)
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
     def test_pythonpath_env_var_contains_common_libs_config_enabled(self, mock_popen):
         mock_process = mock.Mock()
         mock_process.communicate.return_value = ('', '')
@@ -515,7 +515,7 @@ def test_pythonpath_env_var_contains_common_libs_config_enabled(self, mock_popen
         self.assertTrue('PYTHONPATH' in actual_env)
         self.assertTrue(pack_common_lib_path in actual_env['PYTHONPATH'])
 
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
     def test_pythonpath_env_var_not_contains_common_libs_config_disabled(self, mock_popen):
         mock_process = mock.Mock()
         mock_process.communicate.return_value = ('', '')
@@ -806,7 +806,7 @@ def test_content_version_success(self, mock_get_sandbox_virtualenv_path):
         self.assertRaisesRegexp(ValueError, expected_msg, runner.pre_run)
 
     @mock.patch('python_runner.python_runner.get_sandbox_virtualenv_path')
-    @mock.patch('st2common.util.green.shell.subprocess.Popen')
+    @mock.patch('st2common.util.concurrency.subprocess_popen')
     def test_content_version_contains_common_libs_config_enabled(self, mock_popen,
                                                                  mock_get_sandbox_virtualenv_path):
         # Verify that the common libs path correctly reflects directory in git worktree

fixed-requirements.txt

@@ -39,7 +39,7 @@ virtualenv==16.6.0
 sseclient-py==1.7
 python-editor==1.0.4
 prompt-toolkit==1.0.15
-tooz==1.64.2
+tooz==1.65.0
 zake==0.2.2
 routes==2.4.1
 webob==1.8.4

requirements.txt

@@ -52,7 +52,7 @@ semver==2.8.1
 six==1.12.0
 sseclient-py==1.7
 stevedore==1.30.1
-tooz==1.64.2
+tooz==1.65.0
 ujson==1.35
 unittest2
 webob==1.8.4

st2common/in-requirements.txt

@@ -33,3 +33,6 @@ ujson
 # Note: amqp is used by kombu, this needs to be added here to be picked up by
 # requirements fixate script.
 amqp
+# Used by st2-pack-* commands
+gitpython
+lockfile