GNOME Bugzilla – Bug 755735
Add a systemd user service for each D-Bus session service
Last modified: 2015-10-02 05:33:31 UTC
Created attachment 312304 [details] [review] Add a systemd user service for each D-Bus session service This means each service will run in its own cgroup under a systemd user session, instead of being a child of dbus-daemon that is considered to be part of dbus.service. --- The patch is based on 3.12.11, but should apply OK to newer branches with the org.gnome.evolution.dataserver.Calendar parts deleted.
Thanks for a bug report and the patch. I do not know systemd enough, thus forgive me, but could you explain what it is good for, please? All the D-Bus services are run on demand, only when needed, which feels like a good thing to do.
Review of attachment 312304 [details] [review]: Might the systemd be disable-able in configure time too? I guess it doesn't always make sense to build and distribute it everywhere. ::: services/evolution-addressbook-factory/Makefile.am @@ +6,3 @@ + +systemd_user_in_files = evolution-addressbook-factory.service.in +systemd_userdir = $(prefix)/lib/systemd/user It would be good to not define this directory in each Makefile.am, though I see it is already done this way with the servicedir, which is a pita. The right place is configure.ac, from my point of view. Also do not use $(prefix)/lib/, it's wrong for distributions which use multilib and overwrite library path to something like 'lib64'. There is a ${libdir} defined (see configure.ac).
(In reply to Milan Crha from comment #1) > I do not know systemd enough, thus > forgive me, but could you explain what it is good for, please? In traditional D-Bus service activation, activated services are children of `dbus-daemon --session`, so they inherit its inheritable process attributes (environment variables, rlimits, blocked signals, that sort of thing). dbus-daemon is not really designed to be a process supervisor, although it's better than nothing. Traditionally, dbus-daemon has been part of the X11 session, something like this in systemd-cgls: \- session-1.scope \- Xsession |- dbus-daemon --session | \- e-d-s and other activated services \- gnome-session \- firefox If using `systemd --user` with D-Bus as a systemd user service, it thinks the activated services are part of dbus.service, so you get this: \- user@1000.service \- dbus.service |- dbus-daemon --session \- e-d-s and other activated services \- session-1.scope \- Xsession \- gnome-session \- firefox With activation via systemd, the activated services are children of `systemd --user`, which *is* designed to be a process supervisor. In particular, systemd now knows that each e-d-s service is a separate service, and can manage them in the usual systemd ways (e.g. restarting, separating their stderr in the Journal, applying resource limits/fairness): \- user@1000.service |- dbus.service | \- dbus-daemon --session |- evolution-addressbook-factory.service | \- evolution-addressbook-factory ... \- session-1.scope \- Xsession \- gnome-session \- firefox (In reply to Milan Crha from comment #1) > All the D-Bus services are run on demand, only when needed, which feels like > a good thing to do. This is still the case, with or without this patch. It does not add the necessary symlinks to make systemd start the e-d-s services "eagerly" (but a user or integrator could add them, if they want to). (In reply to Milan Crha from comment #2) > Might the systemd be disable-able in configure time too? I guess it doesn't > always make sense to build and distribute it everywhere. If you insist, although there's basically nothing to build (4 text files). > It would be good to not define this directory in each Makefile.am, though I > see it is already done this way with the servicedir, which is a pita. The > right place is configure.ac, from my point of view. Would you like the D-Bus session service directory to have the same treatment while I'm there? > Also do not use $(prefix)/lib/, it's wrong for distributions which use > multilib and overwrite library path to something like 'lib64'. $(prefix)/lib is correct for this, even on platforms that set ${libdir} to /usr/lib64 (mostly Red Hat derivatives) or /usr/lib/x86_64-linux-gnu (Debian derivatives). From systemd's Makefile.am: > userunitdir=$(prefix)/lib/systemd/user I can add a comment if you want.
(In reply to Simon McVittie from comment #3) > (In reply to Milan Crha from comment #1) > > All the D-Bus services are run on demand, only when needed, which feels like > > a good thing to do. > > This is still the case, with or without this patch. It does not add the > necessary symlinks to make systemd start the e-d-s services "eagerly" (but a > user or integrator could add them, if they want to). They might want to add special arguments to the factories as well, because they stop themself when not needed for 10 seconds. > (In reply to Milan Crha from comment #2) > > Might the systemd be disable-able in configure time too? I guess it doesn't > > always make sense to build and distribute it everywhere. > > If you insist, although there's basically nothing to build (4 text files). I was just thinking of distributions not using the systemd. Might be less these days, I do not know, but they surely would just ignore/delete those files, thus I thought to give them an easier configure-time option to avoid the create & delete files round trip. > > It would be good to not define this directory in each Makefile.am, though I > > see it is already done this way with the servicedir, which is a pita. The > > right place is configure.ac, from my point of view. > > Would you like the D-Bus session service directory to have the same > treatment while I'm there? Yes, I'd appreciate if you could fix both of them. > > Also do not use $(prefix)/lib/, it's wrong for distributions which use > > multilib and overwrite library path to something like 'lib64'. > > $(prefix)/lib is correct for this, even on platforms that set ${libdir} to > /usr/lib64 (mostly Red Hat derivatives) or /usr/lib/x86_64-linux-gnu (Debian > derivatives). From systemd's Makefile.am: > > > userunitdir=$(prefix)/lib/systemd/user > > I can add a comment if you want. Aha, interesting, the lib path is confusing in this case, then. Having a .pc file from systemd which would clearly define where the files should be placed might just add a dependency on it (if it exists at all), which we surely do not want to do. What about adding --with-systemd[=dir] configure option, thus one would be able to disable and/or override the systemd service files destination folder? I think of: --with-systemd - the default, places files into $(prefix)/lib/systemd/user --with-systemd=/my/folder/systemd/user - places files into that folder --without-systemd - will not distribute those systemd service files at all
(In reply to Milan Crha from comment #4) > > Would you like the D-Bus session service directory to have the same > > treatment while I'm there? > > Yes, I'd appreciate if you could fix both of them. I have a patch which I'll test shortly. > Having a .pc > file from systemd which would clearly define where the files should be > placed might just add a dependency on it (if it exists at all), which we > surely do not want to do. Indeed. There's no reason why we need to depend on systemd for this - we're just installing metadata that it can consume, we don't actually need to *use* systemd. > What about adding --with-systemd[=dir] configure option, thus one would be > able to disable and/or override the systemd service files destination > folder? I think of: > --with-systemd - the default, places files into $(prefix)/lib/systemd/user > --with-systemd=/my/folder/systemd/user - places files into that folder > --without-systemd - will not distribute those systemd service files at all Yes. I've called it --with-systemduserunitdir, which is rather unwieldy but does seem to be the conventional name for that configure option.
(In reply to Milan Crha from comment #4) > They might want to add special arguments to the factories as well, because > they stop themself when not needed for 10 seconds. If a user or integrator wants to do that, they can override it with an edited version in /etc/systemd/user, analogous to how they could currently put an edited D-Bus service in {~/.local,/usr/local}/share/dbus-1/services. Alternatively, they can use "drop-in" files matching {/etc,/usr/lib}/systemd/user/evolution-addressbook-factory.d/*.conf to override just the ExecStart attribute with one that adds extra arguments. dbus-daemon doesn't have that feature. If the factories exit 0 or kill themselves with SIGTERM on time-out, systemd will interpret that as normal termination, and will not restart them even if they are configured to restart on failure (which I have not done here, but it's possible). If they exit nonzero or kill themselves with a different signal, I could add a bit of extra metadata to tell systemd "this is actually fine".
(In reply to Simon McVittie from comment #6) > If they exit nonzero or kill themselves with a different > signal, I could add a bit of extra metadata to tell systemd "this is > actually fine". Better not to do any auto-restart, I do not like this, especially in case of some failure on the 3rd-party plugin, which can cause crashes, which might then just cause a loop of "crashed-started-crashed-started-...", which is something one really doesn't want to see.
(In reply to Milan Crha from comment #7) > Better not to do any auto-restart That is systemd's default anyway. Having these services be systemd units just makes auto-restart *possible*, with additional configuration to enable it.
Created attachment 312444 [details] [review] Make D-Bus session service directory configure-time configurable --- As requested.
Created attachment 312445 [details] [review] Add a systemd user service for each D-Bus session service This means each service will run in its own cgroup under a systemd user session, instead of being a child of dbus-daemon that is considered to be part of dbus.service. It does not cause the services to be started on login: they are still started on-demand by bus activation. --- As requested, --with-systemduserunitdir=no (or equivalently, --without-systemduserunitdir) will stop these files from being installed.
Thanks for the patches. Both look fine (*), thus I committed them into the sources. (*) The second has a typo in the source-registry, "systemd_user_in_DATA". I corrected that. Created commit e9d5554 in eds master (3.19.1+) Created commit 51a37f7 in eds master (3.19.1+)