From 6a4537277d9eb64899433961f2716cbe8ba42a0e Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 3 Aug 2023 11:38:31 +0200 Subject: [PATCH] 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):