From 030c69ea1bdd8add7b4b93f1586b830f5a8c3abc Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 22 Nov 2023 22:25:03 +0100 Subject: [PATCH 01/33] add mermaid flowchart of agent boot --- .../plainbox/impl/session/agent_bootup.md | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 checkbox-ng/plainbox/impl/session/agent_bootup.md diff --git a/checkbox-ng/plainbox/impl/session/agent_bootup.md b/checkbox-ng/plainbox/impl/session/agent_bootup.md new file mode 100644 index 000000000..5e32afb17 --- /dev/null +++ b/checkbox-ng/plainbox/impl/session/agent_bootup.md @@ -0,0 +1,34 @@ +# Checkbox Agent bootup process + + +```mermaid +graph TD; + proc("agent process starts") + load("load previous session") + resume("resume the previous session") + resume_crashed("resume the previous session") + auto_session{"was the previous session non-interactive"} + mark_pass("mark last running job as passing") + mark_crash("mark last running job as crashing") + idle("go into idle state") + listen("listen for the controllers") + proc --> load + last_job{"was the last job a `noreturn` job?"} + load -->last_job + last_job-->|yes| resume + resume --> mark_pass + + last_job-->|no| auto_session + auto_session-->|no| idle + idle --> listen + mark_pass --> listen + + auto_session-->|yes| resume_crashed + resume_crashed --> mark_crash + mark_crash --> listen + + + + +``` + From 4e3e2a30ed67400c79813e932c49a4f082444219 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 1 Dec 2023 15:05:38 +0100 Subject: [PATCH 02/33] always resume automated sessions via remote --- checkbox-ng/checkbox_ng/launcher/agent.py | 121 +++++++++++------- .../checkbox_ng/launcher/checkbox_cli.py | 9 +- .../checkbox_ng/launcher/test_agent.py | 25 ++++ .../plainbox/impl/session/assistant.py | 9 +- .../plainbox/impl/session/remote_assistant.py | 18 ++- .../plainbox/impl/session/test_assistant.py | 47 ++++--- 6 files changed, 148 insertions(+), 81 deletions(-) diff --git a/checkbox-ng/checkbox_ng/launcher/agent.py b/checkbox-ng/checkbox_ng/launcher/agent.py index 4ef3e055c..f515c4e9d 100644 --- a/checkbox-ng/checkbox_ng/launcher/agent.py +++ b/checkbox-ng/checkbox_ng/launcher/agent.py @@ -1,8 +1,7 @@ # This file is part of Checkbox. # -# Copyright 2017-2019 Canonical Ltd. +# Copyright 2017-2023 Canonical Ltd. # Written by: -# Maciej Kisielewski # # Checkbox is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License version 3, @@ -20,11 +19,15 @@ functionality. """ import gettext +import json import logging import os import socket import sys +from checkbox_ng import app_context +from plainbox.impl.config import Configuration from plainbox.impl.secure.sudo_broker import is_passwordless_sudo +from plainbox.impl.session.assistant import ResumeCandidate from plainbox.impl.session.remote_assistant import RemoteSessionAssistant from plainbox.impl.session.restart import RemoteDebRestartStrategy from plainbox.impl.session.restart import RemoteSnappyRestartStrategy @@ -36,7 +39,6 @@ class SessionAssistantAgent(rpyc.Service): - session_assistant = None controlling_controller_conn = None controller_blaster = None @@ -58,10 +60,14 @@ def exposed_register_controller_blaster(self, callable): def on_connect(self, conn): try: if SessionAssistantAgent.controller_blaster: - msg = 'Forcefully disconnected by new controller from {}:{}'.format( - conn._config['endpoints'][1][0], conn._config['endpoints'][1][1]) + msg = "Forcefully disconnected by new controller from {}:{}".format( + conn._config["endpoints"][1][0], + conn._config["endpoints"][1][1], + ) SessionAssistantAgent.controller_blaster(msg) - old_controller = SessionAssistantAgent.controlling_controller_conn + old_controller = ( + SessionAssistantAgent.controlling_controller_conn + ) if old_controller is not None: old_controller.close() SessionAssistantAgent.controller_blaster = None @@ -82,7 +88,7 @@ def on_disconnect(self, conn): self.controlling_controller_conn = None -class RemoteAgent(): +class RemoteAgent: """ Run checkbox instance as a agent @@ -91,14 +97,23 @@ class RemoteAgent(): part should be run on system-under-test. """ - name = 'agent' + name = "agent" def invoked(self, ctx): if os.geteuid(): raise SystemExit(_("Checkbox agent must be run by root!")) if not is_passwordless_sudo(): raise SystemExit( - _("System is not configured to run sudo without a password!")) + _("System is not configured to run sudo without a password!") + ) + if ctx.args.resume: + msg = ( + "--resume is deprecated and will be removed soon. " + "Automated sessions are now always resumed. " + "Manual sessions can be resumed from the welcome screen." + ) + _logger.warning(msg) + agent_port = ctx.args.port # Check if able to connect to the agent port as indicator of there @@ -106,45 +121,35 @@ def invoked(self, ctx): def agent_port_open(): sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) sock.settimeout(0.5) - result = sock.connect_ex(('127.0.0.1', agent_port)) + result = sock.connect_ex(("127.0.0.1", agent_port)) sock.close() return result + if agent_port_open() == 0: - raise SystemExit(_("Found port {} is open. Is Checkbox agent" - " already running?").format(agent_port)) + raise SystemExit( + _( + "Found port {} is open. Is Checkbox agent" + " already running?" + ).format(agent_port) + ) SessionAssistantAgent.session_assistant = RemoteSessionAssistant( - lambda s: [sys.argv[0] + 'agent']) - snap_data = os.getenv('SNAP_DATA') - snap_rev = os.getenv('SNAP_REVISION') - remote_restart_strategy_debug = os.getenv('REMOTE_RESTART_DEBUG') - if (snap_data and snap_rev) or ctx.args.resume: - if remote_restart_strategy_debug: - strategy = RemoteSnappyRestartStrategy(debug=True) - else: - strategy = RemoteSnappyRestartStrategy() - if os.path.exists(strategy.session_resume_filename): - with open(strategy.session_resume_filename, 'rt') as f: - session_id = f.readline() - SessionAssistantAgent.session_assistant.resume_by_id( - session_id) - elif ctx.args.resume: - # XXX: explicitly passing None to not have to bump Remote API - # TODO: remove on the next Remote API bump - SessionAssistantAgent.session_assistant.resume_by_id(None) - else: - _logger.info("RemoteDebRestartStrategy") - if remote_restart_strategy_debug: - strategy = RemoteDebRestartStrategy(debug=True) - else: - strategy = RemoteDebRestartStrategy() - if os.path.exists(strategy.session_resume_filename): - with open(strategy.session_resume_filename, 'rt') as f: - session_id = f.readline() - _logger.info( - "RemoteDebRestartStrategy resume_by_id %r", session_id) + lambda s: [sys.argv[0] + "agent"] + ) + + # the agent is meant to be run only as a service, + # and we always resume if the session was automated, + # so we don't need to encode check whether we should resume + + sessions = list(ctx.sa.get_resumable_sessions()) + if sessions: + # the sessions are ordered by time, so the first one is the most + # recent one + if is_the_session_noninteractive(sessions[0]): SessionAssistantAgent.session_assistant.resume_by_id( - session_id) + sessions[0].id + ) + self._server = ThreadedServer( SessionAssistantAgent, port=agent_port, @@ -152,15 +157,35 @@ def agent_port_open(): "allow_all_attrs": True, "allow_setattr": True, "sync_request_timeout": 1, - "propagate_SystemExit_locally": True + "propagate_SystemExit_locally": True, }, ) SessionAssistantAgent.session_assistant.terminate_cb = ( - self._server.close) + self._server.close + ) self._server.start() def register_arguments(self, parser): - parser.add_argument('--resume', action='store_true', help=_( - "resume last session")) - parser.add_argument('--port', type=int, default=18871, help=_( - "port to listen on")) + parser.add_argument( + "--resume", action="store_true", help=_("resume last session") + ) + parser.add_argument( + "--port", type=int, default=18871, help=_("port to listen on") + ) + + +def is_the_session_noninteractive( + resumable_session: "ResumeCandidate", +) -> bool: + """ + Check if given session is non-interactive. + + To determine that we need to take the original launcher that had been used + when the session was started, recreate it as a proper Launcher object, and + check if it's in fact non-interactive. + """ + # app blob is a bytes string with a utf-8 encoded json + # let's decode it and parse it as json + app_blob = json.loads(resumable_session.metadata.app_blob.decode("utf-8")) + launcher = Configuration.from_text(app_blob["launcher"], "resumed session") + return launcher.sections["ui"].get("type") == "silent" diff --git a/checkbox-ng/checkbox_ng/launcher/checkbox_cli.py b/checkbox-ng/checkbox_ng/launcher/checkbox_cli.py index 5b6dc0da3..2c58f8943 100644 --- a/checkbox-ng/checkbox_ng/launcher/checkbox_cli.py +++ b/checkbox-ng/checkbox_ng/launcher/checkbox_cli.py @@ -80,7 +80,7 @@ def main(): "slave": "run-agent", "service": "run-agent", "master": "control", - "remote": "control" + "remote": "control", } known_cmds = list(commands.keys()) @@ -141,12 +141,7 @@ def main(): subcmd = commands[args.subcommand]() subcmd.register_arguments(subcmd_parser) sub_args = subcmd_parser.parse_args(sys.argv[subcmd_index + 1 :]) - sa = SessionAssistant( - "com.canonical:checkbox-cli", - "0.99", - "0.99", - ["restartable"], - ) + sa = SessionAssistant() ctx = Context(sub_args, sa) try: socket.getaddrinfo("localhost", 443) # 443 for HTTPS diff --git a/checkbox-ng/checkbox_ng/launcher/test_agent.py b/checkbox-ng/checkbox_ng/launcher/test_agent.py index 85472661b..5d679922c 100644 --- a/checkbox-ng/checkbox_ng/launcher/test_agent.py +++ b/checkbox-ng/checkbox_ng/launcher/test_agent.py @@ -16,9 +16,13 @@ # You should have received a copy of the GNU General Public License # along with Checkbox. If not, see . +import json from unittest import TestCase, mock +from checkbox_ng.launcher.agent import is_the_session_noninteractive from checkbox_ng.launcher.agent import RemoteAgent +from plainbox.impl.session.assistant import ResumeCandidate +from plainbox.impl.session.state import SessionMetaData class AgentTests(TestCase): @@ -61,3 +65,24 @@ def test_invoked_ok( server = threaded_server_mock.return_value # the server was started self.assertTrue(server.start.called) + + +class IsTheSessionNonInteractiveTests(TestCase): + def test_a_non_interactive_one(self): + a_non_interactive_launcher = """ + [ui] + type = silent + """ + app_blob = json.dumps({"launcher": a_non_interactive_launcher}) + metadata = SessionMetaData(app_blob=app_blob.encode("utf-8")) + + candidate = ResumeCandidate("an_id", metadata) + self.assertTrue(is_the_session_noninteractive(candidate)) + + def test_an_interactive(self): + a_non_interactive_launcher = "" # the defautl one is interactive + app_blob = json.dumps({"launcher": a_non_interactive_launcher}) + metadata = SessionMetaData(app_blob=app_blob.encode("utf-8")) + + candidate = ResumeCandidate("an_id", metadata) + self.assertFalse(is_the_session_noninteractive(candidate)) diff --git a/checkbox-ng/plainbox/impl/session/assistant.py b/checkbox-ng/plainbox/impl/session/assistant.py index 33caae857..698009341 100644 --- a/checkbox-ng/plainbox/impl/session/assistant.py +++ b/checkbox-ng/plainbox/impl/session/assistant.py @@ -34,6 +34,9 @@ import time from tempfile import SpooledTemporaryFile + +from checkbox_ng.app_context import application_name + from plainbox.abc import IJobResult from plainbox.abc import IJobRunnerUI from plainbox.abc import ISessionStateTransport @@ -123,7 +126,11 @@ class SessionAssistant: # TODO: create a flowchart of possible states def __init__( - self, app_id, app_version=None, api_version="0.99", api_flags=() + self, + app_id=application_name(), + app_version=None, + api_version="0.99", + api_flags=(), ): """ Initialize a new session assistant. diff --git a/checkbox-ng/plainbox/impl/session/remote_assistant.py b/checkbox-ng/plainbox/impl/session/remote_assistant.py index 34dd7f51a..2de252f2a 100644 --- a/checkbox-ng/plainbox/impl/session/remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/remote_assistant.py @@ -162,7 +162,7 @@ def __init__(self, cmd_callback): def _reset_sa(self): _logger.info("Resetting RSA") self._state = Idle - self._sa = SessionAssistant("service", api_flags={SA_RESTARTABLE}) + self._sa = SessionAssistant() self._be = None self._session_id = "" self._jobs_count = 0 @@ -292,7 +292,8 @@ def start_session(self, configuration): configuration["launcher"], "Remote launcher" ) self._launcher.update_from_another( - launcher_from_controller, "Remote launcher") + launcher_from_controller, "Remote launcher" + ) session_title = ( self._launcher.get_value("launcher", "session_title") or session_title @@ -326,8 +327,6 @@ def start_session(self, configuration): } ).encode("UTF-8") self._sa.update_app_blob(new_blob) - self._sa.configure_application_restart(self._cmd_callback) - self._session_id = self._sa.get_session_id() tps = self._sa.get_test_plans() filtered_tps = set() @@ -702,7 +701,8 @@ def resume_by_id(self, session_id=None): app_blob["launcher"], "Remote launcher" ) self._launcher.update_from_another( - launcher_from_controller, "Remote launcher") + launcher_from_controller, "Remote launcher" + ) self._sa.use_alternate_configuration(self._launcher) self._normal_user = app_blob.get( @@ -740,6 +740,14 @@ def resume_by_id(self, session_id=None): result_dict["outcome"] = IJobResult.OUTCOME_PASS except json.JSONDecodeError: pass + else: + the_job = self._sa.get_job(self._last_job) + if the_job.plugin == "shell": + if "noreturn" in the_job.get_flag_set(): + result_dict["outcome"] = IJobResult.OUTCOME_PASS + else: + result_dict["outcome"] = IJobResult.OUTCOME_CRASH + result = MemoryJobResult(result_dict) if self._last_job: try: diff --git a/checkbox-ng/plainbox/impl/session/test_assistant.py b/checkbox-ng/plainbox/impl/session/test_assistant.py index 5ae48ccf1..c86c158f3 100644 --- a/checkbox-ng/plainbox/impl/session/test_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_assistant.py @@ -26,35 +26,36 @@ from plainbox.vendor import morris -@mock.patch('plainbox.impl.session.assistant.get_providers') +@mock.patch("plainbox.impl.session.assistant.get_providers") class SessionAssistantTests(morris.SignalTestCase): """Tests for the SessionAssitant class.""" - APP_ID = 'app-id' - APP_VERSION = '1.0' - API_VERSION = '0.99' + APP_ID = "app-id" + APP_VERSION = "1.0" + API_VERSION = "0.99" API_FLAGS = [] def setUp(self): """Common set-up code.""" self.sa = SessionAssistant( - self.APP_ID, self.APP_VERSION, self.API_VERSION, self.API_FLAGS) + self.APP_ID, self.APP_VERSION, self.API_VERSION, self.API_FLAGS + ) # Monitor the provider_selected signal since some tests check it self.watchSignal(self.sa.provider_selected) # Create a few mocked providers that tests can use. # The all-important plainbox provider - self.p1 = mock.Mock(spec_set=Provider1, name='p1') - self.p1.namespace = 'com.canonical.plainbox' - self.p1.name = 'com.canonical.plainbox:special' + self.p1 = mock.Mock(spec_set=Provider1, name="p1") + self.p1.namespace = "com.canonical.plainbox" + self.p1.name = "com.canonical.plainbox:special" # An example 3rd party provider - self.p2 = mock.Mock(spec_set=Provider1, name='p2') - self.p2.namespace = 'pl.zygoon' - self.p2.name = 'pl.zygoon:example' + self.p2 = mock.Mock(spec_set=Provider1, name="p2") + self.p2.namespace = "pl.zygoon" + self.p2.name = "pl.zygoon:example" # A Canonical certification provider - self.p3 = mock.Mock(spec_set=Provider1, name='p3') - self.p3.namespace = 'com.canonical.certification' - self.p3.name = 'com.canonical.certification:stuff' + self.p3 = mock.Mock(spec_set=Provider1, name="p3") + self.p3.namespace = "com.canonical.certification" + self.p3.name = "com.canonical.certification:stuff" def _get_mock_providers(self): """Get some mocked provides for testing.""" @@ -64,16 +65,22 @@ def test_expected_call_sequence(self, mock_get_providers): """Track the sequence of allowed method calls.""" mock_get_providers.return_value = self._get_mock_providers() # SessionAssistant.start_new_session() must now be allowed - self.assertIn(self.sa.start_new_session, - UsageExpectation.of(self.sa).allowed_calls) + self.assertIn( + self.sa.start_new_session, + UsageExpectation.of(self.sa).allowed_calls, + ) # Call SessionAssistant.start_new_session() self.sa.start_new_session("just for testing") # SessionAssistant.start_new_session() must no longer allowed - self.assertNotIn(self.sa.start_new_session, - UsageExpectation.of(self.sa).allowed_calls) + self.assertNotIn( + self.sa.start_new_session, + UsageExpectation.of(self.sa).allowed_calls, + ) # SessionAssistant.select_test_plan() must now be allowed - self.assertIn(self.sa.select_test_plan, - UsageExpectation.of(self.sa).allowed_calls) + self.assertIn( + self.sa.select_test_plan, + UsageExpectation.of(self.sa).allowed_calls, + ) # Use the manager to tidy up after the tests when normally you wouldnt # be allowed to self.sa._manager.destroy() From ff142aeb9a7dad5783690a4ca874d663259f915b Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 1 Dec 2023 15:26:25 +0100 Subject: [PATCH 03/33] fix restart logic requiring SA_RESTARTABLE --- .../plainbox/impl/session/assistant.py | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/checkbox-ng/plainbox/impl/session/assistant.py b/checkbox-ng/plainbox/impl/session/assistant.py index 698009341..6168fa6ea 100644 --- a/checkbox-ng/plainbox/impl/session/assistant.py +++ b/checkbox-ng/plainbox/impl/session/assistant.py @@ -201,18 +201,16 @@ def __init__( self.get_old_sessions: ("get previously created sessions"), self.delete_sessions: ("delete previously created sessions"), self.finalize_session: "to finalize session", + self.configure_application_restart: ( + "configure automatic restart capability" + ), + self.use_alternate_restart_strategy: ( + "configure automatic restart capability" + ), } # Restart support self._restart_cmd_callback = None self._restart_strategy = None # None implies auto-detection - if SA_RESTARTABLE in self._flags: - allowed_calls = UsageExpectation.of(self).allowed_calls - allowed_calls[ - self.configure_application_restart - ] = "configure automatic restart capability" - allowed_calls[ - self.use_alternate_restart_strategy - ] = "configure automatic restart capability" @property def config(self): @@ -519,15 +517,13 @@ def start_new_session( self.get_session_id: "to get the id of currently running session", self.hand_pick_jobs: "select jobs to run (w/o a test plan)", self.finalize_session: "to finalize session", + self.configure_application_restart: ( + "configure automatic restart capability" + ), + self.use_alternate_restart_strategy: ( + "configure automatic restart capability" + ), } - if SA_RESTARTABLE in self._flags: - allowed_calls = UsageExpectation.of(self).allowed_calls - allowed_calls[ - self.configure_application_restart - ] = "configure automatic restart capability" - allowed_calls[ - self.use_alternate_restart_strategy - ] = "configure automatic restart capability" @raises(KeyError, UnexpectedMethodCall) def resume_session( From 3a893e3eb94c736758fa146db1cd1da83de08fd2 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 1 Dec 2023 16:10:29 +0100 Subject: [PATCH 04/33] update the url to the stable PPA --- metabox/metabox/lxd_profiles/checkbox.profile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metabox/metabox/lxd_profiles/checkbox.profile b/metabox/metabox/lxd_profiles/checkbox.profile index bc084f6df..5a6c8373a 100644 --- a/metabox/metabox/lxd_profiles/checkbox.profile +++ b/metabox/metabox/lxd_profiles/checkbox.profile @@ -10,7 +10,7 @@ config: apt: sources: stable_ppa: - source: "ppa:hardware-certification/public" + source: "ppa:checkbox-dev/stable" packages: - alsa-base - jq From 3fb268284bcf5997e2b626e317030f27c3c1bb7e Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Thu, 7 Dec 2023 13:59:49 +0100 Subject: [PATCH 05/33] improve coverage of remote assistant code --- .../plainbox/impl/session/test_assistant.py | 16 ++++ .../impl/session/test_remote_assistant.py | 84 +++++++++++++++++++ .../metabox/metabox-provider/units/resume.pxu | 20 +++++ .../scenarios/restart/agent_respawn.py | 44 ++++++++++ 4 files changed, 164 insertions(+) create mode 100644 metabox/metabox/metabox-provider/units/resume.pxu create mode 100644 metabox/metabox/scenarios/restart/agent_respawn.py diff --git a/checkbox-ng/plainbox/impl/session/test_assistant.py b/checkbox-ng/plainbox/impl/session/test_assistant.py index c86c158f3..657d67873 100644 --- a/checkbox-ng/plainbox/impl/session/test_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_assistant.py @@ -84,3 +84,19 @@ def test_expected_call_sequence(self, mock_get_providers): # Use the manager to tidy up after the tests when normally you wouldnt # be allowed to self.sa._manager.destroy() + + def test_start_new_session(self, mock_get_providers): + """Test that start_new_session() works.""" + mock_get_providers.return_value = self._get_mock_providers() + # SessionAssistant.start_new_session() must now be allowed + self.assertIn( + self.sa.start_new_session, + UsageExpectation.of(self.sa).allowed_calls, + ) + # Call SessionAssistant.start_new_session() + self.sa.start_new_session("just for testing") + # SessionAssistant.start_new_session() must no longer allowed + self.assertNotIn( + self.sa.start_new_session, + UsageExpectation.of(self.sa).allowed_calls, + ) diff --git a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py index f975f9864..d7ef93f5c 100644 --- a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py @@ -19,7 +19,12 @@ from unittest import TestCase, mock from plainbox.abc import IJobResult +from plainbox.impl.config import Configuration + +from plainbox.impl.secure.sudo_broker import is_passwordless_sudo + from plainbox.impl.session import remote_assistant +from plainbox.impl.session.assistant import SessionAssistant class RemoteAssistantTests(TestCase): @@ -46,6 +51,85 @@ def not_allowed(self, *args): with self.assertRaises(AssertionError): not_allowed(self_mock) + @mock.patch("plainbox.impl.secure.sudo_broker.is_passwordless_sudo") + def test__reset_sa(self, is_passwordless_sudo_mock): + with mock.patch.object(SessionAssistant, "__init__") as init_mock: + init_mock.return_value = None + # RSA constructor calls _reset_sa, which in turns creates a new SA + rsa = remote_assistant.RemoteSessionAssistant(lambda: None) + init_mock.assert_called_once() + + @mock.patch("fnmatch.filter") + def test_start_session_with_launcher(self, mock_filter): + # the real tp is referenced later on by it's second field + mock_filter.return_value = [("tp", "tp")] + extra_cfg = dict() + extra_cfg["launcher"] = "test_launcher" + rsa = mock.Mock() + rsa._sa.get_test_plans.return_value = [mock.Mock()] + rsa._state = remote_assistant.Idle + with mock.patch("plainbox.impl.config.Configuration.from_text") as cm: + cm.return_value = Configuration() + tps = remote_assistant.RemoteSessionAssistant.start_session( + rsa, extra_cfg + ) + self.assertEqual(tps[0][0][1], "tp") + + @mock.patch("fnmatch.filter") + def test_start_session_without_launcher(self, mock_filter): + # the real tp is referenced later on by it's second field + mock_filter.return_value = [("tp", "tp")] + extra_cfg = dict() + extra_cfg["launcher"] = "test_launcher" + rsa = mock.Mock() + rsa._sa.get_test_plans.return_value = [mock.Mock()] + rsa._state = remote_assistant.Idle + with mock.patch("plainbox.impl.config.Configuration.from_text") as cm: + cm.return_value = Configuration() + tps = remote_assistant.RemoteSessionAssistant.start_session( + rsa, extra_cfg + ) + self.assertEqual(tps[0][0][1], "tp") + + def test_resume_by_id_with_session_id(self): + rsa = mock.Mock() + resumable_session = mock.Mock() + resumable_session.id = "session_id" + rsa._sa.get_resumable_sessions.return_value = [resumable_session] + rsa._state = remote_assistant.Idle + mock_meta = mock.Mock() + mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}' + + rsa._sa.resume_session.return_value = mock_meta + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa, "session_id") + self.assertEqual(rsa._state, "testsselected") + + def test_resume_by_id_bad_session_id(self): + rsa = mock.Mock() + resumable_session = mock.Mock() + resumable_session.id = "session_id" + rsa._sa.get_resumable_sessions.return_value = [resumable_session] + rsa._state = remote_assistant.Idle + mock_meta = mock.Mock() + mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}' + + rsa._sa.resume_session.return_value = mock_meta + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa, "bad_id") + self.assertEqual(rsa._state, "idle") + + def test_resume_by_id_without_session_id(self): + rsa = mock.Mock() + resumable_session = mock.Mock() + resumable_session.id = "session_id" + rsa._sa.get_resumable_sessions.return_value = [resumable_session] + rsa._state = remote_assistant.Idle + mock_meta = mock.Mock() + mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}' + + rsa._sa.resume_session.return_value = mock_meta + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) + self.assertEqual(rsa._state, "testsselected") + class RemoteAssistantFinishJobTests(TestCase): def setUp(self): diff --git a/metabox/metabox/metabox-provider/units/resume.pxu b/metabox/metabox/metabox-provider/units/resume.pxu new file mode 100644 index 000000000..cdbb6fe0d --- /dev/null +++ b/metabox/metabox/metabox-provider/units/resume.pxu @@ -0,0 +1,20 @@ +id: agent-crasher +_summary: Crash the agent +flags: simple +user: root +command: + kill `ps aux|grep run-agent|grep -v 'grep' | awk '{print $2}'` + +id: reboot-emulator +_summary: Emulate the reboot +flags: simple noreturn +user: root +command: + kill `ps aux|grep run-agent|grep -v 'grep' | awk '{print $2}'` + +unit: test plan +id: agent-resume-crash-then-reboot +_name: Agent resume after crash then reboot +include: + agent-crasher + reboot-emulator diff --git a/metabox/metabox/scenarios/restart/agent_respawn.py b/metabox/metabox/scenarios/restart/agent_respawn.py new file mode 100644 index 000000000..c5529c447 --- /dev/null +++ b/metabox/metabox/scenarios/restart/agent_respawn.py @@ -0,0 +1,44 @@ +# This file is part of Checkbox. +# +# Copyright 2023 Canonical Ltd. +# +# 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 . +import textwrap + +from metabox.core.actions import AssertPrinted, AssertRetCode +from metabox.core.scenario import Scenario +from metabox.core.utils import tag + + +@tag("resume") +class ResumeAfterCrash(Scenario): + modes = ["remote"] + launcher = textwrap.dedent( + """ + [launcher] + launcher_version = 1 + stock_reports = text + [test plan] + unit = 2021.com.canonical.certification::agent-resume-crash-then-reboot + forced = yes + [test selection] + forced = yes + [ui] + type = silent + """ + ) + steps = [ + AssertRetCode(1), + AssertPrinted("job crashed : Crash the agent"), + AssertPrinted("job passed : Emulate the reboot"), + ] From 2097a28dc379a0fcc45f1723f05e0ff1d6db8164 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Thu, 7 Dec 2023 16:16:03 +0100 Subject: [PATCH 06/33] add more coverage to the remote assistant --- .../plainbox/impl/session/remote_assistant.py | 4 +- .../impl/session/test_remote_assistant.py | 95 +++++++++++++++++++ 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/checkbox-ng/plainbox/impl/session/remote_assistant.py b/checkbox-ng/plainbox/impl/session/remote_assistant.py index 2de252f2a..84eecac9f 100644 --- a/checkbox-ng/plainbox/impl/session/remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/remote_assistant.py @@ -1,6 +1,6 @@ # This file is part of Checkbox. # -# Copyright 2018 Canonical Ltd. +# Copyright 2018-2023 Canonical Ltd. # Written by: # Maciej Kisielewski # @@ -719,7 +719,7 @@ def resume_by_id(self, session_id=None): result_dict = { "outcome": IJobResult.OUTCOME_PASS, - "comments": _("Automatically passed after resuming execution"), + "comments": "Automatically passed after resuming execution", } session_share = WellKnownDirsHelper.session_share( self._sa._manager.storage.id diff --git a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py index d7ef93f5c..c0fface16 100644 --- a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py @@ -3,6 +3,7 @@ # Copyright 2023 Canonical Ltd. # Written by: # Massimiliano Girardi +# Maciej Kisielewski # # Checkbox is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License version 3, @@ -16,10 +17,15 @@ # You should have received a copy of the GNU General Public License # along with Checkbox. If not, see . +from os.path import exists + from unittest import TestCase, mock +from checkbox_ng.config import load_configs + from plainbox.abc import IJobResult from plainbox.impl.config import Configuration +from plainbox.impl.result import MemoryJobResult from plainbox.impl.secure.sudo_broker import is_passwordless_sudo @@ -130,6 +136,95 @@ def test_resume_by_id_without_session_id(self): remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) self.assertEqual(rsa._state, "testsselected") + @mock.patch("plainbox.impl.session.remote_assistant.load_configs") + def test_resume_by_id_with_result_file_ok(self, mock_load_configs): + rsa = mock.Mock() + resumable_session = mock.Mock() + resumable_session.id = "session_id" + rsa._sa.get_resumable_sessions.return_value = [resumable_session] + rsa.get_rerun_candidates.return_value = [] + rsa._state = remote_assistant.Idle + + mock_meta = mock.Mock() + mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}' + + rsa._sa.resume_session.return_value = mock_meta + os_path_exists_mock = mock.Mock() + + with mock.patch("os.path.exists", os_path_exists_mock): + with mock.patch("builtins.open", mock.mock_open(read_data="pass")): + os_path_exists_mock.return_value = True + # mock_open.return_value.read.return_value = "pass" + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) + + mjr = MemoryJobResult( + { + "outcome": IJobResult.OUTCOME_PASS, + "comments": "Automatically passed after resuming execution", + } + ) + + rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True) + + @mock.patch("plainbox.impl.session.remote_assistant.load_configs") + def test_resume_by_id_with_result_no_file(self, mock_load_configs): + rsa = mock.Mock() + resumable_session = mock.Mock() + resumable_session.id = "session_id" + rsa._sa.get_resumable_sessions.return_value = [resumable_session] + rsa.get_rerun_candidates.return_value = [] + rsa._state = remote_assistant.Idle + + mock_meta = mock.Mock() + mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}' + + rsa._sa.resume_session.return_value = mock_meta + os_path_exists_mock = mock.Mock() + + with mock.patch("os.path.exists", os_path_exists_mock): + os_path_exists_mock.return_value = False + # mock_open.return_value.read.return_value = "pass" + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) + + mjr = MemoryJobResult( + { + "outcome": IJobResult.OUTCOME_PASS, + "comments": "Automatically passed after resuming execution", + } + ) + + rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True) + + @mock.patch("plainbox.impl.session.remote_assistant.load_configs") + def test_resume_by_id_with_result_file_not_json(self, mock_load_configs): + rsa = mock.Mock() + resumable_session = mock.Mock() + resumable_session.id = "session_id" + rsa._sa.get_resumable_sessions.return_value = [resumable_session] + rsa.get_rerun_candidates.return_value = [] + rsa._state = remote_assistant.Idle + + mock_meta = mock.Mock() + mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}' + + rsa._sa.resume_session.return_value = mock_meta + os_path_exists_mock = mock.Mock() + + with mock.patch("os.path.exists", os_path_exists_mock): + with mock.patch("builtins.open", mock.mock_open(read_data="!@!")): + os_path_exists_mock.return_value = True + # mock_open.return_value.read.return_value = "pass" + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) + + mjr = MemoryJobResult( + { + "outcome": IJobResult.OUTCOME_PASS, + "comments": "Automatically passed after resuming execution", + } + ) + + rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True) + class RemoteAssistantFinishJobTests(TestCase): def setUp(self): From c9a362605715c394204efa514611f44b89c74dfb Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Thu, 7 Dec 2023 17:24:50 +0100 Subject: [PATCH 07/33] py35 friendly assertion --- checkbox-ng/plainbox/impl/session/test_remote_assistant.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py index c0fface16..4eb9a535d 100644 --- a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py @@ -63,7 +63,7 @@ def test__reset_sa(self, is_passwordless_sudo_mock): init_mock.return_value = None # RSA constructor calls _reset_sa, which in turns creates a new SA rsa = remote_assistant.RemoteSessionAssistant(lambda: None) - init_mock.assert_called_once() + self.assertEqual(init_mock.call_count, 1) @mock.patch("fnmatch.filter") def test_start_session_with_launcher(self, mock_filter): From 1e803c7e66ffac372845978a342695cb20464ee4 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Thu, 7 Dec 2023 17:49:46 +0100 Subject: [PATCH 08/33] add coverage for sa.config property --- checkbox-ng/plainbox/impl/session/test_assistant.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/checkbox-ng/plainbox/impl/session/test_assistant.py b/checkbox-ng/plainbox/impl/session/test_assistant.py index 657d67873..43cabad24 100644 --- a/checkbox-ng/plainbox/impl/session/test_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_assistant.py @@ -1,13 +1,13 @@ # This file is part of Checkbox. # -# Copyright 2015 Canonical Ltd. +# Copyright 2015-2023 Canonical Ltd. # Written by: # Zygmunt Krynicki +# Maciej Kisielewski # # 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 @@ -19,6 +19,7 @@ """Tests for the session assistant module class.""" +from plainbox.impl.config import Configuration from plainbox.impl.secure.providers.v1 import Provider1 from plainbox.impl.session.assistant import SessionAssistant from plainbox.impl.session.assistant import UsageExpectation @@ -100,3 +101,6 @@ def test_start_new_session(self, mock_get_providers): self.sa.start_new_session, UsageExpectation.of(self.sa).allowed_calls, ) + + def test_config_property(self, mock_get_providers): + self.assertEqual(self.sa.config, self.sa._config) From bb434156a2578a4eea3b0124b87360451b9b047a Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 8 Dec 2023 14:56:27 +0100 Subject: [PATCH 09/33] outline checking for open port --- checkbox-ng/checkbox_ng/launcher/agent.py | 38 +++++++++++-------- .../checkbox_ng/launcher/test_agent.py | 18 +++++++++ 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/checkbox-ng/checkbox_ng/launcher/agent.py b/checkbox-ng/checkbox_ng/launcher/agent.py index f515c4e9d..46cad15e1 100644 --- a/checkbox-ng/checkbox_ng/launcher/agent.py +++ b/checkbox-ng/checkbox_ng/launcher/agent.py @@ -116,22 +116,7 @@ def invoked(self, ctx): agent_port = ctx.args.port - # Check if able to connect to the agent port as indicator of there - # already being a agent running - def agent_port_open(): - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.settimeout(0.5) - result = sock.connect_ex(("127.0.0.1", agent_port)) - sock.close() - return result - - if agent_port_open() == 0: - raise SystemExit( - _( - "Found port {} is open. Is Checkbox agent" - " already running?" - ).format(agent_port) - ) + exit_if_port_unavailable(agent_port) SessionAssistantAgent.session_assistant = RemoteSessionAssistant( lambda s: [sys.argv[0] + "agent"] @@ -189,3 +174,24 @@ def is_the_session_noninteractive( app_blob = json.loads(resumable_session.metadata.app_blob.decode("utf-8")) launcher = Configuration.from_text(app_blob["launcher"], "resumed session") return launcher.sections["ui"].get("type") == "silent" + + +def exit_if_port_unavailable(port: int) -> None: + """ + Check if the port is available and exit if it's not. + + This is used by the agent to check if it's already running. + """ + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.settimeout(0.5) + result = sock.connect_ex(("127.0.0.1", port)) + sock.close() + # the result is 0 if the port is open (which means the low level + # connect() call succeeded), and 1 if it's closed (which means the low + # level connect() call failed) + if result == 0: + raise SystemExit( + _( + "Found port {} is open. Is Checkbox agent" " already running?" + ).format(port) + ) diff --git a/checkbox-ng/checkbox_ng/launcher/test_agent.py b/checkbox-ng/checkbox_ng/launcher/test_agent.py index 5d679922c..7056f638d 100644 --- a/checkbox-ng/checkbox_ng/launcher/test_agent.py +++ b/checkbox-ng/checkbox_ng/launcher/test_agent.py @@ -20,6 +20,7 @@ from unittest import TestCase, mock from checkbox_ng.launcher.agent import is_the_session_noninteractive +from checkbox_ng.launcher.agent import exit_if_port_unavailable from checkbox_ng.launcher.agent import RemoteAgent from plainbox.impl.session.assistant import ResumeCandidate from plainbox.impl.session.state import SessionMetaData @@ -86,3 +87,20 @@ def test_an_interactive(self): candidate = ResumeCandidate("an_id", metadata) self.assertFalse(is_the_session_noninteractive(candidate)) + + +class ExitIfPortUnavilableTests(TestCase): + def test_exit_if_port_unavailable_it_is(self): + mocked_socket = mock.Mock() + mocked_socket.connect_ex.return_value = 1 + with mock.patch("socket.socket") as socket_mock: + socket_mock.return_value = mocked_socket + self.assertIsNone(exit_if_port_unavailable(1234)) + + def test_exit_if_port_unavailable_nope(self): + mocked_socket = mock.Mock() + mocked_socket.connect_ex.return_value = 0 + with mock.patch("socket.socket") as socket_mock: + socket_mock.return_value = mocked_socket + with self.assertRaises(SystemExit): + exit_if_port_unavailable(1234) From 0e57c83ed48febc516bb44b9b9788718e1d8efbb Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 8 Dec 2023 15:03:11 +0100 Subject: [PATCH 10/33] add tests for registering agents args --- checkbox-ng/checkbox_ng/launcher/test_agent.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/checkbox-ng/checkbox_ng/launcher/test_agent.py b/checkbox-ng/checkbox_ng/launcher/test_agent.py index 7056f638d..358a7d4b3 100644 --- a/checkbox-ng/checkbox_ng/launcher/test_agent.py +++ b/checkbox-ng/checkbox_ng/launcher/test_agent.py @@ -104,3 +104,18 @@ def test_exit_if_port_unavailable_nope(self): socket_mock.return_value = mocked_socket with self.assertRaises(SystemExit): exit_if_port_unavailable(1234) + + +class RegisterArgumentsTests(TestCase): + def test_register_arguments(self): + parser = mock.Mock() + rsa = mock.Mock() + with mock.patch("checkbox_ng.launcher.agent._") as _mock: + _mock.return_value = "_" + RemoteAgent.register_arguments(rsa, parser) + parser.add_argument.assert_any_call( + "--resume", action="store_true", help="_" + ) + parser.add_argument.assert_any_call( + "--port", type=int, default=18871, help="_" + ) From 23d6751cfb8a6ae19acd2708f478e0325a2d5268 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 8 Dec 2023 15:21:47 +0100 Subject: [PATCH 11/33] more agent unit tests --- .../checkbox_ng/launcher/test_agent.py | 92 +++++++++++++++++-- 1 file changed, 83 insertions(+), 9 deletions(-) diff --git a/checkbox-ng/checkbox_ng/launcher/test_agent.py b/checkbox-ng/checkbox_ng/launcher/test_agent.py index 358a7d4b3..34c331e4d 100644 --- a/checkbox-ng/checkbox_ng/launcher/test_agent.py +++ b/checkbox-ng/checkbox_ng/launcher/test_agent.py @@ -26,16 +26,16 @@ from plainbox.impl.session.state import SessionMetaData +@mock.patch("checkbox_ng.launcher.agent._") +@mock.patch("checkbox_ng.launcher.agent._logger") +@mock.patch("checkbox_ng.launcher.agent.is_passwordless_sudo") +@mock.patch("checkbox_ng.launcher.agent.SessionAssistantAgent") +@mock.patch("checkbox_ng.launcher.agent.RemoteDebRestartStrategy") +@mock.patch("checkbox_ng.launcher.agent.RemoteSessionAssistant") +@mock.patch("checkbox_ng.launcher.agent.ThreadedServer") +@mock.patch("os.geteuid") +@mock.patch("os.getenv") class AgentTests(TestCase): - @mock.patch("checkbox_ng.launcher.agent._") - @mock.patch("checkbox_ng.launcher.agent._logger") - @mock.patch("checkbox_ng.launcher.agent.is_passwordless_sudo") - @mock.patch("checkbox_ng.launcher.agent.SessionAssistantAgent") - @mock.patch("checkbox_ng.launcher.agent.RemoteDebRestartStrategy") - @mock.patch("checkbox_ng.launcher.agent.RemoteSessionAssistant") - @mock.patch("checkbox_ng.launcher.agent.ThreadedServer") - @mock.patch("os.geteuid") - @mock.patch("os.getenv") # used to load an empty launcher with no error def test_invoked_ok( self, @@ -67,6 +67,80 @@ def test_invoked_ok( # the server was started self.assertTrue(server.start.called) + # used to load an empty launcher with no error + def test_invoked_with_resume_warns( + self, + getenv_mock, + geteuid_mock, + threaded_server_mock, + remote_assistant_mock, + remote_strategy_mock, + session_assistant_mock, + pwdless_sudo_mock, + logger_mock, + gettext_mock, + ): + ctx_mock = mock.MagicMock() + ctx_mock.args.resume = True + + self_mock = mock.MagicMock() + + geteuid_mock.return_value = 0 # agent will not run as non-root + pwdless_sudo_mock.return_value = True # agent needs pwd-less sudo + getenv_mock.return_value = None + + with mock.patch("builtins.open"): + RemoteAgent.invoked(self_mock, ctx_mock) + + # agent server was created + self.assertTrue(threaded_server_mock.called) + server = threaded_server_mock.return_value + # the server was started + self.assertTrue(server.start.called) + logger_mock.warning.assert_called() + + def test_invoked_with_session( + self, + getenv_mock, + geteuid_mock, + threaded_server_mock, + remote_assistant_mock, + remote_strategy_mock, + sa_mock, + pwdless_sudo_mock, + logger_mock, + gettext_mock, + ): + ctx_mock = mock.MagicMock() + ctx_mock.args.resume = False + + self_mock = mock.MagicMock() + + geteuid_mock.return_value = 0 + + ctx_mock.sa.get_resumable_sessions.return_value = [ + ResumeCandidate("an_id", SessionMetaData()) + ] + with mock.patch( + "checkbox_ng.launcher.agent.is_the_session_noninteractive" + ) as itni: + itni.return_value = True + + RemoteAgent.invoked(self_mock, ctx_mock) + remote_assistant_mock.return_value.resume_by_id.assert_called() + + remote_assistant_mock.reset_mock() + + with mock.patch( + "checkbox_ng.launcher.agent.is_the_session_noninteractive" + ) as itni: + itni.return_value = False + + RemoteAgent.invoked(self_mock, ctx_mock) + self.assertFalse( + remote_assistant_mock.return_value.resume_by_id.called + ) + class IsTheSessionNonInteractiveTests(TestCase): def test_a_non_interactive_one(self): From ad13be185b88188224dfca367beb2705886416ec Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 8 Dec 2023 15:27:30 +0100 Subject: [PATCH 12/33] more agent unit tests --- checkbox-ng/checkbox_ng/launcher/test_agent.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/checkbox-ng/checkbox_ng/launcher/test_agent.py b/checkbox-ng/checkbox_ng/launcher/test_agent.py index 34c331e4d..cdf47dbb4 100644 --- a/checkbox-ng/checkbox_ng/launcher/test_agent.py +++ b/checkbox-ng/checkbox_ng/launcher/test_agent.py @@ -97,7 +97,7 @@ def test_invoked_with_resume_warns( server = threaded_server_mock.return_value # the server was started self.assertTrue(server.start.called) - logger_mock.warning.assert_called() + self.assertTrue(logger_mock.warning.called) def test_invoked_with_session( self, @@ -127,7 +127,9 @@ def test_invoked_with_session( itni.return_value = True RemoteAgent.invoked(self_mock, ctx_mock) - remote_assistant_mock.return_value.resume_by_id.assert_called() + self.assertTrue( + remote_assistant_mock.return_value.resume_by_id.called + ) remote_assistant_mock.reset_mock() From f2af9e9aecbdafc01c893bdefc7584eeb7b06a2f Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 8 Dec 2023 15:52:28 +0100 Subject: [PATCH 13/33] add coverage for the job type when resuming --- .../impl/session/test_remote_assistant.py | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py index 4eb9a535d..a1e9a3af2 100644 --- a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py @@ -22,6 +22,7 @@ from unittest import TestCase, mock from checkbox_ng.config import load_configs +from checkbox_ng.launcher.agent import SessionAssistantAgent from plainbox.abc import IJobResult from plainbox.impl.config import Configuration @@ -167,7 +168,9 @@ def test_resume_by_id_with_result_file_ok(self, mock_load_configs): rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True) @mock.patch("plainbox.impl.session.remote_assistant.load_configs") - def test_resume_by_id_with_result_no_file(self, mock_load_configs): + def test_resume_by_id_with_result_no_file_noreturn( + self, mock_load_configs + ): rsa = mock.Mock() resumable_session = mock.Mock() resumable_session.id = "session_id" @@ -181,9 +184,15 @@ def test_resume_by_id_with_result_no_file(self, mock_load_configs): rsa._sa.resume_session.return_value = mock_meta os_path_exists_mock = mock.Mock() + rsa._sa.get_job = mock.Mock() + rsa._sa.get_job.return_value.plugin = "shell" + with mock.patch("os.path.exists", os_path_exists_mock): os_path_exists_mock.return_value = False - # mock_open.return_value.read.return_value = "pass" + rsa._sa.get_job.return_value.get_flag_set.return_value = { + "noreturn" + } + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) mjr = MemoryJobResult( @@ -195,6 +204,39 @@ def test_resume_by_id_with_result_no_file(self, mock_load_configs): rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True) + @mock.patch("plainbox.impl.session.remote_assistant.load_configs") + def test_resume_by_id_with_result_no_file_normal(self, mock_load_configs): + rsa = mock.Mock() + resumable_session = mock.Mock() + resumable_session.id = "session_id" + rsa._sa.get_resumable_sessions.return_value = [resumable_session] + rsa.get_rerun_candidates.return_value = [] + rsa._state = remote_assistant.Idle + + mock_meta = mock.Mock() + mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}' + + rsa._sa.resume_session.return_value = mock_meta + os_path_exists_mock = mock.Mock() + + rsa._sa.get_job = mock.Mock() + rsa._sa.get_job.return_value.plugin = "shell" + + with mock.patch("os.path.exists", os_path_exists_mock): + os_path_exists_mock.return_value = False + rsa._sa.get_job.return_value.get_flag_set.return_value = {} + + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) + + mjr = MemoryJobResult( + { + "outcome": IJobResult.OUTCOME_CRASH, + "comments": "Automatically passed after resuming execution", + } + ) + + rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True) + @mock.patch("plainbox.impl.session.remote_assistant.load_configs") def test_resume_by_id_with_result_file_not_json(self, mock_load_configs): rsa = mock.Mock() From 0fd301a2b53d86568b5e521e774f3ff80b1c3ae6 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Fri, 8 Dec 2023 15:52:53 +0100 Subject: [PATCH 14/33] add tests for SessionAssistantAgent --- .../plainbox/impl/session/test_remote_assistant.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py index a1e9a3af2..b7006d12e 100644 --- a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py @@ -324,3 +324,17 @@ def test_no_result_with_be_but_no_builder(self, MockJobResultBuilder): outcome=IJobResult.OUTCOME_PASS, comments="Automatically passed while resuming", ) + + +class SessionAssistantAgentTests(TestCase): + def test_on_connect(self): + conn = mock.Mock() + conn._config = {"endpoints": [("first", 1), ("second", 2)]} + blaster_mock = mock.Mock() + SessionAssistantAgent.controller_blaster = blaster_mock + SessionAssistantAgent.controlling_controller_conn = mock.Mock() + saa = mock.Mock() + SessionAssistantAgent.on_connect(saa, conn) + blaster_mock.assert_called_with( + "Forcefully disconnected by new controller from second:2" + ) From d658f92158b2a22dcd9ba8c6690f14b8f196c945 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 14:09:53 +0100 Subject: [PATCH 15/33] tweaks after Max's suggestions --- checkbox-ng/checkbox_ng/launcher/agent.py | 6 +++- .../plainbox/impl/session/agent_bootup.md | 16 ++++----- docs/explanation/remote.rst | 36 +++++++++++++++++++ 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/checkbox-ng/checkbox_ng/launcher/agent.py b/checkbox-ng/checkbox_ng/launcher/agent.py index 46cad15e1..6c9efa284 100644 --- a/checkbox-ng/checkbox_ng/launcher/agent.py +++ b/checkbox-ng/checkbox_ng/launcher/agent.py @@ -118,8 +118,12 @@ def invoked(self, ctx): exit_if_port_unavailable(agent_port) + # The RemoteSessionAssistant required a callable that was used to + # start the agent as a part of restart strategy. We don't need that + # functionality, so we just pass a dummy callable that will never be + # called. It's left in to avoid breaking the API. SessionAssistantAgent.session_assistant = RemoteSessionAssistant( - lambda s: [sys.argv[0] + "agent"] + lambda x: None ) # the agent is meant to be run only as a service, diff --git a/checkbox-ng/plainbox/impl/session/agent_bootup.md b/checkbox-ng/plainbox/impl/session/agent_bootup.md index 5e32afb17..61a850b9e 100644 --- a/checkbox-ng/plainbox/impl/session/agent_bootup.md +++ b/checkbox-ng/plainbox/impl/session/agent_bootup.md @@ -1,34 +1,34 @@ # Checkbox Agent bootup process - ```mermaid graph TD; proc("agent process starts") load("load previous session") resume("resume the previous session") resume_crashed("resume the previous session") - auto_session{"was the previous session non-interactive"} + interactive{"was the previous interactive"} mark_pass("mark last running job as passing") mark_crash("mark last running job as crashing") idle("go into idle state") - listen("listen for the controllers") + listen("listen for a controller") proc --> load last_job{"was the last job a `noreturn` job?"} load -->last_job last_job-->|yes| resume resume --> mark_pass - - last_job-->|no| auto_session - auto_session-->|no| idle + + last_job-->|no| interactive + interactive-->|yes| idle idle --> listen mark_pass --> listen - auto_session-->|yes| resume_crashed + interactive-->|no| resume_crashed resume_crashed --> mark_crash mark_crash --> listen - + ``` + diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index d1d84ec75..4ee95f5ae 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -92,6 +92,42 @@ End this test session preserving its data and launch a new one selection screens, in which case a similar test session is immediately started. + +Automatic session resume +======================== + +# Checkbox Agent bootup process + +```mermaid +graph TD; + proc("agent process starts") + load("load previous session") + resume("resume the previous session") + resume_crashed("resume the previous session") + interactive{"was the previous interactive"} + mark_pass("mark last running job as passing") + mark_crash("mark last running job as crashing") + idle("go into idle state") + listen("listen for a controller") + proc --> load + last_job{"was the last job a `noreturn` job?"} + load -->last_job + last_job-->|yes| resume + resume --> mark_pass + + last_job-->|no| interactive + interactive-->|yes| idle + idle --> listen + mark_pass --> listen + + interactive-->|no| resume_crashed + resume_crashed --> mark_crash + mark_crash --> listen +``` + + + + Remote session characteristics ============================== From 9faa950171304d8cb170521efcbf1c8f8d6ff6aa Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 14:47:55 +0100 Subject: [PATCH 16/33] enable mermaid in sphinx --- docs/.sphinx/requirements.txt | 1 + docs/conf.py | 79 ++++++++++++++++++----------------- 2 files changed, 41 insertions(+), 39 deletions(-) diff --git a/docs/.sphinx/requirements.txt b/docs/.sphinx/requirements.txt index 35c8c1c35..9c91300d4 100644 --- a/docs/.sphinx/requirements.txt +++ b/docs/.sphinx/requirements.txt @@ -14,3 +14,4 @@ sphinx-copybutton myst-parser setuptools-scm sphinx-jsonschema +sphinxcontrib-mermaid \ No newline at end of file diff --git a/docs/conf.py b/docs/conf.py index 479fbbc3f..a19371bde 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -4,6 +4,7 @@ import re import checkbox_ng + # Configuration file for the Sphinx documentation builder. # # For the full list of built-in configuration values, see the documentation: @@ -12,84 +13,84 @@ # -- Project information ----------------------------------------------------- # https://www.sphinx-doc.org/en/master/usage/configuration.html#project-information -project = 'Checkbox' -author = 'Canonical Group Ltd' +project = "Checkbox" +author = "Canonical Group Ltd" copyright = "%s, %s" % (datetime.date.today().year, author) # Here we just keep the version number and not any .devhash because that would # make the CI/CD spellcheck fail mistaking any .devhash occurence for an # unknown word -release = re.match(r"(\d+\.{0,1})+", checkbox_ng.__version__).group(0).rstrip(".") +release = ( + re.match(r"(\d+\.{0,1})+", checkbox_ng.__version__).group(0).rstrip(".") +) # Open Graph configuration - defines what is displayed in the website preview ogp_site_url = "https://checkbox.readthedocs-hosted.com/" ogp_site_name = project -ogp_image = "https://assets.ubuntu.com/v1/253da317-image-document-ubuntudocs.svg" +ogp_image = ( + "https://assets.ubuntu.com/v1/253da317-image-document-ubuntudocs.svg" +) html_context = { # Change to the discourse instance you want to be able to link to - #"discourse": "https://discourse.ubuntu.com", + # "discourse": "https://discourse.ubuntu.com", # Change to the GitHub info for your project "github_url": "https://github.com/canonical/checkbox", "github_version": "main", "github_folder": "docs", - "github_filetype": "rst" + "github_filetype": "rst", } # Used for related links - no need to change -if 'discourse' in html_context: - html_context['discourse_prefix'] = html_context['discourse'] + "/t/" +if "discourse" in html_context: + html_context["discourse_prefix"] = html_context["discourse"] + "/t/" # -- General configuration --------------------------------------------------- # https://www.sphinx-doc.org/en/master/usage/configuration.html#general-configuration extensions = [ - 'sphinx_design', - 'sphinx_tabs.tabs', - 'sphinx_reredirects', - 'youtube-links', - 'related-links', - 'custom-rst-roles', - 'terminal-output', - 'sphinx_copybutton', - 'sphinxext.opengraph', - 'myst_parser', - 'sphinx-jsonschema' - ] - -myst_enable_extensions = [ - "substitution", - "deflist" + "sphinxcontrib.mermaid", + "sphinx_design", + "sphinx_tabs.tabs", + "sphinx_reredirects", + "youtube-links", + "related-links", + "custom-rst-roles", + "terminal-output", + "sphinx_copybutton", + "sphinxext.opengraph", + "myst_parser", + "sphinx-jsonschema", ] -exclude_patterns = ['_build', 'Thumbs.db', '.DS_Store', '.sphinx'] +myst_enable_extensions = ["substitution", "deflist"] + +exclude_patterns = ["_build", "Thumbs.db", ".DS_Store", ".sphinx"] rst_epilog = """ .. include:: /reuse/links.txt """ source_suffix = { - '.rst': 'restructuredtext', - '.md': 'markdown', + ".rst": "restructuredtext", + ".md": "markdown", } # Links to ignore when checking links -linkcheck_ignore = [ - 'http://127.0.0.1:8000' - ] +linkcheck_ignore = ["http://127.0.0.1:8000"] # -- Options for HTML output ------------------------------------------------- # https://www.sphinx-doc.org/en/master/usage/configuration.html#options-for-html-output # Find the current builder builder = "dirhtml" -if '-b' in sys.argv: - builder = sys.argv[sys.argv.index('-b')+1] +if "-b" in sys.argv: + builder = sys.argv[sys.argv.index("-b") + 1] # Setting templates_path for epub makes the build fail if builder == "dirhtml" or builder == "html": templates_path = [".sphinx/_templates"] -html_theme = 'furo' +html_theme = "furo" html_last_updated_fmt = "" html_permalinks_icon = "ΒΆ" html_theme_options = { @@ -121,7 +122,7 @@ "color-highlighted-background": "#EbEbEb", "color-link-underline": "var(--color-background-primary)", "color-link-underline--hover": "var(--color-background-primary)", - "color-version-popup": "#772953" + "color-version-popup": "#772953", }, "dark_css_variables": { "color-foreground-secondary": "var(--color-foreground-primary)", @@ -145,17 +146,17 @@ "color-highlighted-background": "#666", "color-link-underline": "var(--color-background-primary)", "color-link-underline--hover": "var(--color-background-primary)", - "color-version-popup": "#F29879" + "color-version-popup": "#F29879", }, } -html_static_path = ['.sphinx/_static'] +html_static_path = [".sphinx/_static"] html_css_files = [ - 'custom.css', - 'github_issue_links.css', + "custom.css", + "github_issue_links.css", ] html_js_files = [ - 'github_issue_links.js', + "github_issue_links.js", ] From 50d650d4d7f454a972d979e0e022e461c6b4ea06 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 14:48:19 +0100 Subject: [PATCH 17/33] translate the mermaid doc from md to rst --- docs/explanation/remote.rst | 54 ++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index 4ee95f5ae..df1b1cad9 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -98,34 +98,34 @@ Automatic session resume # Checkbox Agent bootup process -```mermaid -graph TD; +.. mermaid:: + + graph TD; + proc("agent process starts") - load("load previous session") - resume("resume the previous session") - resume_crashed("resume the previous session") - interactive{"was the previous interactive"} - mark_pass("mark last running job as passing") - mark_crash("mark last running job as crashing") - idle("go into idle state") - listen("listen for a controller") - proc --> load - last_job{"was the last job a `noreturn` job?"} - load -->last_job - last_job-->|yes| resume - resume --> mark_pass - - last_job-->|no| interactive - interactive-->|yes| idle - idle --> listen - mark_pass --> listen - - interactive-->|no| resume_crashed - resume_crashed --> mark_crash - mark_crash --> listen -``` - - + load("load previous session") + resume("resume the previous session") + resume_crashed("resume the previous session") + interactive{"was the previous interactive"} + mark_pass("mark last running job as passing") + mark_crash("mark last running job as crashing") + idle("go into idle state") + listen("listen for a controller") + proc --> load + last_job{"was the last job a `noreturn` job?"} + load -->last_job + last_job-->|yes| resume + resume --> mark_pass + + last_job-->|no| interactive + interactive-->|yes| idle + idle --> listen + mark_pass --> listen + + interactive-->|no| resume_crashed + resume_crashed --> mark_crash + mark_crash --> listen + Remote session characteristics From 97b6a212f377ab17c57fa720b147a06fbb8b23b8 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 14:49:00 +0100 Subject: [PATCH 18/33] remove the md version of the agent bootup diagram --- .../plainbox/impl/session/agent_bootup.md | 34 ------------------- 1 file changed, 34 deletions(-) delete mode 100644 checkbox-ng/plainbox/impl/session/agent_bootup.md diff --git a/checkbox-ng/plainbox/impl/session/agent_bootup.md b/checkbox-ng/plainbox/impl/session/agent_bootup.md deleted file mode 100644 index 61a850b9e..000000000 --- a/checkbox-ng/plainbox/impl/session/agent_bootup.md +++ /dev/null @@ -1,34 +0,0 @@ -# Checkbox Agent bootup process - -```mermaid -graph TD; - proc("agent process starts") - load("load previous session") - resume("resume the previous session") - resume_crashed("resume the previous session") - interactive{"was the previous interactive"} - mark_pass("mark last running job as passing") - mark_crash("mark last running job as crashing") - idle("go into idle state") - listen("listen for a controller") - proc --> load - last_job{"was the last job a `noreturn` job?"} - load -->last_job - last_job-->|yes| resume - resume --> mark_pass - - last_job-->|no| interactive - interactive-->|yes| idle - idle --> listen - mark_pass --> listen - - interactive-->|no| resume_crashed - resume_crashed --> mark_crash - mark_crash --> listen - - - - -``` - - From 11990515d2d4009e4bb24e0583f5610967b38a49 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 14:52:29 +0100 Subject: [PATCH 19/33] add paragraph describing agent resume --- docs/explanation/remote.rst | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index df1b1cad9..5b0fba0cb 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -96,7 +96,16 @@ End this test session preserving its data and launch a new one Automatic session resume ======================== -# Checkbox Agent bootup process +When the agent starts, it checks if there is a previous session that was not +abandoned. If there is, and the session was a non-interactive one it resumes +it. Otherwise, it waits for a Controlelr to connect and chose what to do. + +The outcome of the job that was last running before the session was +interrupted is decided on the type of job it was running. + +The jobs marked with a `noreturn` flag are marked as passing, while other jobs +are considered to have crashed (due to the interruption of the session like a +reboot or a system hang). .. mermaid:: From 3ae2ade381dd18bc84d8c9fec2fce0b7a6399d59 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 14:56:15 +0100 Subject: [PATCH 20/33] smaller diamond --- docs/explanation/remote.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index 5b0fba0cb..975c86fdb 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -121,7 +121,7 @@ reboot or a system hang). idle("go into idle state") listen("listen for a controller") proc --> load - last_job{"was the last job a `noreturn` job?"} + last_job{"last job `noreturn`?"} load -->last_job last_job-->|yes| resume resume --> mark_pass From b8958fc98959df6275465c159ee5aa3f9461ba00 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 15:27:03 +0100 Subject: [PATCH 21/33] fix typo and quote code better --- docs/explanation/remote.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index 975c86fdb..db4212d56 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -98,12 +98,12 @@ Automatic session resume When the agent starts, it checks if there is a previous session that was not abandoned. If there is, and the session was a non-interactive one it resumes -it. Otherwise, it waits for a Controlelr to connect and chose what to do. +it. Otherwise, it waits for a Controller to connect and chose what to do. The outcome of the job that was last running before the session was interrupted is decided on the type of job it was running. -The jobs marked with a `noreturn` flag are marked as passing, while other jobs +The jobs marked with a ``noreturn`` flag are marked as passing, while other jobs are considered to have crashed (due to the interruption of the session like a reboot or a system hang). From d27283acf430c645fb85a2c1298e1a485067706b Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 15:32:14 +0100 Subject: [PATCH 22/33] exclude mermaid blocks from spellcheck; sort list --- docs/.sphinx/spellingcheck.yaml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/.sphinx/spellingcheck.yaml b/docs/.sphinx/spellingcheck.yaml index 691a6ef20..29ff3782a 100644 --- a/docs/.sphinx/spellingcheck.yaml +++ b/docs/.sphinx/spellingcheck.yaml @@ -13,14 +13,15 @@ matrix: - pyspelling.filters.html: comments: false attributes: - - title - alt + - title ignores: - code - - pre - - spellexception - - link - - title - div.relatedlinks - div.visually-hidden - img + - link + - mermaid + - pre + - spellexception + - title From bc6ca8bdceb18df5c58e98239cd9eba33d051dd9 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 15:47:54 +0100 Subject: [PATCH 23/33] surrender to pyspelling --- docs/explanation/remote.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index db4212d56..7e58c478c 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -111,7 +111,7 @@ reboot or a system hang). graph TD; - proc("agent process starts") + process("agent process starts") load("load previous session") resume("resume the previous session") resume_crashed("resume the previous session") @@ -120,8 +120,8 @@ reboot or a system hang). mark_crash("mark last running job as crashing") idle("go into idle state") listen("listen for a controller") - proc --> load - last_job{"last job `noreturn`?"} + process --> load + last_job{"last job ``noreturn``?"} load -->last_job last_job-->|yes| resume resume --> mark_pass From ce581bba1285a2efe3300a7707d608beae85f5c4 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 15:57:35 +0100 Subject: [PATCH 24/33] use better words for system hangs --- docs/explanation/remote.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index 7e58c478c..67376446a 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -105,7 +105,7 @@ interrupted is decided on the type of job it was running. The jobs marked with a ``noreturn`` flag are marked as passing, while other jobs are considered to have crashed (due to the interruption of the session like a -reboot or a system hang). +reboot or a system stall). .. mermaid:: From fd2dc8af74a41dc601b871acfd3183b997d0a92f Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Mon, 11 Dec 2023 16:05:03 +0100 Subject: [PATCH 25/33] correct the exclusion of mermaid content in spelling check --- docs/.sphinx/spellingcheck.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/.sphinx/spellingcheck.yaml b/docs/.sphinx/spellingcheck.yaml index 29ff3782a..8c4a3c4d0 100644 --- a/docs/.sphinx/spellingcheck.yaml +++ b/docs/.sphinx/spellingcheck.yaml @@ -21,7 +21,7 @@ matrix: - div.visually-hidden - img - link - - mermaid + - div.mermaid - pre - spellexception - title From 6e1531f8848d4279a895736d782fbedb0b93920a Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 11:28:55 +0100 Subject: [PATCH 26/33] properly handle no launcher in app_blob + UT --- checkbox-ng/checkbox_ng/launcher/agent.py | 4 +++- checkbox-ng/checkbox_ng/launcher/test_agent.py | 14 ++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/checkbox-ng/checkbox_ng/launcher/agent.py b/checkbox-ng/checkbox_ng/launcher/agent.py index 6c9efa284..913ebd4b8 100644 --- a/checkbox-ng/checkbox_ng/launcher/agent.py +++ b/checkbox-ng/checkbox_ng/launcher/agent.py @@ -176,7 +176,9 @@ def is_the_session_noninteractive( # app blob is a bytes string with a utf-8 encoded json # let's decode it and parse it as json app_blob = json.loads(resumable_session.metadata.app_blob.decode("utf-8")) - launcher = Configuration.from_text(app_blob["launcher"], "resumed session") + launcher = Configuration.from_text( + app_blob.get("launcher", ""), "resumed session" + ) return launcher.sections["ui"].get("type") == "silent" diff --git a/checkbox-ng/checkbox_ng/launcher/test_agent.py b/checkbox-ng/checkbox_ng/launcher/test_agent.py index cdf47dbb4..1ae537ee1 100644 --- a/checkbox-ng/checkbox_ng/launcher/test_agent.py +++ b/checkbox-ng/checkbox_ng/launcher/test_agent.py @@ -157,8 +157,18 @@ def test_a_non_interactive_one(self): self.assertTrue(is_the_session_noninteractive(candidate)) def test_an_interactive(self): - a_non_interactive_launcher = "" # the defautl one is interactive - app_blob = json.dumps({"launcher": a_non_interactive_launcher}) + an_interactive_launcher = """ + [ui] + type = interactive + """ + app_blob = json.dumps({"launcher": an_interactive_launcher}) + metadata = SessionMetaData(app_blob=app_blob.encode("utf-8")) + + candidate = ResumeCandidate("an_id", metadata) + self.assertFalse(is_the_session_noninteractive(candidate)) + + def test_no_launcher_in_app_blob(self): + app_blob = json.dumps({}) metadata = SessionMetaData(app_blob=app_blob.encode("utf-8")) candidate = ResumeCandidate("an_id", metadata) From e4f770a4aa6274d4decdd34a959e240a83c90d75 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 11:31:15 +0100 Subject: [PATCH 27/33] bring back gettext call and mock it in UT --- .../plainbox/impl/session/remote_assistant.py | 2 +- .../impl/session/test_remote_assistant.py | 24 ++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/checkbox-ng/plainbox/impl/session/remote_assistant.py b/checkbox-ng/plainbox/impl/session/remote_assistant.py index 84eecac9f..c637f665e 100644 --- a/checkbox-ng/plainbox/impl/session/remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/remote_assistant.py @@ -719,7 +719,7 @@ def resume_by_id(self, session_id=None): result_dict = { "outcome": IJobResult.OUTCOME_PASS, - "comments": "Automatically passed after resuming execution", + "comments": _("Automatically passed after resuming execution"), } session_share = WellKnownDirsHelper.session_share( self._sa._manager.storage.id diff --git a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py index b7006d12e..4fe340943 100644 --- a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py @@ -152,11 +152,14 @@ def test_resume_by_id_with_result_file_ok(self, mock_load_configs): rsa._sa.resume_session.return_value = mock_meta os_path_exists_mock = mock.Mock() - with mock.patch("os.path.exists", os_path_exists_mock): - with mock.patch("builtins.open", mock.mock_open(read_data="pass")): - os_path_exists_mock.return_value = True - # mock_open.return_value.read.return_value = "pass" - remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) + with mock.patch("plainbox.impl.session.remote_assistant._") as mock__: + mock__.side_effect = lambda x: x + with mock.patch("os.path.exists", os_path_exists_mock): + with mock.patch( + "builtins.open", mock.mock_open(read_data="pass") + ): + os_path_exists_mock.return_value = True + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) mjr = MemoryJobResult( { @@ -164,7 +167,6 @@ def test_resume_by_id_with_result_file_ok(self, mock_load_configs): "comments": "Automatically passed after resuming execution", } ) - rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True) @mock.patch("plainbox.impl.session.remote_assistant.load_configs") @@ -251,12 +253,12 @@ def test_resume_by_id_with_result_file_not_json(self, mock_load_configs): rsa._sa.resume_session.return_value = mock_meta os_path_exists_mock = mock.Mock() - - with mock.patch("os.path.exists", os_path_exists_mock): + with mock.patch("plainbox.impl.session.remote_assistant._") as mock__: + mock__.side_effect = lambda x: x with mock.patch("builtins.open", mock.mock_open(read_data="!@!")): - os_path_exists_mock.return_value = True - # mock_open.return_value.read.return_value = "pass" - remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) + with mock.patch("os.path.exists", os_path_exists_mock): + os_path_exists_mock.return_value = True + remote_assistant.RemoteSessionAssistant.resume_by_id(rsa) mjr = MemoryJobResult( { From 1ae595481a79b2bc2ee50812bf6dea5620a0f1d8 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 11:31:31 +0100 Subject: [PATCH 28/33] smaller diamond redux --- docs/explanation/remote.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/explanation/remote.rst b/docs/explanation/remote.rst index 67376446a..29395430f 100644 --- a/docs/explanation/remote.rst +++ b/docs/explanation/remote.rst @@ -115,7 +115,7 @@ reboot or a system stall). load("load previous session") resume("resume the previous session") resume_crashed("resume the previous session") - interactive{"was the previous interactive"} + interactive{"last session interactive?"} mark_pass("mark last running job as passing") mark_crash("mark last running job as crashing") idle("go into idle state") From b5f71d4f843763dc5e83c20b9b6dd23c6c94fb5a Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 11:35:17 +0100 Subject: [PATCH 29/33] move to function-level mock --- .../plainbox/impl/session/test_remote_assistant.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py index 4fe340943..15c01c1d1 100644 --- a/checkbox-ng/plainbox/impl/session/test_remote_assistant.py +++ b/checkbox-ng/plainbox/impl/session/test_remote_assistant.py @@ -58,13 +58,13 @@ def not_allowed(self, *args): with self.assertRaises(AssertionError): not_allowed(self_mock) + @mock.patch.object(SessionAssistant, "__init__") @mock.patch("plainbox.impl.secure.sudo_broker.is_passwordless_sudo") - def test__reset_sa(self, is_passwordless_sudo_mock): - with mock.patch.object(SessionAssistant, "__init__") as init_mock: - init_mock.return_value = None - # RSA constructor calls _reset_sa, which in turns creates a new SA - rsa = remote_assistant.RemoteSessionAssistant(lambda: None) - self.assertEqual(init_mock.call_count, 1) + def test__reset_sa(self, is_passwordless_sudo_mock, init_mock): + init_mock.return_value = None + # RSA constructor calls _reset_sa, which in turns creates a new SA + rsa = remote_assistant.RemoteSessionAssistant(lambda: None) + self.assertEqual(init_mock.call_count, 1) @mock.patch("fnmatch.filter") def test_start_session_with_launcher(self, mock_filter): From 7c99070392a03cd242155178e4cfca9df9d98614 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 12:57:41 +0100 Subject: [PATCH 30/33] remove "padme" from the lxd_profile once again --- metabox/metabox/lxd_profiles/checkbox.profile | 1 - 1 file changed, 1 deletion(-) diff --git a/metabox/metabox/lxd_profiles/checkbox.profile b/metabox/metabox/lxd_profiles/checkbox.profile index 5a6c8373a..d69a83568 100644 --- a/metabox/metabox/lxd_profiles/checkbox.profile +++ b/metabox/metabox/lxd_profiles/checkbox.profile @@ -17,7 +17,6 @@ config: - python3-jinja2 - python3-markupsafe - python3-packaging - - python3-padme - python3-pip - python3-psutil - python3-pyparsing From 2b0ed4d2b9d2f846bb730840e8d11c0bb1f3c37e Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 13:18:35 +0100 Subject: [PATCH 31/33] remove unnecessary debs from MB profile --- metabox/metabox/lxd_profiles/checkbox.profile | 2 -- 1 file changed, 2 deletions(-) diff --git a/metabox/metabox/lxd_profiles/checkbox.profile b/metabox/metabox/lxd_profiles/checkbox.profile index d69a83568..1bc9b4c0c 100644 --- a/metabox/metabox/lxd_profiles/checkbox.profile +++ b/metabox/metabox/lxd_profiles/checkbox.profile @@ -12,7 +12,6 @@ config: stable_ppa: source: "ppa:checkbox-dev/stable" packages: - - alsa-base - jq - python3-jinja2 - python3-markupsafe @@ -21,7 +20,6 @@ config: - python3-psutil - python3-pyparsing - python3-requests-oauthlib - - python3-tqdm - python3-urwid - python3-xlsxwriter - virtualenv From e5559f0fb2af2949a4c40fbbda41a5c0b4225a0b Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 14:48:48 +0100 Subject: [PATCH 32/33] remove bionic from the special conditions --- metabox/metabox/core/machine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metabox/metabox/core/machine.py b/metabox/metabox/core/machine.py index e4e52725e..70fd6c159 100644 --- a/metabox/metabox/core/machine.py +++ b/metabox/metabox/core/machine.py @@ -302,7 +302,7 @@ def _get_install_source_cmds(self): "sudo python3 -m pip install -e .'" ), ] - if self.config.alias in ["xenial", "bionic"]: + if self.config.alias in ["xenial"]: commands.append( # ensure these two are at the correct version to support xenial ( From 0561d60150718d009f1c984d74418d53ae42c138 Mon Sep 17 00:00:00 2001 From: Maciej Kisielewski Date: Wed, 13 Dec 2023 14:51:29 +0100 Subject: [PATCH 33/33] use simpler comparison --- metabox/metabox/core/machine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metabox/metabox/core/machine.py b/metabox/metabox/core/machine.py index 70fd6c159..bd826e439 100644 --- a/metabox/metabox/core/machine.py +++ b/metabox/metabox/core/machine.py @@ -302,7 +302,7 @@ def _get_install_source_cmds(self): "sudo python3 -m pip install -e .'" ), ] - if self.config.alias in ["xenial"]: + if self.config.alias == "xenial": commands.append( # ensure these two are at the correct version to support xenial (