Opened 10 years ago

Closed 8 years ago

Last modified 8 years ago

#4503 closed Enhancement (fixed)

add basic systemd integration

Reported by: zdzichu Owned by: jordan
Priority: Normal Milestone: 2.80
Component: Daemon Version: 2.33
Severity: Normal Keywords:
Cc: tomek@…, nikoli@…, leho@…

Description

Hi,

I'm attaching patch to add basic hints for systemd integration. This patch adds two files with helper functions. Alternatively, those functions are shipped in library in very recent versions of systemd.

Those functions are NOP in non-systemd environments.

Changes work fine with unit file, which is also attached. This unit should be installed in /lib/systemd/system. I've skipped that bit - autoconf/automake scare me. Nevertheless, this unit file can be shipped by you instead of universal initscript.

Please consider status updates in daemon's mainloop as an example. I would prefer something along "Running XX of YY torrents", but couldn't do it without extending libtransmission API.

Sample output:

# systemctl status transmission-daemon.service 
transmission-daemon.service - Transmission BT Client headless
	  Loaded: loaded (/etc/systemd/system/transmission-daemon.service; disabled)
	  Active: active (running) since Fri, 23 Sep 2011 22:13:09 +0200; 1min 46s ago
	Main PID: 25943 (transmission-da)
	  Status: "Downloading, 21.89 KBps."
	  CGroup: name=systemd:/system/transmission-daemon.service
		  └ 25943 /usr/bin/transmission-daemon -f -T --blocklist -g /var/lib/transmission/.config/transmission

I'm happy to answer any questions.

Attachments (3)

0001-add-systemd-hints.diff (27.3 KB) - added by zdzichu 10 years ago.
transmission-daemon.service (338 bytes) - added by zdzichu 10 years ago.
systemd unit file for transmission-daemon
transmissiond@.service (100 bytes) - added by mus 10 years ago.

Download all attachments as: .zip

Change History (26)

Changed 10 years ago by zdzichu

Changed 10 years ago by zdzichu

systemd unit file for transmission-daemon

comment:1 Changed 10 years ago by jordan

zdzichu, thanks for the patch and for your interest in transmission, but I have a couple of reservations about adding this feature.

  1. There are many distros and non-Linux systems that aren't running systemd. Does this patch break Transmission on those systems? For example, what happens on BSD?
  1. I don't think bundling sd-daemon.[ch] is the right thing to do. Instead -- if transmission-daemon supports systemd at all -- it should test to see if sd_notifyf() is available on the system. (What header should we look in for it?)
  1. The transmission-daemon.service file would be a break from Transmission's policy of letting downstream OSes implement the services scripts. The Transmission team has stayed away from this because we don't want to have to know the ins and outs of all the downstream OSes. :)

What I could see transmission-daemon doing is adding a small wrapper function, tr_notifyf() or something, which would call sd_notify() if systemd was available at compile time, and which would be a NOOP otherwise. I'm not sure how useful that would be without a transmission-daemon.service file though.

comment:2 Changed 10 years ago by zdzichu

Hi,

ad 1). Those function disappear on non-linux systems thanks to:

#if defined(DISABLE_SYSTEMD) || !defined(__linux__)
    return 0;

Additionaly, those functions check if system was booted using systemd. If not, they do nothing.

ad 2). you should check libsystemd-daemon.pc, although it is only provided by very recent versions of systemd. AFAIK none released version of any distribution has this pkgconfig file, but it will change in few weeks. To date, systemd recommends bundling sd-daemon.[ch].

Michał Górny pointed me to this autotool snippet which could be useful:

# Check whether to enable systemd startup notification.
# This requires libsystemd-daemon.
AC_ARG_WITH([systemd-daemon], AS_HELP_STRING([--with-systemd-daemon],
	[Add support for systemd startup notification (default is autodetected)])
	[USE_SYSTEMD_DAEMON=$withval], [USE_SYSTEMD_DAEMON=auto])
AS_IF([test "x$USE_SYSTEMD_DAEMON" != "xno"], [
    PKG_CHECK_MODULES([SYSTEMD_DAEMON], [libsystemd-daemon],
	[AC_DEFINE(USE_SYSTEMD_DAEMON,1,[Use systemd startup notification])],
	[AS_IF([test "x$USE_SYSTEMD_DAEMON" = "xyes"],
	    [AC_MSG_ERROR([systemd startup notification support requested, but libsystemd-daemon not found.])]
	)]
    )
])

ad 3) systemd policy is to try persuade upstream for shipping unit files :-). As you can see, service definition is really short and it is expected for all distribution to have the same unit files.

Generally, those sd-notify* functions are designed to be non-intrusive. When no systemd is available, then do nothing. When there is systemd, they make integration smoother. Those make life of downstream easier even if you do not ship .service file itself.

comment:3 Changed 10 years ago by jordan

  • Version changed from 2.33+ to 2.33

Changed 10 years ago by mus

comment:4 Changed 10 years ago by mus

I would also love to see this upstream, but there are some things I would like to point out:

  1. the After= line can be completely removed from the service file. syslog.target is obsolete by now and there is no point in waiting for network.target because the daemon works just fine even if the network isn't up yet. see also http://lists.freedesktop.org/archives/systemd-devel/2012-May/005056.html
  1. imho "-T --blocklist -g /var/lib/transmission/.config/transmission" should not be specified. This should be kept the default and whoever wants to change this can use the web interface, edit settings.json or create a new service file and override the ExecStart? value (see also ".include" in "man systemd.unit")
  1. the username should not be specified. The user name "transmission" doesn't even exist on my Arch Linux system so the service file wouldn't work at all unless I edit it. I would suggest using systemd's template functionality instead. Set User=%i and add an "@" to the filename: transmissiond@.service . A user could then specify the user the daemon should run as by symlinking it accordingly:
    ln -s /usr/lib/systemd/system/transmissiond@.service /etc/systemd/system/multi-user.target.wants/transmissiond@mus.service
    
  2. I don't really know much about sd_notify functionality, but as far as I can see this is just nice to have. why use the systemd status to show the download speed when you can just use transmission-remote? I'm not saying this shouldn't be added but the developers should know that a service file would be enough for basic systemd support. There is no code change neccessary whatsoever, you would just have to ship the service file which doesn't hurt anybody.

I attached my proposed service file. This service file works without the sd_notify functionality and is in no way distribution-specific and very simple.

comment:5 Changed 9 years ago by tomegun

I'm looking at making transmission work nicely with Arch Linux, and would like to comment on the suggestions made so far:

  1. I don't think mus' suggestion of instantiating the service for different users is the right thing to do. This is a system-level daemon, so it should either be ran as root, or preferably some system-user such as "transmission", and not as any of the users of your system. If that is what you want, then start it as part of your user-session and I guess it would work automatically.
  1. I agree with mus that any setting that can be configured once the daemon is up and running should not be specified on the command line. What exactly needs to be specified on the commandline to get something that works as expected in most cases, I guess the devs would know best, but I'll make a suggestion further down.
  1. It is correct as mus says that the After= line can be dropped.
  1. Regarding the sd_notify functionality, I should first point out that systemd now ships a library providing this functionality so all you need to do is to (optionally) link against that to make things work. That said, if you are still reluctant to include it, that is not the end of the world. The only functionality it has is to let systemd know when transmission should be considered "ready". If you don't wish to use sd_notif, we have two options:

Use Type=simple. This means that transmission will be considered "ready" immediately. This is typically used if a) you know that no one cares whether or not you are ready yet (this might be the case for you) or b) any sockets you are listening to are set up before transmission is started (this is not the case for you).

Use Type=forking. This means that transmission will be considered "ready" when the first process started has exited (after forking). For this to work nicely it is important that systemd is able to figure out which of the remaining processes is the main one, so it knows if it exits. The best way to do this would be if you specify a PIDFile=.

I think it would benefit all your downstreams, not only the ones using systemd, to include a systemd service file as an example (even if you don't integrate it with your buildsystem). It would tell us what you consider to be a minimal config that works "as expected" in most cases.

This is my suggestion:

[Unit]
Description=Transmission Bit Torrent Daemon

[Service]
User=transmission
Type=forking
PIDFile=/run/transmission.pid
ExecStart=/usr/bin/transmission-daemon --pid-file /run/transmission.pid --config-dir /var/lib/transmission/.config/transmission

[Install]
WantedBy=multi-user.target

This expects that the distributions creates a "transmission" user and creates a folder /var/lib/transmission owend by this user.

What do you think? I'd appreciate your input on this even if you don't wish to include a unit file, so I know what to suggest we ship with Arch.

comment:6 Changed 9 years ago by tomegun

I need to make an addition to my previous suggested service file:

The pid file must be stored in /run/transmission/transmission, as the user "transmission" does not have write access to /run. The folder /run/transmission can be created on boot (on a systemd system) with the correct ownership by simply installing the file /usr/lib/tmpfiles.d/transmission.conf with the contents:

d /run/transmission - transmission transmission -

comment:7 Changed 9 years ago by jordan

I'm not sure how this ticket has slipped through the cracks for so long.

zdzichu, tomegun, are your last patches/comments on this still up-to-date? I'm open to the idea of supporting this ticket in Transmission's code.

comment:8 Changed 9 years ago by tomegun

Hi Jordan,

Thanks for getting back to this. I have put a service file in the Arch package of tranmsission and it seems to be working ok, so you could ship it upstream as-is. If you have any concerns about it I'd be happy to discuss it more.

systemd service file: https://projects.archlinux.org/svntogit/packages.git/tree/trunk/transmission.systemd?h=packages/transmission.

As you can see it depends on 1) /run/transmission existing and being writeable on boot, which is taken care of by the following tmpfiles.d fragment, which you probably also want to ship: https://projects.archlinux.org/svntogit/packages.git/tree/trunk/transmission.tmpfiles?h=packages/transmission. 2) the user existing and having a home folder, so we create that on install-time (I guess this is the job of the distro so nothing you should need to do(?)): https://projects.archlinux.org/svntogit/packages.git/tree/trunk/transmission-cli.install?h=packages/transmission.

Cheers,

Tom

comment:9 Changed 9 years ago by ssiloti

I would suggest using Type=simple with -f rather than Type=forking. Simple is generally preferred to forking since there's no point in forking with systemd and simple is well, simpler.

I can't think of any service which would depend on Transmission being ready. If there are then sd_notify would be the preferred solution.

comment:10 Changed 9 years ago by wooptoo

It would be nice if you could add --log-error to the default ExecStart? line, since Transmission is very verbose by default and fills journald with announce messages.

comment:11 Changed 9 years ago by rahulsundaram

Can I get a status update on this? systemd service file is the only non-upstream thing in the Fedora package and I would like to see support merged upstream. FYI, Fedora (in the near future RHEL), (open)SUSE, Mageia, Mandriva, Arch Linux, NixOS etc use systemd by default and Debian installer has recently added support as well. Besides adding this support does not affect non-systemd distros in any way. If you are not convinced about other patches, adding makefile support for it is trivial. Refer to http://www.freedesktop.org/software/systemd/man/daemon.html

comment:12 Changed 9 years ago by jordan

  • Milestone changed from None Set to 2.80
  • Owner set to jordan
  • Status changed from new to assigned

Rahul, thanks for pinging me about this.

This should be done ASAP, so I'm benchmarking for 2.80.

comment:13 Changed 9 years ago by rahulsundaram

Excellent. Thanks!

comment:14 Changed 9 years ago by Nikoli

  • Cc nikoli@… added

comment:15 Changed 8 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

r14084 adds sd_notify() hints as per zdzichu's patch, after checking for systemd-daemon in pkg-config as suggested in comment #2, rather than including the sd_notify() implementation locally.

r14086 adds a .service file, mostly as suggested by mus and tomegun, but using Type=simple with -f rather than Type=forking as suggested by ssiloti, and with --log-error to dial down transmission's verbosity as suggested by wooptoo.

I'm happy to take suggestions/fixes/refinements to this as necessary. Now that this is going into the tarballs, fixes will cycle through much faster than the initial code drop... :)

comment:16 follow-up: Changed 8 years ago by mus

  1. If you add sd_notify support, why not make use of it in the service file with Type=notify?
  2. --config-dir should not be hardcoded in the service file. If I want to change the user the daemon is running as, I would have to override both User and ExecStart? instead of just User. The user "transmission" should just be assumed to have /var/lib/transmission as its home directory, then $HOME/.config/transmision-daemon would be used as expected (that's also how it works currently in Arch).
  3. The comment that /run/transmission is expected to exist is not true anymore since you don't use a PID file anymore. imho the comment should just say that the transmission user is expected to exist and have /var/lib/transmission as its home directory.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by jordan

Replying to mus:

  1. If you add sd_notify support, why not make use of it in the service file with Type=notify?

Because I'm still learning this and didn't know about Type=notify. Good suggestion; fixed.

  1. --config-dir should not be hardcoded in the service file. If I want to change the user the daemon is running as, I would have to override both User and ExecStart? instead of just User. The user "transmission" should just be assumed to have /var/lib/transmission as its home directory, then $HOME/.config/transmision-daemon would be used as expected (that's also how it works currently in Arch).

Hmmm... not as sure about this, since the other templates provided in-ticket hardcode the config dir. Is there some security value in ensuring it's not left open-ended?

  1. The comment that /run/transmission is expected to exist is not true anymore since you don't use a PID file anymore. imho the comment should just say that the transmission user is expected to exist and have /var/lib/transmission as its home directory.

Yep, agreed.

Points 1 and 3 added in r14091; point 2 is still up for discussion

comment:18 Changed 8 years ago by jordan

Also, any opinion on StandardError=syslog?

comment:19 in reply to: ↑ 17 Changed 8 years ago by mus

Replying to jordan:

Replying to mus:

  1. --config-dir should not be hardcoded in the service file. If I want to change the user the daemon is running as, I would have to override both User and ExecStart? instead of just User. The user "transmission" should just be assumed to have /var/lib/transmission as its home directory, then $HOME/.config/transmision-daemon would be used as expected (that's also how it works currently in Arch).

Hmmm... not as sure about this, since the other templates provided in-ticket hardcode the config dir. Is there some security value in ensuring it's not left open-ended?

yes, tomegun had it hardcoded in his originally proposed service as well, but he agreed that it makes more sense to set it as the home directory. The service file in the Arch Linux package doesn't have this hardcoded. See also: https://projects.archlinux.org/svntogit/packages.git/tree/trunk/transmission.systemd?h=packages/transmission https://bugs.archlinux.org/task/30080

I don't see any security value by hardcoding it either. If you don't override the user, it doesn't make any difference. If you do override the user, the service will fail because the other user doesn't have write permission in /var/lib/transmission. Having to override ExecStart as well is an unnecessary annoyance. Overriding ExecStart also means that if at some point options are changed/added in the original service file, this would not automatically apply to the overriden service. You would have to keep track of this myself.

Replying to jordan:

Also, any opinion on StandardError=syslog?

Everything goes to the journal by default, and - from my understanding - if someone still has a syslog server running, it will be forwarded there anyway. So I don't see what advantage this would have!?

comment:20 Changed 8 years ago by rahulsundaram

That's entirely redundant since it is the default for a long time.

comment:21 Changed 8 years ago by jordan

hardcoded config dir removed in r14093.

What's left in the .service file is pretty basic indeed :)

comment:22 Changed 8 years ago by mus

looks all good to me now, thanks! :)

comment:23 Changed 8 years ago by lkraav

  • Cc leho@… added
Note: See TracTickets for help on using tickets.