diff --git a/checkbox-ng/plainbox/impl/ctrl.py b/checkbox-ng/plainbox/impl/ctrl.py index 03aee471b..b02dc7e7d 100644 --- a/checkbox-ng/plainbox/impl/ctrl.py +++ b/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. diff --git a/checkbox-ng/plainbox/impl/depmgr.py b/checkbox-ng/plainbox/impl/depmgr.py index f91bc2f5b..d0e9c7c07 100644 --- a/checkbox-ng/plainbox/impl/depmgr.py +++ b/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. diff --git a/checkbox-ng/plainbox/impl/session/state.py b/checkbox-ng/plainbox/impl/session/state.py index ef10d7c4b..46eb54fbc 100644 --- a/checkbox-ng/plainbox/impl/session/state.py +++ b/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, diff --git a/checkbox-ng/plainbox/impl/session/test_state.py b/checkbox-ng/plainbox/impl/session/test_state.py index 2adafd976..7f1dec63f 100644 --- a/checkbox-ng/plainbox/impl/session/test_state.py +++ b/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) \ No newline at end of file + self.assertSignalOrdering(sig1, sig2, sig3) diff --git a/checkbox-ng/plainbox/impl/test_ctrl.py b/checkbox-ng/plainbox/impl/test_ctrl.py index fa31d835d..c032dd5e5 100644 --- a/checkbox-ng/plainbox/impl/test_ctrl.py +++ b/checkbox-ng/plainbox/impl/test_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 @@ -26,12 +25,14 @@ from subprocess import CalledProcessError from unittest import TestCase +import json import os import shutil from plainbox.abc import IJobResult from plainbox.abc import IProvider1 from plainbox.abc import IProviderBackend1 +from plainbox.suspend_consts import Suspend from plainbox.impl.ctrl import CheckBoxSessionStateController from plainbox.impl.ctrl import SymLinkNest from plainbox.impl.ctrl import gen_rfc822_records_from_io_log @@ -102,6 +103,19 @@ def test_get_dependency_set(self): self.assertEqual( self.ctrl.get_dependency_set(job_f), {('direct', 'j6'), ('resource', 'j6')}) + # Job with an "also-after-suspend" flag, meaning this job should be + # set to run before the suspend job + job_g = JobDefinition({ + "id": "j7", + "flags": Suspend.AUTO_FLAG + }) + suspend_job = JobDefinition({ + "id": Suspend.AUTO_JOB_ID + }) + self.assertEqual( + self.ctrl.get_dependency_set(suspend_job, [job_g]), + {("ordering", "j7")} + ) def test_get_inhibitor_list_PENDING_RESOURCE(self): # verify that jobs that require a resource that hasn't been @@ -227,6 +241,30 @@ def test_get_inhibitor_list_FAILED_DEP(self): [JobReadinessInhibitor( InhibitionCause.FAILED_DEP, j2, None)]) + def test_get_inhibitor_list_NOT_FAILED_DEP(self): + # verify that jobs that depend on another job that ran but + # didn't result in OUTCOME_FAIL produce the NOT_FAILED_DEP + # inhibitor. + j1 = JobDefinition({ + 'id': 'j1', + 'salvages': 'j2', + }) + j2 = JobDefinition({ + 'id': 'j2' + }) + session_state = mock.MagicMock(spec=SessionState) + session_state.job_state_map = { + 'j1': mock.Mock(spec_set=JobState), + 'j2': mock.Mock(spec_set=JobState), + } + jsm_j2 = session_state.job_state_map['j2'] + jsm_j2.job = j2 + jsm_j2.result.outcome = IJobResult.OUTCOME_NONE + self.assertEqual( + self.ctrl.get_inhibitor_list(session_state, j1), + [JobReadinessInhibitor( + InhibitionCause.NOT_FAILED_DEP, j2, None)]) + def test_get_inhibitor_list_good_dep(self): # verify that jobs that depend on another job that ran and has outcome # equal to OUTCOME_PASS don't have any inhibitors @@ -256,6 +294,79 @@ def test_get_inhibitor_list_good_dep(self): self.assertEqual( self.ctrl.get_inhibitor_list(session_state, j1), []) + def test_get_inhibitor_list__suspend_job(self): + j1 = JobDefinition({ + "id": "j1", + "flags": Suspend.AUTO_FLAG, + }) + j2 = JobDefinition({ + "id": "j2", + }) + suspend_job = JobDefinition({ + "id": Suspend.AUTO_JOB_ID + }) + session_state = mock.MagicMock(spec=SessionState) + session_state.job_state_map = { + "j1": mock.Mock(spec_set=JobState), + "j2": mock.Mock(spec_set=JobState), + Suspend.AUTO_JOB_ID: mock.Mock(spec_set=JobState), + } + jsm_j1 = session_state.job_state_map["j1"] + jsm_j1.job = j1 + jsm_j1.result.outcome = IJobResult.OUTCOME_NONE + jsm_j1.readiness_inhibitor_list = [] + jsm_j2 = session_state.job_state_map["j2"] + jsm_j2.job = j2 + jsm_j2.result.outcome = IJobResult.OUTCOME_NONE + jsm_j2.readiness_inhibitor_list = [] + jsm_suspend = session_state.job_state_map[Suspend.AUTO_JOB_ID] + jsm_suspend.job = suspend_job + jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE + jsm_suspend.readiness_inhibitor_list = [] + self.assertEqual( + self.ctrl.get_inhibitor_list(session_state, suspend_job), + [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)]) + + def test_is_job_impacting_suspend__wrong_suspend_job(self): + job = JobDefinition({ + "id": "job", + }) + self.assertEqual( + self.ctrl._is_job_impacting_suspend("wrong-suspend-job-id", job), + False + ) + + def test_is_job_impacting_suspend__flag(self): + job = JobDefinition({ + "id": "job", + "flags": "also-after-suspend", + }) + self.assertEqual( + self.ctrl._is_job_impacting_suspend(Suspend.AUTO_JOB_ID, job), + True + ) + self.assertEqual( + self.ctrl._is_job_impacting_suspend(Suspend.MANUAL_JOB_ID, job), + False + ) + + def test_is_job_impacting_suspend__siblings(self): + job = JobDefinition({ + "id": "job", + "siblings": json.dumps([{ + "id": "sibling-j1", + "depends": Suspend.MANUAL_JOB_ID, + }]) + }) + self.assertEqual( + self.ctrl._is_job_impacting_suspend(Suspend.AUTO_JOB_ID, job), + False + ) + self.assertEqual( + self.ctrl._is_job_impacting_suspend(Suspend.MANUAL_JOB_ID, job), + True + ) + def test_observe_result__normal(self): job = mock.Mock(spec=JobDefinition) result = mock.Mock(spec=IJobResult) diff --git a/checkbox-ng/plainbox/suspend_consts.py b/checkbox-ng/plainbox/suspend_consts.py new file mode 100644 index 000000000..0424da00f --- /dev/null +++ b/checkbox-ng/plainbox/suspend_consts.py @@ -0,0 +1,29 @@ +# This file is part of Checkbox. +# +# Copyright 2024 Canonical Ltd. +# Written by: +# Pierre Equoy +# +# 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 +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Checkbox. If not, see . + +""" +:mod:`plainbox.impl.suspend_consts` -- Suspend-related constants +================================================================ +""" + + +class Suspend: + AUTO_JOB_ID = "com.canonical.certification::suspend/suspend_advanced_auto" + MANUAL_JOB_ID = "com.canonical.certification::suspend/suspend_advanced" + AUTO_FLAG = "also-after-suspend" + MANUAL_FLAG = "also-after-suspend-manual" diff --git a/docs/reference/units/job.rst b/docs/reference/units/job.rst index e57687350..f8a0e7fcb 100644 --- a/docs/reference/units/job.rst +++ b/docs/reference/units/job.rst @@ -315,11 +315,17 @@ Following fields may be used by the job unit: .. _also-after-suspend flag: - ``also-after-suspend``: See :ref:`Job siblings field` below. + ``also-after-suspend``: + Ensure the test will be run before **and** after suspend by creating + a :ref:`sibling` that will depend on the automated + suspend job. The current job is guaranteed to run before suspend. .. _also-after-suspend-manual flag: - ``also-after-suspend-manual``: See :ref:`Job siblings field` below. + ``also-after-suspend-manual``: + Ensure the test will be run before **and** after suspend by creating + a :ref:`sibling` that will depend on the manual + suspend job. The current job is guaranteed to run before suspend. Additional flags may be present in job definition; they are ignored. @@ -399,16 +405,20 @@ Following fields may be used by the job unit: com.canonical.certification::suspend/suspend_advanced foo -.. warning:: - The curly braces used in this field have to be escaped when used in a - template job (python format, Jinja2 templates do not have this issue). - The syntax for templates is:: + .. note:: + If the sibling definition depends on one of the suspend jobs, Checkbox + will make sure the original job runs **before** the suspend job. - _siblings: [ - {{ "id": "bar-after-suspend_{interface}", - "_summary": "bar after suspend", - "depends": "suspend/advanced"}} - ] + .. warning:: + The curly braces used in this field have to be escaped when used in a + template job (python format, Jinja2 templates do not have this issue). + The syntax for templates is:: + + _siblings: [ + {{ "id": "bar-after-suspend_{interface}", + "_summary": "bar after suspend", + "depends": "suspend/advanced"}} + ] .. _Job imports field: