Skip to content

qemu: Ensure virtio journal streaming is killed on shutdown #4290

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 28, 2025
Merged

qemu: Ensure virtio journal streaming is killed on shutdown #4290

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion mantle/platform/qemu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1649,7 +1649,12 @@ func (builder *QemuBuilder) VirtioJournal(config *conf.Conf, queryArguments stri
# won't be added to this unit, which would cause it to get
# taken down when isolating to emergency.target
DefaultDependencies=no
After=systemd.journal.service
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this was always wrong and did nothing, we now use the socket correctly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. systemd.journal.service should have been systemd-journal.service ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but it's more correct to be after the socket, which is what we want to talk to

After=systemd-journald.socket
# Do however ensure we get killed before /var is going to be
# unmounted, otherwise we keep it open.
After=local-fs.target
# Do get killed on shutdown
Conflicts=shutdown.target
Comment on lines +1656 to +1657
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While Conflicts=shutdown.target makes the intent clear, it might introduce a race condition during shutdown. According to systemd.unit(5), Conflicts= dependencies are orthogonal to After= and Before= ordering dependencies.

Since both this unit and local-fs.target have Conflicts=shutdown.target, when shutdown.target is activated, systemd will stop both units, but their relative order is not guaranteed by the After=local-fs.target directive. This could lead to this service being stopped after local-fs.target, which would re-introduce the problem this PR aims to solve.

The After=local-fs.target directive is sufficient to ensure this unit is stopped before local-fs.target during a graceful shutdown. Removing the Conflicts=shutdown.target will rely on this ordering, which is more deterministic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need Conflicts=shutdown.target because we have IgnoreOnIsolate=true ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I think Gemini's review doesn't understand the semantics here with IgnoreOnIsolate.

That said IgnoreOnIsolate can create very messy situations like this and it'd definitely be better to try to kill this unit entirely...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. and we have IgnoreOnIsolate because we want to continue to log even when heading into emergency.target.

# After systemd-journal-flush because otherwise the journalctl -f
# below will stop when the journal is flushed. Not sure if this is
# a bug or intended behavior.
Expand Down
Loading