Ensure that jobs that have an "after suspend" counterpart are run bef…

…ore suspend (New) (canonical#1037)

Some jobs should be run before *and* after suspend. To take this into
account, the `siblings` field was developed[1], along with the
`also-after-suspend` and `also-after-suspend-manual` flags[2].

These would let Checkbox spawn a similar job with an added dependency on
the suspend job (either the manual or the automated version of it).

The problem is that the original job did not have any dependency to
force it to be run *before* the suspend job.

This was not an issue for test plans organized manually, using regular
expressions, because you could have an include section that looks like:

    storage_.*
    com.canonical.certification::suspend/suspend_advanced_auto
    after-suspend-storage.*

However, now that template ids can be added in test plans[3], this is a
problem.

This patch will make sure the jobs that need to run before suspend are
added as dependencies of their related suspend job, so that regardless
of the order in the test plan, they will be run in the proper order.

Documentation is updated to mention guarantee of job running before
suspend in Job Unit reference page.

Fix: canonical#1010

[1]
https://checkbox.readthedocs.io/en/stable/reference/units/job.html#job-siblings-field
[2]
https://checkbox.readthedocs.io/en/stable/reference/units/job.html#also-after-suspend-flag
[3]
https://checkbox.readthedocs.io/en/latest/reference/units/test-plan.html

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)