bus1 / dbus-broker

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

dbus-broker.service needs a dependency After=systemd-sysusers.service #268

Closed poettering closed 3 years ago

poettering commented 3 years ago

For some reason dbus-broker.service sets DefaultDependencies=no. Why that? dbus-daemon didn't do that?

I think dbus should just run as regular service, i.e. please drop the DefautlDependencies=no

But at the very least it need an After=systemd-sysusers.service since otherwise it will load its policy before the system users it lists exist. See https://github.com/systemd/systemd/issues/19212

bluca commented 3 years ago

It was changed to an early boot service by: https://github.com/bus1/dbus-broker/commit/954362424aad

poettering commented 3 years ago

Hmm, I see.

I guess it should do this:

…
DefaultDependencies=no
After=sysinit.target dbus.socket
Before=basic.target shutdown.target
Requires=dbus.socket
Conflicts=shutdown.target
…
rathann commented 3 years ago

@poettering The only difference from the Fedora default unit file seems to be the addition of After=sysinit.target dbus.socket. Unfortunately, that doesn't help. systemd-networkd.service and systemd-networkd.socket both fail to start.

poettering commented 3 years ago

@rathann hmm, what? systemd-networkd.socket fails for you? That doesn't sound like something that has anything to do with the issue raised here?

dvdhrm commented 3 years ago
…
DefaultDependencies=no
After=sysinit.target dbus.socket
Before=basic.target shutdown.target
Requires=dbus.socket
Conflicts=shutdown.target
…

@poettering, you sure with this? I will gladly add it, but I am having a hard time judging whether this is correct. It matches the current unit-file, apart from After=sysinit.target dbus.socket. I guess the idea is to hook dbus between sysinit.target and basic.target, and thus be before any D-Bus service, but after anything that is part of system setup?

I think the underlying problem here is that all D-Bus users have a dependency on D-Bus, and want to be started after D-Bus is available, and thus will be stopped before D-Bus is stopped. However, due to the way socket activation works, they all just order against the socket. This is fine for startup, but for shutdown it does not help at all.

Wouldn't the correct fix be to add an implicit After=dbus.service when something has After=dbus.socket? But somehow make it only apply to teardown, not startup? Or maybe it is even fine for startup, since the After only orders, it does not pull anything in.

poettering commented 3 years ago

@poettering, you sure with this? I will gladly add it, but I am having a hard time judging whether this is correct. It matches the current unit-file, apart from After=sysinit.target dbus.socket.

So the After=dbus.socket is not strictly necessary, it's implicit because once dbus.socket is loaded it looks for the service it is supposed to activate, and then adds a Before= dep on that, and that will imply the After= dep the other way. The only reason I think adding the dep explicitly would be nice is due to the symlink aliasing you do: i think it' simply philosophically nicer if you declare deps on the units you actually mean instead of relying on the aliasing since this means if you do "systemctl show" while the dbus-broker units are not enabled you'll still see the right deps in place. If you strictly rely on the symlink aliasing then the dbus-broker.service ⟷ dbus.socket deps will be incomplete while the service is disabled and that's not great for introspection.

Anyway, I'd recommend the After=dbus.socket, but it's not essential.

The After=sysinit.target is more important though. It's a more generic form of saying "After=systemd-sysusers.service", and that one you really need because it will create system users which dbus-broker then wants to resolve because they are likely listed in the dbus policy. Without the dep things are racy: if dbus-broker starts up early enough the users might not exist yet, and then operate on an incomplete policy.

sysinit.target is supposed to abstract away the early-boot one-time setup things, such as syusers/tmpfiles and related stuff. It's also where foreign stuff should plug in work-alikes. For example sssd might want to register system user too, and should thus order itself before sysinit.target, too. As long dbus-broker orders itself after that it should be able to pick up everything it needs.

I guess the idea is to hook dbus between sysinit.target and basic.target, and thus be before any D-Bus service, but after anything that is part of system setup?

I think the underlying problem here is that all D-Bus users have a dependency on D-Bus, and want to be started after D-Bus is available, and thus will be stopped before D-Bus is stopped. However, due to the way socket activation works, they all just order against the socket. This is fine for startup, but for shutdown it does not help at all.

So far the rule actually was (i.e. before you added 954362424aad, and how it still is in dbus-daemon reference implementation): if you want to talk D-Bus during shutdown you better order yourself manually against dbus.service, as you then do not just need the socket to be bound but the actual service behind it to operate for you. And you need a similar dep from your service to whatever other service you intend to reach via D-Bus and so on. If you don't have the deps then the assumption is that you won't need any other service to be up, and also not dbus, and everything can be stopped simultaneously.

or with other words, our thinking always was: don't assume that dbus IPC works till the end implicitly. If you write a service and need during shutdown, then declare the dep on the dbus service explicitly. Now, with 954362424aad you went a different way: you opted to make it your problem instead of leaving it to be a problem of the services that actually need this. Which I am not totally against it, but this does create the issue that you need to be very careful with the deps you actually set because you are suddenly in early boot territory where you need to be more careful with stuff since many basic things simply don't exist yet.

Anyway, if you order dbus-broker.service After=sysinit.target and Before=basic.target you should get everything you need. System users mentioned in dbus policy should all already exist but you still get started before any regular service and more importantly remain running until after all regular services exited.

dvdhrm commented 3 years ago

@poettering, you sure with this? I will gladly add it, but I am having a hard time judging whether this is correct. It matches the current unit-file, apart from After=sysinit.target dbus.socket.

So the After=dbus.socket is not strictly necessary, it's implicit because once dbus.socket is loaded it looks for the service it is supposed to activate, and then adds a Before= dep on that, and that will imply the After= dep the other way. The only reason I think adding the dep explicitly would be nice is due to the symlink aliasing you do: i think it' simply philosophically nicer if you declare deps on the units you actually mean instead of relying on the aliasing since this means if you do "systemctl show" while the dbus-broker units are not enabled you'll still see the right deps in place. If you strictly rely on the symlink aliasing then the dbus-broker.service ⟷ dbus.socket deps will be incomplete while the service is disabled and that's not great for introspection.

Anyway, I'd recommend the After=dbus.socket, but it's not essential.

Right. I thought I remembered that the dependency is implicit. But I agree, adding it makes it easier to work with.

The After=sysinit.target is more important though. It's a more generic form of saying "After=systemd-sysusers.service", and that one you really need because it will create system users which dbus-broker then wants to resolve because they are likely listed in the dbus policy. Without the dep things are racy: if dbus-broker starts up early enough the users might not exist yet, and then operate on an incomplete policy.

sysinit.target is supposed to abstract away the early-boot one-time setup things, such as syusers/tmpfiles and related stuff. It's also where foreign stuff should plug in work-alikes. For example sssd might want to register system user too, and should thus order itself before sysinit.target, too. As long dbus-broker orders itself after that it should be able to pick up everything it needs.

Yeah, not sure why we missed that. I added it now!

I guess the idea is to hook dbus between sysinit.target and basic.target, and thus be before any D-Bus service, but after anything that is part of system setup?

I think the underlying problem here is that all D-Bus users have a dependency on D-Bus, and want to be started after D-Bus is available, and thus will be stopped before D-Bus is stopped. However, due to the way socket activation works, they all just order against the socket. This is fine for startup, but for shutdown it does not help at all.

So far the rule actually was (i.e. before you added 9543624, and how it still is in dbus-daemon reference implementation): if you want to talk D-Bus during shutdown you better order yourself manually against dbus.service, as you then do not just need the socket to be bound but the actual service behind it to operate for you. And you need a similar dep from your service to whatever other service you intend to reach via D-Bus and so on. If you don't have the deps then the assumption is that you won't need any other service to be up, and also not dbus, and everything can be stopped simultaneously.

or with other words, our thinking always was: don't assume that dbus IPC works till the end implicitly. If you write a service and need during shutdown, then declare the dep on the dbus service explicitly. Now, with 9543624 you went a different way: you opted to make it your problem instead of leaving it to be a problem of the services that actually need this. Which I am not totally against it, but this does create the issue that you need to be very careful with the deps you actually set because you are suddenly in early boot territory where you need to be more careful with stuff since many basic things simply don't exist yet.

I wouldn't mind doing what dbus-daemon does, but it does not work. It is not that we wanted to change this, but we saw lots of failures during shutdown. We saw the same with dbus-daemon, and we wanted it fixed.

Thing is, lots of applications are ready to deal with failing communication to other services during shutdown, but they are unable to deal with actual dbus failures.

Anyway, we thought it is easier to treat D-Bus as a system facility that applications should not worry about, so we pulled it before basic.target.

Thanks for the report! Should be fixed in 28af5ac996c94922fc49660e0efa955f57575187.

dvdhrm commented 3 years ago

Fixed in 28af5ac.