Improve error handling in for ssh_raw/rsync in ansible/puppet/bash

solar/core/handlers/ansible_template.py

@@ -19,7 +19,6 @@ import os
 from solar.core.handlers.base import SOLAR_TEMP_LOCAL_LOCATION
 from solar.core.handlers.base import TempFileHandler
 from solar.core.log import log
-from solar import errors
 
 # otherwise fabric will sys.exit(1) in case of errors
 env.warn_only = True
@@ -40,7 +39,8 @@ class AnsibleTemplate(TempFileHandler):
         self._copy_templates_and_scripts(resource, action_name)
         self.transport_sync.copy(resource, self.dst, '/tmp')
         self.transport_sync.copy(resource, '/vagrant/library', '/tmp')
-        self.transport_sync.sync_all()
+        sync_results = self.transport_sync.sync_all()
+        self.verify_sync_results(sync_results)
 
         # remote paths are not nested inside solar_local
         remote_playbook_file = playbook_file.replace(
@@ -52,15 +52,8 @@ class AnsibleTemplate(TempFileHandler):
                      '-i', remote_inventory_file, remote_playbook_file]
         log.debug('EXECUTING: %s', ' '.join(call_args))
 
-        out = self.transport_run.run(resource, *call_args)
+        rst = self.transport_run.run(resource, *call_args)
-        log.debug(out)
+        self.verify_run_result(call_args, rst)
-        if out.failed:
-            raise errors.SolarError(out)
-
-        # with fabric_api.shell_env(ANSIBLE_HOST_KEY_CHECKING='False'):
-        #     out = fabric_api.local(' '.join(call_args), capture=True)
-        # if out.failed:
-        #     raise errors.SolarError(out)
 
     def _create_inventory(self, r):
         directory = self.dirs[r.name]

solar/core/handlers/base.py

@@ -21,6 +21,7 @@ import tempfile
 from solar.core.log import log
 from solar.core.transports.ssh import SSHRunTransport
 from solar.core.transports.ssh import SSHSyncTransport
+from solar import errors
 from solar import utils
 
 
@@ -42,6 +43,22 @@ class BaseHandler(object):
         self.transport_sync.bind_with(self.transport_run)
         self.transport_run.bind_with(self.transport_sync)
 
+    def verify_sync_results(self, results):
+        for result in results:
+            if isinstance(result, tuple) and len(result) == 3:
+                # TODO Include file information in result
+                rc, out, err = result
+                log.debug('RC %s OUT %s ERR %s', rc, out, err)
+                if rc:
+                    raise errors.SolarError(err)
+
+    def verify_run_result(self, cmd, result):
+        rc, out, err = result
+        log.debug('CMD %r RC %s OUT %s ERR %s', cmd, rc, out, err)
+        if rc:
+            message = 'CMD %r failed RC %s ERR %s' % (cmd, rc, err)
+            raise errors.SolarError(message)
+
     def __enter__(self):
         return self
 

solar/core/handlers/naive_sync.py

@@ -27,4 +27,5 @@ class NaiveSync(BaseHandler):
         # to understand where src comes from
         for item in args['sources']:
             self.transport_sync.copy(resource, item['src'], item['dst'])
-            self.transport_sync.sync_all()
+            results = self.transport_sync.sync_all()
+            self.verify_sync_results(results)

solar/core/handlers/puppet.py

@@ -37,7 +37,8 @@ class Puppet(TempFileHandler):
 
         self.prepare_templates_and_scripts(resource, action_file, '')
         self.transport_sync.copy(resource, action_file, action_file_name)
-        self.transport_sync.sync_all()
+        sync_results = self.transport_sync.sync_all()
+        self.verify_sync_results(sync_results)
 
         cmd_args = ['puppet', 'apply', '-vd',
                     action_file_name,
@@ -46,7 +47,7 @@ class Puppet(TempFileHandler):
             cmd_args.append('--modulepath={}'.format(
                 resource.args['puppet_modules']))
 
-        cmd = self.transport_run.run(
+        rc, out, err = self.transport_run.run(
             resource,
             *cmd_args,
             env={
@@ -55,12 +56,12 @@ class Puppet(TempFileHandler):
             use_sudo=True,
             warn_only=True
         )
+        log.debug('CMD %r RC %s OUT %s ERR %s', cmd_args, rc, out, err)
         # 0 - no changes, 2 - successfull changes
-        if cmd.return_code not in [0, 2]:
+        if rc not in [0, 2]:
             raise errors.SolarError(
-                'Puppet for {} failed with {}'.format(
+                'Puppet for {} failed with RC {}'.format(
-                    resource.name, cmd.return_code))
+                    resource.name, rc))
-        return cmd
 
     def _make_args(self, resource):
         return {resource.name: {'input': resource.args}}

solar/core/handlers/shell.py

@@ -18,7 +18,6 @@ import os
 from solar.core.handlers.base import SOLAR_TEMP_LOCAL_LOCATION
 from solar.core.handlers.base import TempFileHandler
 from solar.core.log import log
-from solar import errors
 
 
 class Shell(TempFileHandler):
@@ -34,17 +33,14 @@ class Shell(TempFileHandler):
         self._copy_templates_and_scripts(resource, action_name)
 
         self.transport_sync.copy(resource, self.dst, '/tmp')
-        self.transport_sync.sync_all()
+        sync_results = self.transport_sync.sync_all()
+        # TODO Include file information in result
+        self.verify_sync_results(sync_results)
 
-        cmd = self.transport_run.run(
+        rst = self.transport_run.run(
             resource,
             'bash', action_file_name,
             use_sudo=True,
             warn_only=True
         )
-
+        self.verify_run_results(['bash', action_file_name], rst)
-        if cmd.return_code:
-            raise errors.SolarError(
-                'Bash execution for {} failed with {}'.format(
-                    resource.name, cmd.return_code))
-        return cmd

solar/core/transports/base.py

@@ -38,7 +38,7 @@ class Executor(object):
 
     def run(self, transport):
         if self.valid:
-            self._executor(transport)
+            return self._executor(transport)
 
 
 class SolarRunResultWrp(object):
@@ -131,8 +131,10 @@ class SyncTransport(SolarTransport):
             self.preprocess(executor)
 
     def run_all(self):
+        rst = []
         for executor in self.executors:
-            executor.run(self)
+            rst.append(executor.run(self))
+        return rst
 
     def sync_all(self):
         """Syncs all
@@ -142,8 +144,9 @@ class SyncTransport(SolarTransport):
         Could be someday changed to parallel thing.
         """
         self.preprocess_all()
-        self.run_all()
+        rst = self.run_all()
         self.executors = []  # clear after all
+        return rst
 
 
 class RunTransport(SolarTransport):

solar/core/transports/bat.py

@@ -60,8 +60,12 @@ class OnAll(object):
 
     def __get__(self, obj, objtype):
         def _inner(*args, **kwargs):
+            results = []
             for transport in obj._used_transports:
-                getattr(transport, self._target)(*args, **kwargs)
+                rst = getattr(transport, self._target)(*args, **kwargs)
+                if rst:
+                    results.extend(rst)
+            return results
         return _inner
 
 

solar/core/transports/rsync.py

@@ -12,11 +12,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from fabric import api as fabric_api
-
 from solar.core.log import log
 from solar.core.transports.base import Executor
 from solar.core.transports.base import SyncTransport
+from solar.utils import execute
 
 # XXX:
 # currently we don't support key verification or acceptation
@@ -55,9 +54,8 @@ class RsyncSyncTransport(SyncTransport):
                          _from=_from,
                          _to=_to)
 
-        rsync_executor = lambda transport: fabric_api.local(
+        rsync_executor = lambda transport: execute(
-            rsync_cmd
+            rsync_cmd, shell=True)
-        )
 
         log.debug("RSYNC CMD: %r" % rsync_cmd)
 

solar/core/transports/ssh_raw.py

@@ -12,10 +12,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-from fabric import api as fabric_api
 
 from solar.core.log import log
 from solar.core.transports.base import RunTransport
+from solar.utils import execute
 
 
 class _RawSSHTransport(object):
@@ -68,5 +68,6 @@ class RawSSHRunTransport(RunTransport, _RawSSHTransport):
         ssh_cmd += (self._ssh_command_host(settings), remote_cmd)
 
         log.debug("RAW SSH CMD: %r", ssh_cmd)
+        # TODO convert it to SolarRunResult
 
-        return fabric_api.local(' '.join(ssh_cmd))
+        return execute(' '.join(ssh_cmd), shell=True)

solar/utils.py

@@ -43,6 +43,17 @@ def communicate(command, data):
         stderr=subprocess.PIPE)
     return popen.communicate(input=data)[0]
 
+
+def execute(command, shell=False):
+    popen = subprocess.Popen(
+        command,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        shell=shell)
+    out, err = popen.communicate()
+    return popen.returncode, out, err
+
+
 # Configure jinja2 filters
 jinja_env_with_filters = Environment()
 jinja_env_with_filters.filters['to_json'] = to_json