Use systemd service manager notification

From: Christian Hesse <[email protected]...>

Notify systemd service manager when our initialization sequence completed. This helps ordering services as dependencies can rely on vpn being available.

---
 distro/systemd/[email protected]... | 1 +
 distro/systemd/[email protected]... | 1 +
 src/openvpn/init.c                     | 6 ++++++
 src/openvpn/init.h                     | 4 ++++
 4 files changed, 12 insertions(+)

distro/systemd/[email protected]... +1 -0

index 18b84dd..f64a239 100644
@@ -7,6 +7,7 @@ Documentation=https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage
 Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO
 
 [Service]
+Type=notify
 PrivateTmp=true
 RuntimeDirectory=openvpn-client
 RuntimeDirectoryMode=0710

distro/systemd/[email protected]... +1 -0

index a2b7b52..890e6a9 100644
@@ -7,6 +7,7 @@ Documentation=https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage
 Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO
 
 [Service]
+Type=notify
 PrivateTmp=true
 RuntimeDirectory=openvpn-server
 RuntimeDirectoryMode=0710

src/openvpn/init.c +6 -0

index 2ccbab2..551e579 100644
@@ -1251,10 +1251,16 @@ initialization_sequence_completed (struct context *c, const unsigned int flags)
       show_adapters (M_INFO|M_NOPREFIX);
       msg (M_INFO, "%s With Errors ( see http://openvpn.net/faq.html#dhcpclientserv )", message);
 #else
+#ifdef ENABLE_SYSTEMD
+      sd_notifyf(0, "STATUS=Failed to start up: %s With Errors\nERRNO=1", message);
+#endif /* HAVE_SYSTEMD_SD_DAEMON_H */
       msg (M_INFO, "%s With Errors", message);
 #endif
     }
   else
+#ifdef ENABLE_SYSTEMD
+    sd_notifyf(0, "READY=1\nSTATUS=%s\nMAINPID=%lu", message, (unsigned long) getpid());
+#endif
     msg (M_INFO, "%s", message);
 
   /* Flag that we initialized */

src/openvpn/init.h +4 -0

index 524bc64..0518b06 100644
@@ -27,6 +27,10 @@
 
 #include "openvpn.h"
 
+#ifdef ENABLE_SYSTEMD
+#include <systemd/sd-daemon.h>
+#endif
+
 /*
  * Baseline maximum number of events
  * to wait for.

Quoted 5 lines

From: Christian Hesse <[email protected]...>

Notify systemd service manager when our initialization sequence completed. This helps ordering services as dependencies can rely on vpn being available.

Funny detail is that I have a somewhat similar patch in a local git tree, awaiting proper testing ... I postponed it as this is not something we will pull into v2.4. We're going to release 2.4_rc1 this week, and that is too late for more intrusive changes (even though the changeset itself is small, the code changes makes OpenVPN behave somewhat different when managed by systemd).

Just a question, as it is good to see more people looking into these code paths ... I was considering to extend my approach to update STATUS= a bit more frequently. On the client side, I thought it would be good if the status line had "Resolving %s", "Connecting to %s", "Successful connection to %s" or "Failed to connect to %s". On the server side I was pondering on a "Successfully started, %i clients connected". What do you think about that? Does the sd_notify() API support more frequent updates?

Also when using Type=notify ... does systemd expect the OpenVPN process to fork into the background or run in the foreground as now?

Ok, lets go into detail. We can use three different settings: Type=simple, Type=forking and Type=notify.

* We used Type=forking for a long time. That is fine: systemd reports success when the process forks off first time. That is when openvpn successfully completed initialization sequence.

* The current systemd unit use Type=simple (which is implicit). systemd reports success as soon as the process is executed, it does not wait for anything. So startup can look like that: systemd starts openvpn process -> unit is in state 'started' -> openvpn bails out with an error before the initialization sequence completed -> systemd unit is in state 'failed' now. The problem is that it was in state 'started'
intermittently: Manual systemctl (starting service from command line) reports success, other services depending on openvpn are started while dependency failed later, ... This is just broken.

* My patch introduces Type=notify. The (main) process must not fork, so most things work like simple, except that systemd does not report success on process execution, but waits for the sd_notify() call. We do not have intermittent state 'success' and everything works as expected.

I will not package the code as-is with our Arch Linux package. Either I revert back to Type=forking or apply the patch for Type=notify.

So I still vote to apply this as soon as possible.

src/openvpn/init.c +0 -0

@@ -926,6 +926,13 @@ bool
 possibly_become_daemon (const struct options *options)
 {
   bool ret = false;

Hi,

Quoted 12 lines · Christian Hesse

src/openvpn/init.c +6 -0

@@ -926,6 +926,13 @@ bool
 possibly_become_daemon (const struct options *options)
 {
   bool ret = false;
+
+#ifdef ENABLE_SYSTEMD
+  /* return without forking if we are running from systemd */
+  if (sd_notify(0, "READY=0") > 0)
+    return ret;
+#endif

src/openvpn/init.c bool · line 938

Quoted 4 lines

src/openvpn/init.c +1 -0

+
   if (options->daemon)
     {
       ASSERT (!options->inetd);  

Does this mean I cannot run openvpn --config bla.conf --daemon from the command line any more on a systemd system? This would be a deal-breaker for me.

Steffan Karger <[email protected]...> on Wed, 2016/11/30 10:06:

Quoted 15 lines

Hi,

Quoted 12 lines · Christian Hesse

src/openvpn/init.c +6 -0

@@ -926,6 +926,13 @@ bool
 possibly_become_daemon (const struct options *options)
 {
   bool ret = false;
+
+#ifdef ENABLE_SYSTEMD
+  /* return without forking if we are running from systemd */
+  if (sd_notify(0, "READY=0") > 0)
+    return ret;
+#endif

Quoted 8 lines

src/openvpn/init.c bool · line 938

Quoted 4 lines

src/openvpn/init.c +1 -0

+
   if (options->daemon)
     {
       ASSERT (!options->inetd);  

Does this mean I cannot run openvpn --config bla.conf --daemon from the command line any more on a systemd system? This would be a deal-breaker for me.

No. That means openvpn knows when it is run from within a system service. Daemonization is refused there as we we start with "Type=notify".

sd_notify() is a no-op when run from command line. So everything works as usual.

Steffan Karger <[email protected]...> on Wed, 2016/11/30 10:06:

Quoted 15 lines

Hi,

Quoted 12 lines · Christian Hesse

src/openvpn/init.c +6 -0

@@ -926,6 +926,13 @@ bool
 possibly_become_daemon (const struct options *options)
 {
   bool ret = false;
+
+#ifdef ENABLE_SYSTEMD
+  /* return without forking if we are running from systemd */
+  if (sd_notify(0, "READY=0") > 0)
+    return ret;
+#endif

Quoted 8 lines

src/openvpn/init.c bool · line 938

Quoted 4 lines

src/openvpn/init.c +1 -0

+
   if (options->daemon)
     {
       ASSERT (!options->inetd);  

Does this mean I cannot run openvpn --config bla.conf --daemon from the command line any more on a systemd system? This would be a deal-breaker for me.

No. That means openvpn knows when it is run from within a system service. Daemonization is refused there as we we start with "Type=notify".

sd_notify() is a no-op when run from command line. So everything works as usual.

From: Christian Hesse <[email protected]...>

We start with systemd Type=notify, so refuse to daemonize.

Signed-off-by Christian Hesse <[email protected]...>

---
 distro/systemd/[email protected]... | 1 -
 distro/systemd/[email protected]... | 1 -
 src/openvpn/init.c                     | 7 +++++++
 3 files changed, 7 insertions(+), 2 deletions(-)

distro/systemd/[email protected]... +0 -1

index f64a239..5618af3 100644
@@ -12,7 +12,6 @@ PrivateTmp=true
 RuntimeDirectory=openvpn-client
 RuntimeDirectoryMode=0710
 WorkingDirectory=/etc/openvpn/client
-ExecStartPre=/bin/sh -c 'grep -q -E ^daemon %i.conf || exit 0 && /usr/bin/echo "OpenVPN configuration cannot contain --daemon when being managed by systemd" ; exit 1'
 ExecStart=/usr/sbin/openvpn --suppress-timestamps --nobind --config %i.conf
 CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10

distro/systemd/[email protected]... +0 -1

index 890e6a9..b9b4dba 100644
@@ -12,7 +12,6 @@ PrivateTmp=true
 RuntimeDirectory=openvpn-server
 RuntimeDirectoryMode=0710
 WorkingDirectory=/etc/openvpn/server
-ExecStartPre=/bin/sh -c 'grep -q -E ^daemon %i.conf || exit 0 && /usr/bin/echo "OpenVPN configuration cannot contain --daemon when being managed by systemd" ; exit 1'
 ExecStart=/usr/sbin/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
 CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10

src/openvpn/init.c +0 -0

index 551e579..7ab5c52 100644
@@ -926,6 +926,13 @@ bool
 possibly_become_daemon (const struct options *options)
 {
   bool ret = false;

Quoted 44 lines

From: Christian Hesse <[email protected]...>

We start with systemd Type=notify, so refuse to daemonize.

Signed-off-by Christian Hesse <[email protected]...>

---
 distro/systemd/[email protected]... | 1 -
 distro/systemd/[email protected]... | 1 -
 src/openvpn/init.c                     | 7 +++++++
 3 files changed, 7 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/distro/systemd/[email protected]... index f64a239..5618af3 100644

distro/systemd/[email protected]... +0 -1

@@ -12,7 +12,6 @@ PrivateTmp=true
 RuntimeDirectory=openvpn-client
 RuntimeDirectoryMode=0710
 WorkingDirectory=/etc/openvpn/client
-ExecStartPre=/bin/sh -c 'grep -q -E ^daemon %i.conf || exit 0

&& /usr/bin/echo "OpenVPN configuration cannot contain --daemon when being managed by systemd" ; exit 1' ExecStart=/usr/sbin/openvpn --suppress-timestamps --nobind --config %i.conf CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE LimitNPROC=10 diff --git a/distro/systemd/[email protected]... b/distro/systemd/[email protected]... index 890e6a9..b9b4dba 100644 --- a/distro/systemd/[email protected]... +++ b/distro/systemd/[email protected]... @@ -12,7 +12,6 @@ PrivateTmp=true RuntimeDirectory=openvpn-server RuntimeDirectoryMode=0710
WorkingDirectory=/etc/openvpn/server
-ExecStartPre=/bin/sh -c 'grep -q -E ^daemon %i.conf || exit 0 && /usr/bin/echo "OpenVPN configuration cannot contain --daemon when being managed by systemd" ; exit 1' ExecStart=/usr/sbin/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE LimitNPROC=10 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 551e579..7ab5c52 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -926,6 +926,13 @@ bool
possibly_become_daemon (const struct options *options)

 {
   bool ret = false;

distro/systemd/[email protected]...

Quoted 10 lines

distro/systemd/[email protected]... +7 -0

+
+#ifdef ENABLE_SYSTEMD
+  /* return without forking if we are running from systemd */
+  if (sd_notify(0, "READY=0") > 0)
+    return ret;
+#endif
+
   if (options->daemon)
     {
       ASSERT (!options->inetd);

NAK on this approach. We cannot dictate that users _must_ start OpenVPN as a daemon via systemd if it has been built with systemd support.

I understand the sentiment for this change, but we need to ensure users may use their own scripts and hand-crafted configs to start OpenVPN, also if systemd is present.

On failure, these calls return a negative errno-style error code. If $NOTIFY_SOCKET was not set and hence no status data could be sent, 0 is returned. If the status was sent, these functions return with a positive return value. In order to support both, init systems that implement this scheme and those which do not, it is generally recommended to ignore the return value of this call.