search

bus1 / dbus-broker

Linux D-Bus Message Broker
https://github.com/bus1/dbus-broker/wiki
Apache License 2.0
667 stars 78 forks source link

units: drop the ordering after sysinit.target #271

Closed keszybz closed 2 years ago

keszybz commented 3 years ago

It is causing significant delays during boot, see https://bugzilla.redhat.com/show_bug.cgi?id=1976653.

Also, the justification for this ordering was a misunderstanding:

Secondly, make sure to order after sysinit.target, so basic system setup is done before we start. This is especially important to make sure the launcher can resolve user names and read the disk.

Ordering after sysinit.target is not necessary to "read the disk". The basic file system is established before services are started. Some other file systems may be mounted later, but system services should not access those file systems. (In cases where they do, RequiresMountsFor= may be used, but dbus-broker shouldn't require any special paths, afaik). OTOH, sysinit.target is not enough to establish lookup of arbitrary user names. After=nss-user-lookup.target would be required, but that would create an even worse ordering loop, because user lookups may require established network. sssd.service is ordered before nss-user-lookup.target, but after basic.target.

dvdhrm commented 2 years ago

Makes sense to me. FTR, we are not using nss.

We are, but only if passwd is not sufficient. You get a loud warning in syslog then, though.

poettering commented 1 year ago

So I am pretty sure this should reverted, it doesn't really make any sense.

So a while back dracut decided to add dbus to the initrd. Which I think is a really bad idea, since you cannot schedule when dbus services become activatable right now, so if dbus is there at all, things will deadlock here and there because they try to talk to dbus, which then activates something which then hangs because the system is simply not far enough.

D-Bus support was added to dracut here: https://github.com/dracutdevs/dracut/pull/900

In the dbus support they then hacked in their own version of dbus.socket: https://github.com/dracutdevs/dracut/blob/master/modules.d/06dbus-broker/module-setup.sh#L62 because of course dbus doesn't actually run in the initrd, the deps don't allow it. they hence patched the socket unit for their needs, but then left the dbus broker service as is though.

Then they of course ran into the deadlocks that cost them 90s at boot until the bus call timed out. This led to this: https://bugzilla.redhat.com/show_bug.cgi?id=1976653#c4

Noone bothered with trying to understand these implications though. Instead they asked for this PR #271 here to drop the sysinit.target dep. Which makes little sense, given that dbus.socket actually implicitly has After=sysinit.target, and since dbus-broker.service has After=sysinit.target it wil transitively also get After=sysinit.target. – Except of course that for their forked version of dbus.socket the sysinit.target order doesn't exist.

But it's simply wrong arranging the dbus service here to match their downstream forked dbus.socket file...

Generally: it might be OK to add dbus to the initrd, but this must be done carefully which means the fact that a service name becomes activatable must be something that can be scheduled, so that it isn't scheduled to early. Otherwise you'll always run into deadlocks like the above. But this requires major reworking of dbus. Just willy-nilly adding it anyway to the initrd is just a bad idea ignoring all this brokenness.

poettering commented 1 year ago

So, I am not going to file a revert, because I am lazy, and it's strictly speaking not necessary to revert this, because of the fact dbus-broker.service has both After=dbus.sock and Requires=dbus.socket. And since dbus.socket implicitly has After=sysinit.target and Requires=sysinit.target, the chain is complete and the revert is not strictly necessary.

So this commit#s reasoning was bogus if you ask me, but it doesn't precisely hurt either. Hence not bothering with a revert.