systemd service zfs-import-cache.service no longer works correctly

[siebenmann]

As of commit 87abfcb, the ExecStart specified in zfs-import-cache.service is:

ExecStart=/sbin/modprobe zfs && @sbindir@/zpool import -c @sysconfdir@/zfs/zpool.cache -aN

However, systemd ExecStart is not a shell command line; it is a command (or several commands separated by ';'). As a result, the entire ExecStart is handed to modprobe as command line arguments and modprobe errors out (its specific complaint is that it doesn't have a -N argument). Even if modprobe ignored the surplus arguments, this would not run 'zpool import'. I think that the simple way to make this work is to put the modprobe command in an ExecStartPre= statement.

[behlendorf]

I was afraid of that. If you could propose a clean fix that would be great.

[siebenmann]

What seems to work for me is splitting the ExecStart into:

ExecStartPre=/sbin/modprobe zfs
ExecStart=@sbindir@/zpool import -c @sysconfdir@/zfs/zpool.cache -aN

Based on my system and on my reading of systemd.service, this should work right; if the ExecStartPre modprobe fails, the ExecStart is not run and the unit is considered failed (the intent of the existing '&&', or so I assume). It's possible that the modprobe should be started with a '-' so that even if it fails the 'zpool import' will be tried just in the hopes that it will work.

(My sysadmin bias is that ZFS should try relatively hard to bring pools up, because failure to bring pools up is basically a fatal boot failure. But for the first fix I would just split to ExecStartPre/ExecStart without the '-'.)

A similar fix is needed to zfs-import-scan.service.in.

[tycho]

I just ran into this. The solution I used was this:

http://git.uplinklabs.net/snoonan/projects/archlinux/ec2/ec2-packages.git/plain/zfs-git/0001-zfs-import-.service-ExecStart-doesn-t-support.patch

Feel free to pull that patch into the zfs repo if it looks alright.

[behlendorf]

@tycho thanks for the patch. One question, why did you opt to use ExecStart instead of ExecStartPre for the modprobe? I'm no systemd expert so I'm curious, the documentation doesn't seem to make it clear why you'd choose one over the other.

http://www.freedesktop.org/software/systemd/man/systemd.service.html

[behlendorf]

@siebenmann @tycho I opened a pull request, #3444, with your proposed fix if I could get you guys to review and sign off on it we can get this fixed in master.

[tycho]

@behlendorf In this case, I believe ExecStart/ExecStartPre will be functionally the same. I'll pose the question on the systemd-devel mailing list.

http://lists.freedesktop.org/archives/systemd-devel/2015-May/032268.html

[tycho]

There's a reply on the mailing list.

http://lists.freedesktop.org/archives/systemd-devel/2015-May/032269.html

Copying here for posterity.

From: Christian Seiler christian at iwakd.de 
Date: Tue May 26 14:26:11 PDT 2015
Subject: [systemd-devel] Re: ExecStart vs ExecStartPre

On 05/26/2015 11:12 PM, Steven Noonan wrote:
> Hi there,
> 
> I'm wondering what the functional difference is between doing:
> 
> ExecStartPre=/bin/foo
> ExecStart=/bin/bar
> 
> and
> 
> ExecStart=/bin/foo
> ExecStart=/bin/bar
> 
> From my read of the systemd.service man page, they appear to have the
> same behavior in the common use case.

Three differences come directly to mind:

 - If you have unit of type that is NOT oneshot (simple, forking,
   etc.), you can have only a single ExecStart= line, not multiple
   ones. The main service process must be started in ExecStart=
   and not ExecStartPre=.

 - Even in Type=oneshot units you must have at least one ExecStart=
   line (but can in any case have an arbitrary amount of ExecStartPre=
   lines, even zero).

 - If you set PermissionsStartOnly= or RootDirectoryStartOnly=, then
   certain settings will be applied to ExecStart= but not to
   ExecStartPre= (see manpage for details).

(There are probably more.)

Generally speaking, I follow the following guidelines when writing
units:

 - Type=oneshot: I typically use only ExecStart= and only use
   ExecStartPre= if I have to use *StartOnly=true (see above).

 - other types: ExecStart= to start the service proper, ExecStartPre=
   for preparatory things (like generating a default config if none
   is already present or something along those lines)

But it really depends on your use case and as always YMMV.

Hope that helps!

Christian

[behlendorf]

Interesting, well you learn something every day. It seems to me our use case would fall under this sort of general rule. I'm inclined to go with ExecStartPre= if you're both OK with that.

 - other types: ExecStart= to start the service proper, ExecStartPre=
   for preparatory things (like generating a default config if none
   is already present or something along those lines)

[tycho]

Yep. Already signed off on your version in the pull request. 😄