Merge lp:~zyga/checkbox/fix-1299210 into lp:checkbox

[Zygmunt Krynicki]

3e1382d plainbox:ctrl: add CheckBoxExecutionController.get_warm_up_for_job
ac93d40 plainbox:abc: add IExecutionController.get_warm_up_for_job()
b8b9380 plainbox:ctrl: implement RootViaPTL1ExecutionController.get_warm_up_for_job
0f9b67b plainbox:runner: fix typo in comment
d0dad89 plainbox:runner: split off _get_ctrl_for_job() from _run_extcmd()
1437bb0 plainbox:runner: add JobRunner.get_warm_up_sequence()
536eceb plainbox:commands:run: use runner.get_warm_up_sequence()
ae60a9f plainbox:abc: add IJobRunner.get_warm_up_sequence()
bb587dc plainbox:commands:run: simplify tests related to auth warmup
87dde25 checkbox-ng:commands:cli: refactor CliInvocation
cae29ce checkbox-ng:commands:cli: use get_warm_up_sequence
02b129a checkbox-ng:commands:cli: remove CliInvocation._auth_warmup_needed()
3628855 plainbox:commands:run: remove RunInvocation._auth_warmup_needed()
4d02c62 plainbox:runner: drop authenticate_warmup()

[Daniel Manrique (roadmr)]

Looks good, I like the part where it will just continue asking for the password, we had a lot of trouble with checkbox in the past on that scenario :)

review: Approve

=== modified file 'checkbox-ng/checkbox_ng/commands/cli.py'
--- checkbox-ng/checkbox_ng/commands/cli.py	2014-05-12 13:13:13 +0000
+++ checkbox-ng/checkbox_ng/commands/cli.py	2014-05-13 09:24:14 +0000
@@ -47,7 +47,6 @@
 from plainbox.impl.job import JobTreeNode
 from plainbox.impl.result import DiskJobResult, MemoryJobResult
 from plainbox.impl.runner import JobRunner
-from plainbox.impl.runner import authenticate_warmup
 from plainbox.impl.runner import slugify
 from plainbox.impl.secure.config import Unset
 from plainbox.impl.secure.qualifiers import CompositeQualifier
@@ -303,7 +302,10 @@
                         self.provider_list,
                         self.settings['default_whitelist']))
         manager.checkpoint()
-
+        runner = JobRunner(
+            manager.storage.location, self.provider_list,
+            os.path.join(manager.storage.location, 'io-logs'),
+            command_io_delegate=self)
         if self.is_interactive and not resume_in_progress:
             # Pre-run all local jobs
             desired_job_list = select_jobs(
@@ -315,13 +317,13 @@
             self._update_desired_job_list(manager, desired_job_list)
             # Ask the password before anything else in order to run local jobs
             # requiring privileges
-            if self._auth_warmup_needed(manager):
+            warm_up_list = runner.get_warm_up_sequence(manager.state.run_list)
+            if warm_up_list:
                 print("[ {} ]".format(_("Authentication")).center(80, '='))
-                return_code = authenticate_warmup()
-                if return_code:
-                    raise SystemExit(return_code)
+                for warm_up_func in warm_up_list:
+                    warm_up_func()
             self._local_only = True
-            self._run_jobs(ns, manager)
+            self._run_jobs(runner, ns, manager)
             self._local_only = False
 
         if not resume_in_progress:
@@ -332,11 +334,11 @@
             if self.is_interactive:
                 # Ask the password before anything else in order to run jobs
                 # requiring privileges
-                if self._auth_warmup_needed(manager):
+                warm_up_list = runner.get_warm_up_sequence(manager.state.run_list)
+                if warm_up_list:
                     print("[ {} ]".format(_("Authentication")).center(80, '='))
-                    return_code = authenticate_warmup()
-                    if return_code:
-                        raise SystemExit(return_code)
+                    for warm_up_func in warm_up_list:
+                        warm_up_func()
                 tree = SelectableJobTreeNode.create_tree(
                     manager.state.run_list,
                     legacy_mode=True)
@@ -365,7 +367,7 @@
                 else:
                     print(_("Estimated duration cannot be determined for"
                             " manual jobs."))
-        self._run_jobs(ns, manager)
+        self._run_jobs(runner, ns, manager)
         manager.destroy()
 
         # FIXME: sensible return value
@@ -411,31 +413,11 @@
             manager.state.metadata.running_job_name = None
             manager.checkpoint()
 
-    def _run_jobs(self, ns, manager):
-        runner = JobRunner(
-            manager.storage.location, self.provider_list,
-            os.path.join(manager.storage.location, 'io-logs'),
-            command_io_delegate=self)
+    def _run_jobs(self, runner, ns, manager):
         self._run_jobs_with_session(ns, manager, runner)
         if not self._local_only:
             self.save_results(manager)
 
-    def _auth_warmup_needed(self, manager):
-        # Don't warm up plainbox-trusted-launcher-1 if none of the providers
-        # use it. We assume that the mere presence of a provider makes it
-        # possible for a root job to be preset but it could be improved to
-        # actually know when this step is absolutely not required (no local
-        # jobs, no jobs
-        # need root)
-        if all(not provider.secure for provider in self.provider_list):
-            return False
-        # Don't use authentication warm-up if none of the jobs on the run list
-        # requires it.
-        if all(job.user is None for job in manager.state.run_list):
-            return False
-        # Otherwise, do pre-authentication
-        return True
-
     def save_results(self, manager):
         if self.is_interactive:
             print("[ {} ]".format(_('Results')).center(80, '='))

=== modified file 'plainbox/plainbox/abc.py'
--- plainbox/plainbox/abc.py	2014-04-30 18:34:21 +0000
+++ plainbox/plainbox/abc.py	2014-05-13 09:24:14 +0000
@@ -356,6 +356,23 @@
         # executing. We could expose the underlying process mechanics so that
         # QT/GTK applications could tie that directly into their event loop.
 
+    @abstractmethod
+    def get_warm_up_sequence(self, job_list):
+        """
+        Determine if authentication warm-up may be needed.
+
+        :param job_lits:
+            A list of jobs that may be executed
+        :returns:
+            A list of methods to call to complete the warm-up step.
+
+        Authentication warm-up is related to the plainbox-secure-launcher-1
+        program that can be 'warmed-up' to perhaps cache the security
+        credentials. This is usually done early in the testing process so that
+        we can prompt for passwords before doing anything that takes an
+        extended amount of time.
+        """
+
 
 class IUserInterfaceIO(metaclass=ABCMeta):
     """
@@ -652,6 +669,22 @@
             The higher the value, the more applicable this controller is.
         """
 
+    @abstractmethod
+    def get_warm_up_for_job(self, job):
+        """
+        Get a warm-up function that should be called before running this job.
+
+        :returns:
+            A callable (without arguments) or None, depending on needs of a
+            particular job.
+
+        The warm-up function is an optional advisory interface to improve the
+        testing experience for the user. A job may not require any warm-up. In
+        such case the return value is None. Note that even if this function is
+        not called the testing process should perform the same way (correctly)
+        but the user may be prompted for additional steps mid-way.
+        """
+
 
 class IBuildSystem(metaclass=ABCMeta):
     """

=== modified file 'plainbox/plainbox/impl/commands/run.py'
--- plainbox/plainbox/impl/commands/run.py	2014-05-09 12:48:33 +0000
+++ plainbox/plainbox/impl/commands/run.py	2014-05-13 09:24:14 +0000
@@ -45,7 +45,6 @@
 from plainbox.impl.exporter import get_all_exporters
 from plainbox.impl.result import DiskJobResult, MemoryJobResult
 from plainbox.impl.runner import JobRunner
-from plainbox.impl.runner import authenticate_warmup
 from plainbox.impl.runner import slugify
 from plainbox.impl.session import SessionManager
 from plainbox.impl.session import SessionMetaData
@@ -424,11 +423,11 @@
         Ask the password before anything else in order to run jobs requiring
         privileges
         """
-        if self._auth_warmup_needed():
+        warm_up_list = self.runner.get_warm_up_sequence(self.state.run_list)
+        if warm_up_list:
             print(_("[ Authentication ]").center(80, '='))
-            return_code = authenticate_warmup()
-            if return_code:
-                raise SystemExit(return_code)
+            for warm_up_func in warm_up_list:
+                warm_up_func()
 
     def run_all_selected_jobs(self):
         """
@@ -510,22 +509,6 @@
             answer = input("{} [{}] ".format(prompt, ", ".join(allowed)))
         return answer
 
-    def _auth_warmup_needed(self):
-        # Don't warm up plainbox-trusted-launcher-1 if none of the providers
-        # use it. We assume that the mere presence of a provider makes it
-        # possible for a root job to be preset but it could be improved to
-        # acutally know when this step is absolutely not required (no local
-        # jobs, no jobs
-        # need root)
-        if all(not provider.secure for provider in self.provider_list):
-            return False
-        # Don't use authentication warm-up if none of the jobs on the run list
-        # requires it.
-        if all(job.user is None for job in self.state.run_list):
-            return False
-        # Otherwise, do pre-authentication
-        return True
-
     def _save_results(self, output_file, input_stream):
         if output_file is sys.stdout:
             print(_("[ Results ]").center(80, '='))

=== modified file 'plainbox/plainbox/impl/commands/test_run.py'
--- plainbox/plainbox/impl/commands/test_run.py	2014-04-01 23:35:57 +0000
+++ plainbox/plainbox/impl/commands/test_run.py	2014-05-13 09:24:14 +0000
@@ -111,28 +111,16 @@
     def test_run_without_args(self, mock_check_output):
         with TestIO(combined=True) as io:
             with self.assertRaises(SystemExit) as call:
-                with patch('plainbox.impl.commands.run.authenticate_warmup') as mock_warmup:
-                    mock_warmup.return_value = 0
-                    main(['run'])
+                main(['run'])
             self.assertEqual(call.exception.args, (0,))
-        expected1 = """
-        ===============================[ Analyzing Jobs ]===============================
-        Estimated duration cannot be determined for automated jobs.
-        Estimated duration cannot be determined for manual jobs.
-        ==============================[ Running All Jobs ]==============================
-        ==================================[ Results ]===================================
-        """
-        expected2 = """
-        ===============================[ Authentication ]===============================
-        ===============================[ Analyzing Jobs ]===============================
-        Estimated duration cannot be determined for automated jobs.
-        Estimated duration cannot be determined for manual jobs.
-        ==============================[ Running All Jobs ]==============================
-        ==================================[ Results ]===================================
-        """
-        self.assertIn(io.combined, [
-            cleandoc(expected1) + "\n",
-            cleandoc(expected2) + "\n"])
+        expected = """
+        ===============================[ Analyzing Jobs ]===============================
+        Estimated duration cannot be determined for automated jobs.
+        Estimated duration cannot be determined for manual jobs.
+        ==============================[ Running All Jobs ]==============================
+        ==================================[ Results ]===================================
+        """
+        self.assertEqual(io.combined, cleandoc(expected) + "\n")
 
     def test_output_format_list(self):
         with TestIO(combined=True) as io:

=== modified file 'plainbox/plainbox/impl/ctrl.py'
--- plainbox/plainbox/impl/ctrl.py	2014-03-26 20:01:03 +0000
+++ plainbox/plainbox/impl/ctrl.py	2014-05-13 09:24:14 +0000
@@ -59,6 +59,7 @@
 from plainbox.impl.secure.rfc822 import RFC822SyntaxError
 from plainbox.impl.secure.rfc822 import gen_rfc822_records
 from plainbox.impl.session.jobs import JobReadinessInhibitor
+from plainbox.vendor import extcmd
 
 __all__ = [
     'CheckBoxSessionStateController',
@@ -559,6 +560,14 @@
         """
         return os.path.join(self._session_dir, "CHECKBOX_DATA")
 
+    def get_warm_up_for_job(self, job):
+        """
+        Get a warm-up function that should be called before running this job.
+
+        :returns:
+            None
+        """
+
 
 class UserJobExecutionController(CheckBoxExecutionController):
     """
@@ -753,6 +762,30 @@
         else:
             return 0
 
+    def get_warm_up_for_job(self, job):
+        """
+        Get a warm-up function that should be called before running this job.
+
+        :returns:
+            a warm-up function for jobs that need to run as another
+            user or None if the job can run as the current user.
+        """
+        if job.user is None:
+            return
+        else:
+            return plainbox_trusted_launcher_warm_up
+
+
+def plainbox_trusted_launcher_warm_up():
+    """
+    Warm-up function for plainbox-trusted-laucher-1.
+
+    returned by :meth:`RootViaPTL1ExecutionController.get_warm_up_for_job()`
+    """
+    warmup_popen = extcmd.ExternalCommand()
+    return warmup_popen.call(
+        ['pkexec', 'plainbox-trusted-launcher-1', '--warmup'])
+
 
 class RootViaPkexecExecutionController(
         CheckBoxDifferentialExecutionController):

=== modified file 'plainbox/plainbox/impl/runner.py'
--- plainbox/plainbox/impl/runner.py	2014-02-20 21:43:56 +0000
+++ plainbox/plainbox/impl/runner.py	2014-05-13 09:24:14 +0000
@@ -63,18 +63,6 @@
     return ''.join(c if c in valid_chars else '_' for c in _string)
 
 
-def authenticate_warmup():
-    """
-    Call the checkbox trusted launcher in warmup mode.
-
-    This will use the corresponding PolicyKit action and start the
-    authentication agent (depending on the installed policy file)
-    """
-    warmup_popen = extcmd.ExternalCommand()
-    return warmup_popen.call(
-        ['pkexec', 'plainbox-trusted-launcher-1', '--warmup'])
-
-
 class IOLogRecordGenerator(extcmd.DelegateBase):
     """
     Delegate for extcmd that generates io_log entries.
@@ -233,6 +221,29 @@
             UserJobExecutionController(session_dir, provider_list),
         ]
 
+    def get_warm_up_sequence(self, job_list):
+        """
+        Determine if authentication warm-up may be needed.
+
+        :param job_lits:
+            A list of jobs that may be executed
+        :returns:
+            A list of methods to call to complete the warm-up step.
+
+        Authentication warm-up is related to the plainbox-secure-launcher-1
+        program that can be 'warmed-up' to perhaps cache the security
+        credentials. This is usually done early in the testing process so that
+        we can prompt for passwords before doing anything that takes an
+        extended amount of time.
+        """
+        warm_up_list = []
+        for job in job_list:
+            ctrl = self._get_ctrl_for_job(job)
+            warm_up_func = ctrl.get_warm_up_for_job(job)
+            if warm_up_func is not None and warm_up_func not in warm_up_list:
+                warm_up_list.add(warm_up_func)
+        return warm_up_list
+
     def run_job(self, job, config=None):
         """
         Run the specified job an return the result
@@ -657,6 +668,21 @@
         return return_code, record_path
 
     def _run_extcmd(self, job, config, extcmd_popen):
+        ctrl = self._get_ctrl_for_job(job)
+        return ctrl.execute_job(job, config, extcmd_popen)
+
+    def _get_ctrl_for_job(self, job):
+        """
+        Get the execution controller most applicable to run this job
+
+        :param job:
+            A job definition to run
+        :returns:
+            An execution controller instance
+        :raises LookupError:
+            if no execution controller capable of running the specified job can
+            be found
+        """
         # Compute the score of each controller
         ctrl_score = [
             (ctrl, ctrl.get_score(job))
@@ -665,12 +691,11 @@
         ctrl_score.sort(key=lambda pair: pair[1])
         # Get the best score
         ctrl, score = ctrl_score[-1]
-        # Ensure that the controler is viable
+        # Ensure that the controller is viable
         if score < 0:
-            raise RuntimeError(
+            raise LookupError(
                 _("No exec controller supports job {}").format(job))
         logger.debug(
             _("Selected execution controller %s (score %d) for job %r"),
             ctrl.__class__.__name__, score, job.id)
-        # Delegate and execute
-        return ctrl.execute_job(job, config, extcmd_popen)
+        return ctrl