qemu: Ensure virtio journal streaming is killed on shutdown #4290
Conversation
In ostreedev/ostree#3513 i'm trying to ensure that all the ostree mounts are cleaned up. It turned out that it was *this* unit keeping `/var` open because the `journalctl` binary doesn't know to stop tracking the `/var/log/journal` files when the daemon has flushed. (We could obviously fix systemd to handle this) But anyways the main reason this exists is to forward logs from startup and especially entering emergency.target. While it's useful to get logs from shutdown, there are other ways to do that. This makes `var.mount` reliably unmounted for me.
| # 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 |
There was a problem hiding this comment.
Also this was always wrong and did nothing, we now use the socket correctly
There was a problem hiding this comment.
i.e. systemd.journal.service should have been systemd-journal.service ?
There was a problem hiding this comment.
Probably yes, but it's more correct to be after the socket, which is what we want to talk to
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to ensure the virtio journal streaming service is terminated correctly on shutdown to prevent it from holding /var open. The changes to the systemd unit file are mostly correct, but I've identified a potential race condition with the use of Conflicts=shutdown.target and have provided a suggestion to improve the shutdown ordering robustness.
| # Do get killed on shutdown | ||
| Conflicts=shutdown.target |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Do we need Conflicts=shutdown.target because we have IgnoreOnIsolate=true ?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
right. and we have IgnoreOnIsolate because we want to continue to log even when heading into emergency.target.
In ostreedev/ostree#3513 i'm trying to ensure that all the ostree mounts are cleaned up. It turned out that it was this unit keeping
/varopen because thejournalctlbinary doesn't know to stop tracking the/var/log/journalfiles when the daemon has flushed.(We could obviously fix systemd to handle this)
But anyways the main reason this exists is to forward logs from startup and especially entering emergency.target. While it's useful to get logs from shutdown, there are other ways to do that.
This makes
var.mountreliably unmounted for me.