Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a30e440
Add functionality to notify systemd on olad startup and config reload
shenghaoyang Jul 12, 2018
a5e57fe
Add systemd notification support to olad v2
shenghaoyang Jul 14, 2018
183ac8d
Fix license and doxygen warnings.
shenghaoyang Jul 14, 2018
693fe33
Merge branch 'master' into add_notify
shenghaoyang Jul 15, 2018
0257949
Merge branch 'master' into add_notify
shenghaoyang Jul 16, 2018
50c9bda
Style fixups for commits to add systemd notification functionality
shenghaoyang Jul 16, 2018
29556d0
Add additional checks and error messages when configuring for systemd
shenghaoyang Jul 16, 2018
b7afd38
Style changes for function names and log messages
shenghaoyang Jul 25, 2018
83807d4
Enable systemd support by default on supported build systems
shenghaoyang Jul 25, 2018
469361d
Add comment clarifying usage of strerror_r
shenghaoyang Jul 25, 2018
f15b385
Add a declaration for XSI-compliant strerror_r
shenghaoyang Jul 25, 2018
d674930
Fix formatting issues in log messages
shenghaoyang Jul 28, 2018
bf8deff
Move strerror_r wrapper into utility library
shenghaoyang Jul 28, 2018
175b393
Add new wrapper function for strerror_r and update naming
shenghaoyang Aug 9, 2018
17d917c
Merge branch 'master' into add_notify
shenghaoyang Aug 9, 2018
1cd1202
Refactor StrError_R() and fix doxygen references.
shenghaoyang Aug 10, 2018
64400b8
Merge branch 'master' into add_notify
shenghaoyang Aug 11, 2018
d62a8f6
Fix formatting, style, and spelling, refactor StrError_R
shenghaoyang Aug 12, 2018
ff34724
Add config error on being unable to explicitly enable systemd support
shenghaoyang Aug 12, 2018
f6f5813
Switch to internal ola library function for int to string conversion
shenghaoyang Aug 12, 2018
15efd6f
Merge branch 'master' into add_notify
shenghaoyang Aug 19, 2018
18735c4
Merge branch 'master' into add_notify
shenghaoyang Aug 25, 2018
a21f0a4
Add systemd unit file for running olad with systemd in user mode
shenghaoyang Aug 25, 2018
c714ee8
Add more documentation URLs to the unit file for the user instance.
shenghaoyang Sep 10, 2018
527b115
Merge branch 'master' into add_notify
shenghaoyang Sep 11, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,21 @@ AC_ARG_ENABLE(
[AS_HELP_STRING([--disable-doxygen-version],
[Substitute the Doxygen version with latest, for the website])])

# systemd support
AC_ARG_ENABLE(
[systemd],
[AS_HELP_STRING([--enable-systemd], [Enable systemd support])])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to negate this, so it's turned on by default where available (or no-one will realise it exists/use it). See e.g. https://github.com/OpenLightingProject/ola/blob/master/configure.ac#L573-L589 and don't forget to update the AS_IF below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eek.. an automatic dependency feels a little weird to me, unless we put it out for people to know. If we put the existence of this option in NEWS (which is probably where it will end up in) and you suspect:

no-one will realise it exists/use it

then, if we put in a warning that libsystemd is going to become an automatic dependency, I kinda also think that

no-one will realise it exists/use it

until bug reports come in regarding libsystemd not being found on non-build systems...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we have the best of both worlds with our plugins, a literal auto mode, so by default if the bits are there, you get your plugin and if they aren't you don't, but you can also force it on (so it complains if dependencies are missing) or force it off (so it doesn't use it even if present).

You may be right about bug reports, I guess I'd tend towards the nudge theory, if we turn it on, then people who read the docs will turn it off easily, those who don't will ideally search or file a bug/ask on the mailing list and there's a workaround.

If we leave it off, we should at least enable it in our Debian build config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, alright. Let's default it to enabled. We're going to be playing devil's advocate (to some people out there) and silently promote systemd adoption...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 83807d4


have_libsystemd="no"
AS_IF([test "x$enable_systemd" = "xyes"],
[PKG_CHECK_MODULES([libsystemd], [libsystemd >= 38], [have_libsystemd="yes"])
AC_CHECK_HEADER([systemd/sd-daemon.h], [], [have_libsystemd="no"])])

AS_IF([test "x$have_libsystemd" = xyes],
[AC_DEFINE([HAVE_LIBSYSTEMD], [1], [define if systemd support wanted])])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd probably say this is if systemd support present (or present and wanted).


AM_CONDITIONAL([HAVE_LIBSYSTEMD], [test "x$have_libsystemd" = xyes])

# UUCP Lock directory
AC_ARG_WITH([uucp-lock],
[AS_HELP_STRING([--with-uucp-lock],
Expand Down
5 changes: 5 additions & 0 deletions olad/Makefile.mk
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ ola_server_sources += olad/HttpServerActions.cpp \
ola_server_additional_libs += common/http/libolahttp.la
endif

if HAVE_LIBSYSTEMD
ola_server_sources += olad/NotifySystemd.cpp olad/Systemd.h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally match .h and .cpp filenames.

ola_server_additional_libs += $(libsystemd_LIBS)
endif

# lib olaserver
lib_LTLIBRARIES += olad/libolaserver.la

Expand Down
51 changes: 51 additions & 0 deletions olad/NotifySystemd.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Library General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*
* NotifySystemd.cpp
* Provides wrapped access to the systemd notification interface.
* Copyright (C) 2018 Shenghao Yang
*/

#if HAVE_CONFIG_H
#include <config.h>
#endif // HAVE_CONFIG_H

#if HAVE_LIBSYSTEMD
#include <systemd/sd-daemon.h>
#include <errno.h>
#include <string.h>
#endif // HAVE_LIBSYSTEMD

#include "ola/Logging.h"

#include "olad/Systemd.h"

namespace ola {

int notify_systemd(int unset_environment, const char *state) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We go CamelCase for our method names.

int rtn = sd_notify(unset_environment, state);
if (rtn < 0) {
char buf[1024];
OLA_WARN << "Error sending notification to systemd: " <<
strerror_r(-rtn, buf, sizeof(buf));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll only print the error code from strerror_r currently. Looking at https://linux.die.net/man/3/strerror_r it seems you should be able to just do << strerror(-rtn).

Can you also move the second << to the next line and align with the one above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think you should add a configure check for strerror_r (see https://github.com/OpenLightingProject/ola/blob/master/configure.ac#L185 and e.g. https://github.com/OpenLightingProject/ola/blob/master/common/base/Credentials.cpp#L342 ), it's the first time it's being used in the codebase and we run on a variety of weird platforms. See also some similar fun and discussion with readdir_r #1055 .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll only print the error code from strerror_r currently. Looking at https://linux.die.net/man/3/strerror_r it seems you should be able to just do << strerror(-rtn).

I saw that OLA was compiling with GNU extensions - in that case strerror_r returns a pointer to either an impelemtation-allocated array or a pointer to the buffer that's passed in. We always have to use the returned pointer to reference the error message. I could mangle the defines a little bit and get the POSIX compatible one, but I'd like to confirm the usage of GNU extensions - I dug through the configure file and found loads of references to std=gnu++XX instead of std=c++XX.

Yeah, I'll be able to do a simple strerror() call, but I'm a little concerned that the other threads could be calling into it at the same time, because (I think) some of the plugins do spawn threads for their own use - I see a bunch of threads under htop for olad. Unless they don't ever call strerror(), then it's unsafe to call strerror() at this site.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, in which case can we just have a comment for our future selves if we switch away from the GNU extensions in future.

Well currently multiple plugins call strerror (and some do indeed spawn their own threads, at least one call is in a spawned thread), so we may have that issue already.

We should probably be moving to strerror_r throughout, but equally I wouldn't want this one usage to break the build on machines that don't have it, or by adding it to configure stop it building on those OSes.

Copy link
Contributor Author

@shenghaoyang shenghaoyang Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, in which case can we just have a comment for our future selves if we switch away from the GNU extensions in future.

Sure, alright.

We should probably be moving to strerror_r throughout, but equally I wouldn't want this one usage to break the build on machines that don't have it, or by adding it to configure stop it building on those OSes.

Any modern system with systemd should have this - deliberately using strerror() feels pretty icky. Maybe we can take a page from nginx: they call strerror() in a section where it is known that no other thread can call strerror() and caches that for use.

Have a look at their implementation: https://github.com/nginx/nginx/blob/f8a9d528df92c7634088e575e5c3d63a1d4ab8ea/src/os/unix/ngx_errno.c

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 469361d

}
return rtn;
}

bool notify_available() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again

return (sd_notify(0, "") != 0);
}

} // namespace ola
10 changes: 10 additions & 0 deletions olad/OlaServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@
#include "olad/OladHTTPServer.h"
#endif // HAVE_LIBMICROHTTPD

#ifdef HAVE_LIBSYSTEMD
#include "olad/Systemd.h"
#endif // HAVE_LIBSYSTEMD

DEFINE_s_uint16(rpc_port, r, ola::OlaServer::DEFAULT_RPC_PORT,
"The port to listen for RPCs on. Defaults to 9010.");
DEFINE_default_bool(register_with_dns_sd, true,
Expand Down Expand Up @@ -472,9 +476,15 @@ bool OlaServer::InternalNewConnection(
}

void OlaServer::ReloadPluginsInternal() {
#ifdef HAVE_LIBSYSTEMD
ola::notify_systemd(0, "RELOADING=1\nSTATUS=Reloading plugins\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the socket have gone away by the time this gets fired though? Potentially minutes or hours/days later?

My previous comment was more along the lines of if I'm trying to troubleshoot why my reload isn't getting to systemd, I'd want to see (at info/debug level) the positive return code from calling sd_notify, so I know it's the systemd end I need to look at, and not OLA, or vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the socket have gone away by the time this gets fired though? Potentially minutes or hours/days later?

Well, I'm not sure that this could even happen, unless someone removes the environment variable. The socket is actually passed as a socket address (path name or name in abstract namespace, a-la unix domain sockets), and sd_notify establishes a new socket sending data to that address, closing it on every return.

If systemd did somehow... fail, I think the whole system would be going up in flames, by then. But if it did happen and nobody is listening on the socket address, then the sendmsg() call in sd_notify() will fail and the error will be logged through OLA_WARN.

The only way for the notify_systemd call to fail silently is the removal of the NOTIFY_SOCKET environment variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, fair enough.

#endif // HAVE_LIBSYSTEMD
OLA_INFO << "Reloading plugins";
StopPlugins();
m_plugin_manager->LoadAll();
#ifdef HAVE_LIBSYSTEMD
ola::notify_systemd(0, "READY=1\nSTATUS=Plugin reload complete\n");
#endif // HAVE_LIBSYSTEMD
}

void OlaServer::UpdatePidStore(const RootPidStore *pid_store) {
Expand Down
14 changes: 14 additions & 0 deletions olad/Olad.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@
// which needs to be after WinSock2.h, hence this order
#include "olad/OlaDaemon.h"

#if HAVE_LIBSYSTEMD
#include "olad/Systemd.h"
#endif // HAVE_LIBSYSTEMD

#include "ola/Logging.h"
#include "ola/base/Credentials.h"
#include "ola/base/Flags.h"
Expand Down Expand Up @@ -171,6 +175,16 @@ int main(int argc, char *argv[]) {
ola::NewCallback(olad->GetOlaServer(), &ola::OlaServer::ReloadPlugins));
#endif // _WIN32

#if HAVE_LIBSYSTEMD
if (ola::notify_available()) {
OLA_INFO << "Systemd notification socket present. Sending notifications.";
} else {
OLA_WARN << "Systemd notification socket not present";
}
// Does not need to be guarded. sd_notify does its own internal check on
// the socket's presence, as well.
ola::notify_systemd(0, "READY=1\nSTATUS=Startup complete\n");
#endif // HAVE_LIBSYSTEMD
olad->Run();
return ola::EXIT_OK;
}
46 changes: 46 additions & 0 deletions olad/Systemd.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Library General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Systemd.h
* Interface to systemd functionality.
* Copyright (C) 2018 Shenghao Yang
*/

#ifndef OLAD_SYSTEMD_H_
#define OLAD_SYSTEMD_H_

namespace ola {

/*
* @brief Notify systemd about daemon state changes.
*
* This function logs on failures to queue notifications, but only if the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SPaG "logs failures to queue" or "logs on failure to queue"

* notification socket environment variable is set.
*
* @param unset_environment whether to unset the notification socket environment
* variable so child processes cannot utilize it.
* @param state state block to pass to systemd.
* @return value returned from \ref sd_notify()
*/
int notify_systemd(int unset_environment, const char *state);

/*
* @brief Tests whether the systemd notification socket is available.
* @return @c true if the socket is available, @c false if not.
*/
bool notify_available();

} // namespace ola
#endif // OLAD_SYSTEMD_H_