extended usage of Executor class, added client-side timeout options

7 files changed, 206 insertions, 143 deletions

diff --git a/src/lib/Bcfg2/Client/Client.py b/src/lib/Bcfg2/Client/Client.py
index 6aa9ace1f..0488fcf21 100644
--- a/src/lib/Bcfg2/Client/Client.py
+++ b/src/lib/Bcfg2/Client/Client.py
@@ -14,10 +14,9 @@ import Bcfg2.Client.XML
 import Bcfg2.Client.Proxy
 import Bcfg2.Client.Frame
 import Bcfg2.Client.Tools
-from Bcfg2.Utils import locked
+from Bcfg2.Utils import locked, Executor
 from Bcfg2.Compat import xmlrpclib
 from Bcfg2.version import __version__
-from subprocess import Popen, PIPE
 
 
 class Client(object):
@@ -42,6 +41,9 @@ class Client(object):
                                    to_file=self.setup['logging'])
         self.logger = logging.getLogger('bcfg2')
         self.logger.debug(self.setup)
+
+        self.cmd = Executor(self.setup['command_timeout'])
+
         if self.setup['bundle_quick']:
             if not self.setup['bundle'] and not self.setup['skipbundle']:
                 self.logger.error("-Q option requires -b or -B")
@@ -95,16 +97,14 @@ class Client(object):
                          stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH |
                          stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH |
                          stat.S_IWUSR)  # 0755
-                proc = Popen(scriptname, stdin=PIPE, stdout=PIPE, stderr=PIPE)
-                ret.text, err = proc.communicate()
-                rv = proc.wait()
-                if err:
+                rv = self.cmd.run(scriptname, timeout=self.setup['timeout'])
+                if rv.stderr:
                     self.logger.warning("Probe %s has error output: %s" %
-                                        (name, err))
-                if rv:
+                                        (name, rv.stderr))
+                if not rv.success:
                     self._probe_failure(name, "Return value %s" % rv)
                 self.logger.info("Probe %s has result:" % name)
-                self.logger.info(ret.text)
+                self.logger.info(rv.stdout)
             finally:
                 os.unlink(scriptname)
         except SystemExit:
diff --git a/src/lib/Bcfg2/Client/Tools/SELinux.py b/src/lib/Bcfg2/Client/Tools/SELinux.py
index 414ca1f93..08d943251 100644
--- a/src/lib/Bcfg2/Client/Tools/SELinux.py
+++ b/src/lib/Bcfg2/Client/Tools/SELinux.py
@@ -12,7 +12,6 @@ import seobject
 import Bcfg2.Client.XML
 import Bcfg2.Client.Tools
 from Bcfg2.Client.Tools.POSIX.File import POSIXFile
-from subprocess import Popen, PIPE
 
 
 def pack128(int_val):
@@ -734,9 +733,7 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
                 self._all = dict()
                 self.logger.debug("SELinux: Getting modules from semodule")
                 try:
-                    proc = Popen(['semodule', '-l'], stdout=PIPE, stderr=PIPE)
-                    out = proc.communicate()[0]
-                    rv = proc.wait()
+                    rv = self.tool.cmd.run(['semodule', '-l'])
                 except OSError:
                     # semanage failed; probably not in $PATH.  try to
                     # get the list of modules from the filesystem
@@ -745,13 +742,9 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
                                       err)
                     self._all.update(self._all_records_from_filesystem())
                 else:
-                    if rv:
-                        self.logger.error("SELinux: Failed to run semodule: %s"
-                                          % err)
-                        self._all.update(self._all_records_from_filesystem())
-                    else:
+                    if rv.success:
                         # ran semodule successfully
-                        for line in out.splitlines():
+                        for line in rv.stdout.splitlines():
                             mod, version = line.split()
                             self._all[mod] = (version, 1)
 
@@ -759,6 +752,10 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
                         for mod in self._all_records_from_filesystem().keys():
                             if mod not in self._all:
                                 self._all[mod] = ('', 0)
+                    else:
+                        self.logger.error("SELinux: Failed to run semodule: %s"
+                                          % rv.error)
+                        self._all.update(self._all_records_from_filesystem())
         return self._all
 
     def _all_records_from_filesystem(self):
@@ -870,26 +867,23 @@ class SELinuxSemoduleHandler(SELinuxEntryHandler):
         self.logger.debug("Install SELinux module %s with semodule -i %s" %
                           (entry.get('name'), self._filepath(entry)))
         try:
-            proc = Popen(['semodule', '-i', self._filepath(entry)],
-                         stdout=PIPE, stderr=PIPE)
-            err = proc.communicate()[1]
-            rv = proc.wait()
+            rv = self.tool.cmd.run(['semodule', '-i', self._filepath(entry)])
         except OSError:
             err = sys.exc_info()[1]
             self.logger.error("Failed to install SELinux module %s with "
                               "semodule: %s" % (entry.get("name"), err))
             return False
-        if rv:
-            self.logger.error("Failed to install SELinux module %s with "
-                              "semodule: %s" % (entry.get("name"), err))
-            return False
-        else:
+        if rv.success:
             if entry.get("disabled", "false").lower() == "true":
                 self.logger.warning("SELinux: Cannot disable modules with "
                                     "semodule")
                 return False
             else:
                 return True
+        else:
+            self.logger.error("Failed to install SELinux module %s with "
+                              "semodule: %s" % (entry.get("name"), rv.error))
+            return False
 
     def _addargs(self, entry):
         """ argument list for adding entries """
diff --git a/src/lib/Bcfg2/Client/Tools/__init__.py b/src/lib/Bcfg2/Client/Tools/__init__.py
index 7014f334f..cd86a2a4b 100644
--- a/src/lib/Bcfg2/Client/Tools/__init__.py
+++ b/src/lib/Bcfg2/Client/Tools/__init__.py
@@ -4,9 +4,8 @@ import os
 import sys
 import stat
 import select
-import logging
-import subprocess
 import Bcfg2.Client.XML
+from Bcfg2.Utils import Executor, ClassName
 from Bcfg2.Compat import input, walk_packages  # pylint: disable=W0622
 
 __all__ = [m[1] for m in walk_packages(path=__path__)]
@@ -26,103 +25,6 @@ class ToolInstantiationError(Exception):
     pass
 
 
-class ExecutorResult(object):
-    """ Returned as the result of a call to
-    :func:`Bcfg2.Client.Tools.Executor.run`. The result can be
-    accessed via the instance variables, documented below, as a
-    boolean (which is equivalent to
-    :attr:`Bcfg2.Client.Tools.ExecutorResult.success`), or as a tuple,
-    which, for backwards compatibility, is equivalent to
-    ``(result.retval, result.stdout.splitlines())``. """
-
-    def __init__(self, stdout, stderr, retval):
-        #: The output of the command
-        self.stdout = stdout
-
-        #: The error produced by the command
-        self.stderr = stderr
-
-        #: The return value of the command.
-        self.retval = retval
-
-        #: Whether or not the command was successful.  If the
-        #: ExecutorResult is used as a boolean, ``success`` is
-        #: returned.
-        self.success = retval == 0
-
-        #: A friendly error message
-        self.error = None
-        if self.retval:
-            if self.stderr:
-                self.error = "%s (rv: %s)" % (self.stderr, self.retval)
-            elif self.stdout:
-                self.error = "%s (rv: %s)" % (self.stdout, self.retval)
-            else:
-                self.error = "No output or error; return value %s" % \
-                    self.retval
-
-    def __repr__(self):
-        if self.error:
-            return "Errored command result: %s" % self.error
-        elif self.stdout:
-            return "Successful command result: %s" % self.stdout
-        else:
-            return "Successful command result: No output"
-
-    def __getitem__(self, idx):
-        """ This provides compatibility with the old Executor, which
-        returned a tuple of (return value, stdout split by lines). """
-        return (self.retval, self.stdout.splitlines())[idx]
-
-    def __nonzero__(self):
-        return self.__bool__()
-
-    def __bool__(self):
-        return self.success
-
-
-class Executor(object):
-    """ A better version of Bcfg2.Client.Tool.Executor, which captures
-    stderr, raises exceptions on error, and doesn't use the shell to
-    execute by default """
-
-    def __init__(self):
-        self.logger = logging.getLogger(self.__class__.__name__)
-
-    def run(self, command, inputdata=None, shell=False):
-        """ Run a command, given as a list, optionally giving it the
-        specified input data """
-        if isinstance(command, str):
-            cmdstr = command
-        else:
-            cmdstr = " ".join(command)
-        self.logger.debug("Running: %s" % cmdstr)
-        proc = subprocess.Popen(command, shell=shell, bufsize=16384,
-                                stdin=subprocess.PIPE, stdout=subprocess.PIPE,
-                                stderr=subprocess.PIPE, close_fds=True)
-        if inputdata:
-            for line in inputdata.splitlines():
-                self.logger.debug('> %s' % line)
-        (stdout, stderr) = proc.communicate(input=inputdata)
-        for line in stdout.splitlines():  # pylint: disable=E1103
-            self.logger.debug('< %s' % line)
-        for line in stderr.splitlines():  # pylint: disable=E1103
-            self.logger.info(line)
-        return ExecutorResult(stdout, stderr, proc.wait())
-
-
-class ClassName(object):
-    """ This very simple descriptor class exists only to get the name
-    of the owner class.  This is used because, for historical reasons,
-    we expect every tool to have a ``name`` attribute that is in
-    almost all cases the same as the ``__class__.__name__`` attribute
-    of the plugin object.  This makes that more dynamic so that each
-    plugin isn't repeating its own name."""
-
-    def __get__(self, inst, owner):
-        return owner.__name__
-
-
 class Tool(object):
     """ The base tool class.  All tools subclass this.
 
@@ -197,9 +99,9 @@ class Tool(object):
         #: The XML configuration for this client
         self.config = config
 
-        #: An :class:`Bcfg2.Client.Tools.Executor` object for
+        #: An :class:`Bcfg2.Utils.Executor` object for
         #: running external commands.
-        self.cmd = Executor()
+        self.cmd = Executor(timeout=self.setup['command_timeout'])
 
         #: A list of entries that have been modified by this tool
         self.modified = []
diff --git a/src/lib/Bcfg2/Options.py b/src/lib/Bcfg2/Options.py
index fe9a480a4..3f4e9a83c 100644
--- a/src/lib/Bcfg2/Options.py
+++ b/src/lib/Bcfg2/Options.py
@@ -334,6 +334,16 @@ def get_bool(val):
         raise ValueError("Not a boolean value", val)
 
 
+def get_timeout(val):
+    """ convert the timeout value into a float or None """
+    if val is None:
+        return val
+    timeout = float(val)  # pass ValueError up the stack
+    if timeout <= 0:
+        return None
+    return timeout
+
+
 def get_size(value):
     """ Given a number of bytes in a human-readable format (e.g.,
     '512m', '2g'), get the absolute number of bytes as an integer """
@@ -801,6 +811,16 @@ CLIENT_EXIT_ON_PROBE_FAILURE = \
            long_arg=True,
            cf=('client', 'exit_on_probe_failure'),
            cook=get_bool)
+CLIENT_PROBE_TIMEOUT = \
+    Option("Timeout when running client probes",
+           default=None,
+           cf=('client', 'probe_timeout'),
+           cook=get_timeout)
+CLIENT_COMMAND_TIMEOUT = \
+    Option("Timeout when client runs other external commands (not probes)",
+           default=None,
+           cf=('client', 'command_timeout'),
+           cook=get_timeout)
 
 # bcfg2-test and bcfg2-lint options
 TEST_NOSEOPTS = \
@@ -1131,7 +1151,9 @@ CLIENT_COMMON_OPTIONS = \
          serverCN=CLIENT_SCNS,
          timeout=CLIENT_TIMEOUT,
          decision_list=CLIENT_DECISION_LIST,
-         probe_exit=CLIENT_EXIT_ON_PROBE_FAILURE)
+         probe_exit=CLIENT_EXIT_ON_PROBE_FAILURE,
+         probe_timeout=CLIENT_PROBE_TIMEOUT,
+         command_timeout=CLIENT_COMMAND_TIMEOUT)
 CLIENT_COMMON_OPTIONS.update(DRIVER_OPTIONS)
 CLIENT_COMMON_OPTIONS.update(CLI_COMMON_OPTIONS)
 
diff --git a/src/lib/Bcfg2/Server/Plugin/base.py b/src/lib/Bcfg2/Server/Plugin/base.py
index e74909ee9..25a687874 100644
--- a/src/lib/Bcfg2/Server/Plugin/base.py
+++ b/src/lib/Bcfg2/Server/Plugin/base.py
@@ -2,6 +2,7 @@
 
 import os
 import logging
+from Bcfg2.Utils import ClassName
 
 
 class Debuggable(object):
@@ -59,18 +60,6 @@ class Debuggable(object):
             self.logger.error(message)
 
 
-class ClassName(object):
-    """ This very simple descriptor class exists only to get the name
-    of the owner class.  This is used because, for historical reasons,
-    we expect every plugin to have a ``name`` attribute that is in
-    almost all cases the same as the ``__class__.__name__`` attribute
-    of the plugin object.  This makes that more dynamic so that each
-    plugin isn't repeating its own name. """
-
-    def __get__(self, inst, owner):
-        return owner.__name__
-
-
 class Plugin(Debuggable):
     """ The base class for all Bcfg2 Server plugins. """
 
diff --git a/src/lib/Bcfg2/Utils.py b/src/lib/Bcfg2/Utils.py
index 247e4f16b..3b1559528 100644
--- a/src/lib/Bcfg2/Utils.py
+++ b/src/lib/Bcfg2/Utils.py
@@ -3,9 +3,25 @@ used by both client and server.  Stuff that doesn't fit anywhere
 else. """
 
 import fcntl
+import logging
+import threading
+import subprocess
 from Bcfg2.Compat import any  # pylint: disable=W0622
 
 
+class ClassName(object):
+    """ This very simple descriptor class exists only to get the name
+    of the owner class.  This is used because, for historical reasons,
+    we expect every server plugin and every client tool to have a
+    ``name`` attribute that is in almost all cases the same as the
+    ``__class__.__name__`` attribute of the plugin object.  This makes
+    that more dynamic so that each plugin and tool isn't repeating its own
+    name."""
+
+    def __get__(self, inst, owner):
+        return owner.__name__
+
+
 class PackedDigitRange(object):
     """ Representation of a set of integer ranges. A range is
     described by a comma-delimited string of integers and ranges,
@@ -69,9 +85,147 @@ class PackedDigitRange(object):
 
 
 def locked(fd):
-    """ Acquire a lock on a file """
+    """ Acquire a lock on a file.
+
+    :param fd: The file descriptor to lock
+    :type fd: int
+    :returns: bool - True if the file is already locked, False
+              otherwise """
     try:
         fcntl.lockf(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
     except IOError:
         return True
     return False
+
+
+class ExecutorResult(object):
+    """ Returned as the result of a call to
+    :func:`Bcfg2.Utils.Executor.run`. The result can be accessed via
+    the instance variables, documented below, as a boolean (which is
+    equivalent to :attr:`Bcfg2.Utils.ExecutorResult.success`), or as a
+    tuple, which, for backwards compatibility, is equivalent to
+    ``(result.retval, result.stdout.splitlines())``."""
+
+    def __init__(self, stdout, stderr, retval):
+        #: The output of the command
+        self.stdout = stdout
+
+        #: The error produced by the command
+        self.stderr = stderr
+
+        #: The return value of the command.
+        self.retval = retval
+
+        #: Whether or not the command was successful.  If the
+        #: ExecutorResult is used as a boolean, ``success`` is
+        #: returned.
+        self.success = retval == 0
+
+        #: A friendly error message
+        self.error = None
+        if self.retval:
+            if self.stderr:
+                self.error = "%s (rv: %s)" % (self.stderr, self.retval)
+            elif self.stdout:
+                self.error = "%s (rv: %s)" % (self.stdout, self.retval)
+            else:
+                self.error = "No output or error; return value %s" % \
+                    self.retval
+
+    def __repr__(self):
+        if self.error:
+            return "Errored command result: %s" % self.error
+        elif self.stdout:
+            return "Successful command result: %s" % self.stdout
+        else:
+            return "Successful command result: No output"
+
+    def __getitem__(self, idx):
+        """ This provides compatibility with the old Executor, which
+        returned a tuple of (return value, stdout split by lines). """
+        return (self.retval, self.stdout.splitlines())[idx]
+
+    def __nonzero__(self):
+        return self.__bool__()
+
+    def __bool__(self):
+        return self.success
+
+
+class Executor(object):
+    """ A convenient way to run external commands with
+    :class:`subprocess.Popen` """
+
+    def __init__(self, timeout=None):
+        """
+        :param timeout: Set a default timeout for all commands run by
+                        this Executor object
+        :type timeout: float
+        """
+        self.logger = logging.getLogger(self.__class__.__name__)
+        self.timeout = timeout
+
+    def _timeout_callback(self, proc):
+        """ Get a callback (suitable for passing to
+        :class:`threading.Timer`) that kills the given process.
+
+        :param proc: The process to kill upon timeout.
+        :type proc: subprocess.Popen
+        :returns: function """
+        def _timeout():
+            """ Callback that kills ``proc`` """
+            if proc.poll() == None:
+                try:
+                    proc.kill()
+                    self.logger.warning("Process exceeeded timeout, killing")
+                except OSError:
+                    pass
+
+        return _timeout
+
+    def run(self, command, inputdata=None, shell=False, timeout=None):
+        """ Run a command, given as a list, optionally giving it the
+        specified input data.
+
+        :param command: The command to run, as a list (preferred) or
+                        as a string.  See :class:`subprocess.Popen` for
+                        details.
+        :type command: list or string
+        :param inputdata: Data to pass to the command on stdin
+        :type inputdata: string
+        :param shell: Run the given command in a shell (not recommended)
+        :type shell: bool
+        :param timeout: Kill the command if it runs longer than this
+                        many seconds.  Set to 0 or -1 to explicitly
+                        override a default timeout.
+        :type timeout: float
+        :returns: :class:`Bcfg2.Utils.ExecutorResult`
+        """
+        if isinstance(command, str):
+            cmdstr = command
+        else:
+            cmdstr = " ".join(command)
+        self.logger.debug("Running: %s" % cmdstr)
+        proc = subprocess.Popen(command, shell=shell, bufsize=16384,
+                                stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE, close_fds=True)
+        if timeout is None:
+            timeout = self.timeout
+        if timeout is not None:
+            timer = threading.Timer(float(timeout),
+                                    self._timeout_callback(proc),
+                                    [proc])
+            timer.start()
+        try:
+            if inputdata:
+                for line in inputdata.splitlines():
+                    self.logger.debug('> %s' % line)
+            (stdout, stderr) = proc.communicate(input=inputdata)
+            for line in stdout.splitlines():  # pylint: disable=E1103
+                self.logger.debug('< %s' % line)
+            for line in stderr.splitlines():  # pylint: disable=E1103
+                self.logger.info(line)
+            return ExecutorResult(stdout, stderr, proc.wait())
+        finally:
+            if timeout is not None:
+                timer.cancel()
diff --git a/testsuite/Testsrc/Testlib/TestClient/TestTools/TestPOSIX/Test__init.py b/testsuite/Testsrc/Testlib/TestClient/TestTools/TestPOSIX/Test__init.py
index 4048be7ca..bf7f1eecc 100644
--- a/testsuite/Testsrc/Testlib/TestClient/TestTools/TestPOSIX/Test__init.py
+++ b/testsuite/Testsrc/Testlib/TestClient/TestTools/TestPOSIX/Test__init.py
@@ -37,6 +37,8 @@ def get_posix_object(logger=None, setup=None, config=None):
         logger.debug = Mock(side_effect=print_msg)
     if not setup:
         setup = MagicMock()
+    if 'command_timeout' not in setup:
+        setup['command_timeout'] = None
     return Bcfg2.Client.Tools.POSIX.POSIX(logger, setup, config)