Ensure that jobs that have an after suspend counterpart are run before suspend (New)

checkbox-ng/plainbox/impl/ctrl.py

@@ -7,7 +7,6 @@
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
 # as published by the Free Software Foundation.
-
 #
 # Checkbox is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ -49,6 +48,7 @@ class to select the best method to execute a command of a particular job.  This
 import subprocess
 import sys
 import threading
+from functools import partial
 from subprocess import check_output, CalledProcessError, STDOUT
 
 from plainbox.abc import IJobResult
@@ -71,6 +71,7 @@ class to select the best method to execute a command of a particular job.  This
 from plainbox.impl.unit.template import TemplateUnit
 from plainbox.impl.unit.unit import MissingParam
 from plainbox.impl.validation import Severity
+from plainbox.suspend_consts import Suspend
 from plainbox.vendor import morris
 from plainbox.vendor import extcmd
 
@@ -102,12 +103,14 @@ class CheckBoxSessionStateController(ISessionStateController):
           of resource definitions.
     """
 
-    def get_dependency_set(self, job):
+    def get_dependency_set(self, job, job_list=None):
         """
         Get the set of direct dependencies of a particular job.
 
         :param job:
             A IJobDefinition instance that is to be visited
+        :param job_list:
+            List of jobs to check dependencies from
         :returns:
             set of pairs (dep_type, job_id)
 
@@ -125,15 +128,70 @@ def get_dependency_set(self, job):
             resource_deps = job.get_resource_dependencies()
         except ResourceProgramError:
             resource_deps = ()
+        suspend_job_id_list = [
+            Suspend.AUTO_JOB_ID,
+            Suspend.MANUAL_JOB_ID,
+        ]
+        if job.id in suspend_job_id_list:
+            suspend_deps = self._get_before_suspend_dependency_set(
+                job.id, job_list
+            )
+        else:
+            suspend_deps = set()
         result = set(
             itertools.chain(
                 zip(itertools.repeat(direct), direct_deps),
                 zip(itertools.repeat(resource), resource_deps),
                 zip(itertools.repeat(ordering), after_deps),
+                zip(itertools.repeat(ordering), suspend_deps),
             )
         )
         return result
 
+    def _get_before_suspend_dependency_set(self, suspend_job_id, job_list):
+        """
+        Get the set of after dependencies of a suspend job.
+
+        Jobs that have a ``also-after-suspend[-manual]`` flag should be run
+        before their associated suspend job. Similary, jobs that declare a
+        sibling with a dependency on a suspend job should be run before said
+        suspend job. This function finds these jobs and add them as a
+        dependency for their associated suspend job.
+
+        :param suspend_job_id:
+            The id of a suspend job. One of the following is expected:
+            Suspend.AUTO_JOB_ID or Suspend.MANUAL_JOB_ID.
+        :param job_list:
+            List of jobs to search dependencies on.
+        :returns:
+            A set of job ids that need to be run before the suspend job
+        """
+        p_suspend_job_id = partial(
+            self._is_job_impacting_suspend, suspend_job_id
+        )
+        suspend_deps_jobs = filter(p_suspend_job_id, job_list)
+        suspend_deps = set(job.id for job in suspend_deps_jobs)
+        return suspend_deps
+
+    def _is_job_impacting_suspend(self, suspend_job_id, job):
+        """
+        Check if the ``suspend_job_id`` job needs to be run after a given
+        ``job``. This is the case if the ``job`` has a "also after suspend"
+        flag, or if it defines a sibling that has a dependency on the suspend
+        job.
+        """
+        expected_flag = {
+            Suspend.AUTO_JOB_ID: Suspend.AUTO_FLAG,
+            Suspend.MANUAL_JOB_ID: Suspend.MANUAL_FLAG,
+        }.get(suspend_job_id)
+        if job.flags and expected_flag in job.flags:
+            return True
+        if job.siblings:
+            for sibling_data in json.loads(job.tr_siblings()):
+                if suspend_job_id in sibling_data.get("depends", []):
+                    return True
+        return False
+
     def get_inhibitor_list(self, session_state, job):
         """
         Get a list of readiness inhibitors that inhibit a particular job.
@@ -236,8 +294,59 @@ def get_inhibitor_list(self, session_state, job):
                     related_job=dep_job_state.job,
                 )
                 inhibitors.append(inhibitor)
+        if job.id in [Suspend.AUTO_JOB_ID, Suspend.MANUAL_JOB_ID]:
+            for inhibitor in self._get_suspend_inhibitor_list(
+                session_state, job
+            ):
+                inhibitors.append(inhibitor)
         return inhibitors
 
+    def _get_suspend_inhibitor_list(self, session_state, suspend_job):
+        """
+        Get a list of readiness inhibitors that inhibit a suspend job.
+
+        Jobs that have a ``also-after-suspend[-manual]`` flag should be run
+        before their associated suspend job. Similary, jobs that declare a
+        sibling with a dependency on a suspend job should be run before said
+        suspend job. This function finds these jobs and add them as a
+        inhibitor for their associated suspend job.
+
+        :param session_state:
+            A SessionState instance that is used to interrogate the
+            state of the session where it matters for a particular
+            job. Currently this is used to access resources and job
+            results.
+        :param suspend_job:
+            A suspend job.
+        :returns:
+            List of JobReadinessInhibitor
+        """
+        suspend_inhibitors = []
+        undesired_inhibitor = JobReadinessInhibitor(
+            cause=InhibitionCause.UNDESIRED
+        )
+        # We are only interested in jobs that are actually going to run
+        run_list = [
+            state.job
+            for state in session_state.job_state_map.values()
+            if undesired_inhibitor not in state.readiness_inhibitor_list
+        ]
+        p_suspend_job_id = partial(
+            self._is_job_impacting_suspend, suspend_job.id
+        )
+        suspend_inhibitors_jobs = filter(p_suspend_job_id, run_list)
+        for job in suspend_inhibitors_jobs:
+            if (
+                session_state.job_state_map[job.id].result.outcome
+                == IJobResult.OUTCOME_NONE
+            ):
+                inhibitor = JobReadinessInhibitor(
+                    cause=InhibitionCause.PENDING_DEP,
+                    related_job=job,
+                )
+                suspend_inhibitors.append(inhibitor)
+        return suspend_inhibitors
+
     def observe_result(self, session_state, job, result, fake_resources=False):
         """
         Notice the specified test result and update readiness state.

checkbox-ng/plainbox/impl/depmgr.py

@@ -340,12 +340,12 @@ def _solve(self, visit_list=None):
         if visit_list is None:
             visit_list = self._job_list
         for job in visit_list:
-            self._visit(job)
+            self._visit(job=job, visit_list=visit_list)
         logger.debug(_("Done solving"))
         # Return the solution
         return self._solution
 
-    def _visit(self, job, trail=None):
+    def _visit(self, job, visit_list, trail=None):
         """
         Internal method of DependencySolver.
 
@@ -367,7 +367,9 @@ def _visit(self, job, trail=None):
             # If the trail was not specified start a trail for this node
             if trail is None:
                 trail = [job]
-            for dep_type, job_id in job.controller.get_dependency_set(job):
+            for dep_type, job_id in job.controller.get_dependency_set(
+                job, visit_list
+            ):
                 # Dependency is just an id, we need to resolve it
                 # to a job instance. This can fail (missing dependencies)
                 # so let's guard against that.
@@ -383,7 +385,7 @@ def _visit(self, job, trail=None):
                     logger.debug(_("Visiting dependency: %r"), next_job)
                     # Update the trail as we visit that node
                     trail.append(next_job)
-                    self._visit(next_job, trail)
+                    self._visit(next_job, visit_list, trail)
                     trail.pop()
             # We've visited (recursively) all dependencies of this node,
             # let's color it black and append it to the solution list.

checkbox-ng/plainbox/impl/session/state.py

@@ -41,6 +41,7 @@
 from plainbox.impl.unit.job import JobDefinition
 from plainbox.impl.unit.unit_with_id import UnitWithId
 from plainbox.impl.unit.testplan import TestPlanUnitSupport
+from plainbox.suspend_consts import Suspend
 from plainbox.vendor import morris
 
 
@@ -1091,24 +1092,21 @@ def _add_job_siblings_unit(self, new_job, recompute, via):
                         field_offset_map=new_job.field_offset_map),
                     recompute,
                     via)
-        if 'also-after-suspend' in new_job.get_flag_set():
+        if Suspend.AUTO_FLAG in new_job.get_flag_set():
             data = {
                 key: value for key, value in new_job._data.items()
                 if not key.endswith('siblings')
             }
-            data['flags'] = data['flags'].replace('also-after-suspend', '')
-            data['flags'] = data['flags'].replace(
-                'also-after-suspend-manual', '')
+            data['flags'] = data['flags'].replace(Suspend.AUTO_FLAG, '')
+            data['flags'] = data['flags'].replace(Suspend.MANUAL_FLAG, '')
             data['id'] = "after-suspend-{}".format(new_job.partial_id)
             data['_summary'] = "{} after suspend (S3)".format(
                 new_job.summary)
-            provider_id = "com.canonical.certification"
-            suspend_test_id = "suspend/suspend_advanced_auto"
             if new_job.depends:
                 data['depends'] += " {}".format(new_job.id)
             else:
                 data['depends'] = "{}".format(new_job.id)
-            data['depends'] += " {}::{}".format(provider_id, suspend_test_id)
+            data['depends'] += " {}".format(Suspend.AUTO_JOB_ID)
             self._add_job_unit(
                 JobDefinition(
                     data,
@@ -1119,24 +1117,21 @@ def _add_job_siblings_unit(self, new_job, recompute, via):
                     field_offset_map=new_job.field_offset_map),
                 recompute,
                 via)
-        if 'also-after-suspend-manual' in new_job.get_flag_set():
+        if Suspend.MANUAL_FLAG in new_job.get_flag_set():
             data = {
                 key: value for key, value in new_job._data.items()
                 if not key.endswith('siblings')
             }
-            data['flags'] = data['flags'].replace('also-after-suspend', '')
-            data['flags'] = data['flags'].replace(
-                'also-after-suspend-manual', '')
+            data['flags'] = data['flags'].replace(Suspend.AUTO_FLAG, '')
+            data['flags'] = data['flags'].replace(Suspend.MANUAL_FLAG, '')
             data['id'] = "after-suspend-manual-{}".format(new_job.partial_id)
             data['_summary'] = "{} after suspend (S3)".format(
                 new_job.summary)
-            provider_id = "com.canonical.certification"
-            suspend_test_id = "suspend/suspend_advanced"
             if new_job.depends:
                 data['depends'] += " {}".format(new_job.id)
             else:
                 data['depends'] = "{}".format(new_job.id)
-            data['depends'] += " {}::{}".format(provider_id, suspend_test_id)
+            data['depends'] += " {}".format(Suspend.MANUAL_JOB_ID)
             self._add_job_unit(
                 JobDefinition(
                     data,

checkbox-ng/plainbox/impl/session/test_state.py

@@ -43,8 +43,8 @@
 from plainbox.impl.testing_utils import make_job
 from plainbox.impl.unit.job import JobDefinition
 from plainbox.impl.unit.category import CategoryUnit
-
 from plainbox.impl.unit.unit_with_id import UnitWithId
+from plainbox.suspend_consts import Suspend
 from plainbox.vendor import mock
 from plainbox.vendor.morris import SignalTestCase
 
@@ -309,7 +309,7 @@ def test_add_sibling_unit(self):
 
     def test_also_after_suspend_flag(self):
         # Define a job
-        job = make_job("A", summary="foo", flags="also-after-suspend")
+        job = make_job("A", summary="foo", flags=Suspend.AUTO_FLAG)
         # Define an empty session
         session = SessionState([])
         # Add the job to the session
@@ -319,12 +319,13 @@ def test_also_after_suspend_flag(self):
         self.assertIn(job, session.job_list)
         self.assertEqual(session.job_list[1].id, "after-suspend-A")
         self.assertEqual(session.job_list[1].summary, "foo after suspend (S3)")
+        expected_depends = "A {}".format(Suspend.AUTO_JOB_ID)
         self.assertEqual(
             session.job_list[1].depends,
-            ("A com.canonical.certification::suspend/suspend_advanced_auto"),
+            (expected_depends),
         )
         sibling = session.job_list[1]
-        self.assertNotIn("also-after-suspend", sibling.get_flag_set())
+        self.assertNotIn(Suspend.AUTO_FLAG, sibling.get_flag_set())
         # Both jobs got added to job state map
         self.assertIs(session.job_state_map[job.id].job, job)
         self.assertIs(session.job_state_map[sibling.id].job, sibling)
@@ -346,7 +347,7 @@ def test_also_after_suspend_flag(self):
 
     def test_also_after_suspend_manual_flag(self):
         # Define a job
-        job = make_job("A", summary="foo", flags="also-after-suspend-manual")
+        job = make_job("A", summary="foo", flags=Suspend.MANUAL_FLAG)
         # Define an empty session
         session = SessionState([])
         # Add the job to the session
@@ -356,12 +357,13 @@ def test_also_after_suspend_manual_flag(self):
         self.assertIn(job, session.job_list)
         self.assertEqual(session.job_list[1].id, "after-suspend-manual-A")
         self.assertEqual(session.job_list[1].summary, "foo after suspend (S3)")
+        expected_depends = "A {}".format(Suspend.MANUAL_JOB_ID)
         self.assertEqual(
             session.job_list[1].depends,
-            "A com.canonical.certification::suspend/suspend_advanced",
+            expected_depends,
         )
         sibling = session.job_list[1]
-        self.assertNotIn("also-after-suspend-manual", sibling.get_flag_set())
+        self.assertNotIn(Suspend.MANUAL_FLAG, sibling.get_flag_set())
         # Both jobs got added to job state map
         self.assertIs(session.job_state_map[job.id].job, job)
         self.assertIs(session.job_state_map[sibling.id].job, sibling)
@@ -1215,4 +1217,4 @@ def test_on_job_removed__via_state(self):
         sig1 = self.assertSignalFired(self.ctx.on_unit_removed, self.job)
         sig2 = self.assertSignalFired(self.ctx.state.on_unit_removed, self.job)
         sig3 = self.assertSignalFired(self.ctx.state.on_job_removed, self.job)
-        self.assertSignalOrdering(sig1, sig2, sig3)
+        self.assertSignalOrdering(sig1, sig2, sig3)