Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure journalctl -b is properly captured on shutdown #1755

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

ogayot
Copy link
Member

@ogayot ogayot commented Aug 3, 2023

With recent changes, the journalctl -b command would fail to run:

      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'

Fixed by making sure that:

  • the output of journalctl -b is captured instead of going into the journal (which in itself feels like a bug).
  • the command runner expects the stdout, stderr and other possible keyword arguments.

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 <[email protected]>
@ogayot
Copy link
Member Author

ogayot commented Aug 3, 2023

With this change, the installer-journal.txt file will contain some boilerplate data as shown in the example above:

Running as unit: run-u1060.service
not running: journalctl -b      <---- This is the output of the command
Finished with result: success
Main processes terminated with: code=exited/status=0
Service runtime: 4ms
CPU time consumed: 3ms

If this isn't ok, we should use the --quiet option from systemd-run. This should remove the boilerplate. Otherwise, we can use another mechanism that does not lean on systemd-run.

@ogayot ogayot requested a review from dbungert August 3, 2023 15:59
@ogayot ogayot merged commit 2fdb0be into canonical:main Aug 4, 2023
10 checks passed
@ogayot ogayot deleted the copy-logs-fix branch August 4, 2023 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants