[PATCH] systemd notification support

[PATCH] systemd notification support

[FUJITA Tomonori]

From: Apollon Oikonomopoulos <apoikos@debian.org>

tgtd may take some time to initialize before accepting management
commands. Since management commands are used to bring the targets up
during service startup, we have to make sure that tgtd is responsive
before proceeding. To this end, we add a call to sd_notify(3) right
before entering the event loop to signal systemd (if applicable) that
the main process is ready.

Systemd support with the relevant includes and linker flags is optional,
controlled by the SD_NOTIFY make flag.

Signed-off-by: Apollon Oikonomopoulos <apoikos@debian.org>
---
 Makefile     | 1 +
 usr/Makefile | 8 ++++++++
 usr/tgtd.c   | 4 ++++
 usr/tgtd.h   | 4 ++++
 4 files changed, 17 insertions(+)

diff --git a/Makefile b/Makefile
index eb56bb5..0b5163b 100644
--- a/Makefile
+++ b/Makefile
@@ -13,6 +13,7 @@ export VERSION PREFIX
 export ISCSI_RDMA
 export CEPH_RBD
 export GLFS_BD
+export SD_NOTIFY
 
 .PHONY: all
 all: programs doc conf scripts
diff --git a/usr/Makefile b/usr/Makefile
index 6234f3f..817a3fb 100644
--- a/usr/Makefile
+++ b/usr/Makefile
@@ -21,6 +21,10 @@ ifneq ($(GLFS_BD),)
 MODULES += bs_glfs.so
 endif
 
+ifneq ($(SD_NOTIFY),)
+CFLAGS += -DUSE_SYSTEMD
+endif
+
 ifneq ($(shell test -e /usr/include/sys/eventfd.h && test -e /usr/include/libaio.h && echo 1),)
 CFLAGS += -DUSE_EVENTFD
 TGTD_OBJS += bs_aio.o
@@ -47,6 +51,10 @@ CFLAGS += -DBSDIR=\"$(DESTDIR)$(libdir)/backing-store\"
 
 LIBS += -lpthread -ldl
 
+ifneq ($(SD_NOTIFY),)
+LIBS += -lsystemd-daemon
+endif
+
 PROGRAMS += tgtd tgtadm tgtimg
 TGTD_OBJS += tgtd.o mgmt.o target.o scsi.o log.o driver.o util.o work.o \
 		concat_buf.o parser.o spc.o sbc.o mmc.o osd.o scc.o smc.o \
diff --git a/usr/tgtd.c b/usr/tgtd.c
index 9975e03..23ad413 100644
--- a/usr/tgtd.c
+++ b/usr/tgtd.c
@@ -617,6 +617,10 @@ int main(int argc, char **argv)
 
 	bs_init();
 
+#ifdef USE_SYSTEMD
+	sd_notify(0, "READY=1\nSTATUS=Starting event loop...");
+#endif
+
 	event_loop();
 
 	lld_exit();
diff --git a/usr/tgtd.h b/usr/tgtd.h
index 8a25521..d8b2ac1 100644
--- a/usr/tgtd.h
+++ b/usr/tgtd.h
@@ -5,6 +5,10 @@
 #include "scsi_cmnd.h"
 #include "tgtadm_error.h"
 
+#ifdef USE_SYSTEMD
+#include <systemd/sd-daemon.h>
+#endif
+
 struct concat_buf;
 
 #define NR_SCSI_OPCODES		256
-- 
1.8.5.2 (Apple Git-48)

Re: [PATCH] systemd notification support

[Andy Grover]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On 05/20/2014 07:43 PM, FUJITA Tomonori wrote:
> From: Apollon Oikonomopoulos <apoikos@debian.org>
>
> tgtd may take some time to initialize before accepting management
> commands. Since management commands are used to bring the targets up
> during service startup, we have to make sure that tgtd is responsive
> before proceeding. To this end, we add a call to sd_notify(3) right
> before entering the event loop to signal systemd (if applicable) that
> the main process is ready.
>
> Systemd support with the relevant includes and linker flags is optional,
> controlled by the SD_NOTIFY make flag.
>
> Signed-off-by: Apollon Oikonomopoulos <apoikos@debian.org>

Hi Apollon,

Fedora tgtd pkg maintainer here.

Better systemd integration seems like a good way to go. BTW I feel deep 
shame about the systemd service file that Fedora is currently using 
(attached), especially the sleep. Does this or another change perhaps 
result in a better way to integrate tgt with systemd?

Do we need two service files or something?

It would be great if tgt could use the same service file(s) on all 
systemd-enabled distros, and these could be included with tgtd.

Regards -- Andy


[-- Attachment #2: tgtd.service --]
[-- Type: text/plain, Size: 1229 bytes --]

[Unit]
Description=tgtd iSCSI target daemon
After=network.target

[Service]
EnvironmentFile=/etc/sysconfig/tgtd

ExecStart=/usr/sbin/tgtd -f $TGTD_OPTS
# see bz 848942. workaround for a race for now.
ExecStartPost=/bin/sleep 5
# Put tgtd into "offline" state until all the targets are configured.
# We don't want initiators to (re)connect and fail the connection
# if it's not ready.
ExecStartPost=/usr/sbin/tgtadm --op update --mode sys --name State -v offline
# Configure the targets.
ExecStartPost=/usr/sbin/tgt-admin -e -c $TGTD_CONFIG
# Put tgtd into "ready" state.
ExecStartPost=/usr/sbin/tgtadm --op update --mode sys --name State -v ready

# Update configuration for targets. Only targets which
# are not in use will be updated.
ExecReload=/usr/sbin/tgt-admin --update ALL -c $TGTD_CONFIG

# NOTE: Shutdown of the iscsi target may cause data corruption
# for initiators that are connected.
ExecStop=/usr/sbin/tgtadm --op update --mode sys --name State -v offline
# Remove all targets. It only removes targets which are not in use.
ExecStop=/usr/sbin/tgt-admin --update ALL -c /dev/null
# tgtd will exit if all targets were removed
ExecStop=/usr/sbin/tgtadm --op delete --mode system

[Install]
WantedBy=multi-user.target

Re: [PATCH] systemd notification support

[Apollon Oikonomopoulos]

Hi Andy,

On 14:00 Wed 21 May     , Andy Grover wrote:
> Hi Apollon,
> 
> Fedora tgtd pkg maintainer here.
> 
> Better systemd integration seems like a good way to go. BTW I feel deep
> shame about the systemd service file that Fedora is currently using
> (attached), especially the sleep. Does this or another change perhaps result
> in a better way to integrate tgt with systemd?

The sleep statement is precisely what I wanted to get rid of when I was 
writing our own service file[1]. I've currently set it to an 
aggressive-but-works-for-me value of 0.2 seconds but I really don't like 
it at all.

When the service type is set to `notify', systemd will wait for the main 
process specified by ExecStart to send a readiness notification before 
proceeding with any ExecStartPost directives and so it seems to be the 
"correct" way to solve the race. Since the modifications required to use 
sd_notify(3) are really minor and the interface very simple, I think 
it's worth enabling it.

A working alternative would be to bind the IPC unix socket in systemd 
and pass it down to tgtd using socket activation and sd_listen_fds(3), 
but this requires more extensive patching of tgtd and is IMHO a bit of 
an ugly workaround, because the IPC interface is meant to be internal to 
tgt.

> Do we need two service files or something?

Without readiness notification, systemd assumes that the service is 
ready as soon as the process has been spawned (and could really make no 
other assumption in a reliable manner). This means that having two 
services with a dependency wouldn't solve the race either. Apart from 
the issue at hand, the two services idea has occurred to me as well, but 
I see no real advantage in e.g. being able to stop just the targets 
without stopping tgtd. Do you have a specific use-case in mind?

> 
> It would be great if tgt could use the same service file(s) on all
> systemd-enabled distros, and these could be included with tgtd.

I totally agree. Seeing that our service files are similar[1] (with the 
exception of us forcing all targets cleared during shutdown), I don't 
think that's something impossible to achieve.

Regards,
Apollon

[1] http://anonscm.debian.org/gitweb/?p=collab-maint/tgt.git;a=blob;f=debian/tgt.service;h=00232c4fd308d68288c75d23af515984aca394bf;hb=HEAD

Re: [PATCH] systemd notification support

[FUJITA Tomonori]

On Wed, 21 May 2014 11:43:27 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> From: Apollon Oikonomopoulos <apoikos@debian.org>
> 
> tgtd may take some time to initialize before accepting management
> commands. Since management commands are used to bring the targets up
> during service startup, we have to make sure that tgtd is responsive
> before proceeding. To this end, we add a call to sd_notify(3) right
> before entering the event loop to signal systemd (if applicable) that
> the main process is ready.
> 
> Systemd support with the relevant includes and linker flags is optional,
> controlled by the SD_NOTIFY make flag.
> 
> Signed-off-by: Apollon Oikonomopoulos <apoikos@debian.org>
> ---
>  Makefile     | 1 +
>  usr/Makefile | 8 ++++++++
>  usr/tgtd.c   | 4 ++++
>  usr/tgtd.h   | 4 ++++
>  4 files changed, 17 insertions(+)

Applied, thanks a lot!

Re: [PATCH] systemd notification support

[FUJITA Tomonori]

On Thu, 22 May 2014 01:00:24 +0300
Apollon Oikonomopoulos <apoikos@debian.org> wrote:

> > It would be great if tgt could use the same service file(s) on all
> > systemd-enabled distros, and these could be included with tgtd.
> 
> I totally agree. Seeing that our service files are similar[1] (with the 
> exception of us forcing all targets cleared during shutdown), I don't 
> think that's something impossible to achieve.

Integrating nicely with systemd sounds great. Looking forward to
the further patches ;)