From 6a4537277d9eb64899433961f2716cbe8ba42a0e Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 3 Aug 2023 11:38:31 +0200 Subject: [PATCH 1/2] runner: make runner.start() forward kwargs to astart_command() In a recent patch, we switched the mechanism used for running some shutdown related commands. We used to lean on run_command() and we now lean on runner.run(). runner.run() works a bit differently because it runs the command through systemd-run - and by default does not capture the output (it goes into the journal). Another difference is that runner.run() would not forward the keyword arguments (like stdout, stderr) to subprocess. Well, it would forward those arguments to runner.start() which would raise an exception: File "subiquity/server/controllers/shutdown.py", line 118, in copy_logs_to_target await self.app.command_runner.run( File "subiquity/server/runner.py", line 98, in run proc = await self.start(cmd, **opts) ^^^^^^^^^^^^^^^^^^^^^^^ TypeError: DryRunCommandRunner.start() got an unexpected keyword argument 'stdout' We now ensure that those keyword arguments are properly forwarded to subprocess (through astart_command). Signed-off-by: Olivier Gayot --- subiquity/server/runner.py | 20 ++++++++++++++++---- subiquity/server/tests/test_runner.py | 12 +++++++++++- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/subiquity/server/runner.py b/subiquity/server/runner.py index 8a4a95d5..a0cbb78f 100644 --- a/subiquity/server/runner.py +++ b/subiquity/server/runner.py @@ -70,12 +70,17 @@ class LoggedCommandRunner: return prefix + cmd async def start( - self, cmd: List[str], *, private_mounts: bool = False, capture: bool = False + self, + cmd: List[str], + *, + private_mounts: bool = False, + capture: bool = False, + **astart_kwargs, ) -> asyncio.subprocess.Process: forged: List[str] = self._forge_systemd_cmd( cmd, private_mounts=private_mounts, capture=capture ) - proc = await astart_command(forged) + proc = await astart_command(forged, **astart_kwargs) proc.args = forged return proc @@ -128,10 +133,17 @@ class DryRunCommandRunner(LoggedCommandRunner): return self.delay async def start( - self, cmd: List[str], *, private_mounts: bool = False, capture: bool = False + self, + cmd: List[str], + *, + private_mounts: bool = False, + capture: bool = False, + **astart_kwargs, ) -> asyncio.subprocess.Process: delay = self._get_delay_for_cmd(cmd) - proc = await super().start(cmd, private_mounts=private_mounts, capture=capture) + proc = await super().start( + cmd, private_mounts=private_mounts, capture=capture, **astart_kwargs + ) await asyncio.sleep(delay) return proc diff --git a/subiquity/server/tests/test_runner.py b/subiquity/server/tests/test_runner.py index c975b082..270bf87c 100644 --- a/subiquity/server/tests/test_runner.py +++ b/subiquity/server/tests/test_runner.py @@ -14,7 +14,8 @@ # along with this program. If not, see . import os -from unittest.mock import patch +import subprocess +from unittest.mock import ANY, patch from subiquity.server.runner import DryRunCommandRunner, LoggedCommandRunner from subiquitycore.tests import SubiTestCase @@ -103,6 +104,15 @@ class TestLoggedCommandRunner(SubiTestCase): ] self.assertEqual(cmd, expected) + async def test_start(self): + runner = LoggedCommandRunner(ident="my-id", use_systemd_user=False) + + with patch("subiquity.server.runner.astart_command") as astart_mock: + await runner.start(["/bin/ls"], stdout=subprocess.PIPE) + + expected_cmd = ANY + astart_mock.assert_called_once_with(expected_cmd, stdout=subprocess.PIPE) + class TestDryRunCommandRunner(SubiTestCase): def setUp(self): From 34242846db39e4ed2f7b50b74d16588e7e1c7831 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 3 Aug 2023 12:03:16 +0200 Subject: [PATCH 2/2] shutdown: make sure we capture the output of journalctl -b Signed-off-by: Olivier Gayot --- subiquity/server/controllers/shutdown.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/subiquity/server/controllers/shutdown.py b/subiquity/server/controllers/shutdown.py index da4466f8..7cf05304 100644 --- a/subiquity/server/controllers/shutdown.py +++ b/subiquity/server/controllers/shutdown.py @@ -120,7 +120,10 @@ class ShutdownController(SubiquityController): try: with open_perms(journal_txt) as output: await self.app.command_runner.run( - ["journalctl", "-b"], stdout=output, stderr=subprocess.STDOUT + ["journalctl", "-b"], + capture=True, + stdout=output, + stderr=subprocess.STDOUT, ) except Exception: log.exception("saving journal failed")