log: Add configure-time option to use systemd journal instead of syslog #327

chrissie-c · 2018-10-02T11:39:05Z

As most of this patch is in the config system I'm hoping some nice person will fix it up for me - it does (seem to ) work, but I suspect it's not optimal.

The actual code is trivial.

jfriesse · 2018-10-02T12:53:30Z

configure.ac


+if test "x$enable_systemd_journal" = "xyes" ; then
+  AX_SAVE_FLAGS
+  AC_SEARCH_LIBS([sd_journal_send], [systemd-journal],,[AC_MSG_ERROR([cannot find sd_journal_send() function])])


I would recommend using PKG_CHECK_MODULES (because it sets correctly not only the LIBS but also the CFLAGS.

Also (without deep knowledge about history) this code seems to work for CentOS 7, but Fedora 28+ seems not to have systemd-journal library at all and instead it's using libsystemd (https://www.freedesktop.org/software/systemd/man/sd-journal.html)

jfriesse · 2018-10-04T10:21:23Z

configure.ac

+  if test "x$have_systemd" = "xyes" ; then
+     AC_DEFINE_UNQUOTED(USE_JOURNAL, 1, [Use systemd journal logging])
+     USE_JOURNAL="yes"
+     echo "CC: found systemd & journal requested"


This debug print should go or be replaced by something without CC: prefix :)

err yes. That should never have been left in. TBH I only pushed it to move a copy to my Fedora system for testing!

jfriesse · 2018-10-04T10:39:01Z

examples/Makefile.am

 mapnotify_SOURCES = mapnotify.c $(top_builddir)/include/qb/qbmap.h
 mapnotify_CPPFLAGS = -I$(top_builddir)/include -I$(top_srcdir)/include
-mapnotify_LDADD = $(top_builddir)/lib/libqb.la
+mapnotify_LDADD = $(top_builddir)/lib/libqb.la $(SYSTEMD_LIBS)


I don't think this adding $(SYSTEMD_LIBS) everywhere is good idea (and if it really is, then libqb.pc must be changed).

What about changing lib/Makefile.am and add $(SYSTEMD_CFLAGS) and $(SYSTEMD_LIBS) to libqb_la_CFLAGS and libqb_la_LIBADD? Then libqb.la gets dependency on systemd (when needed) and apps using libqb will need no change? (please correct me if I'm wrong, I'm not libtool magic master).

Possibly. I'm struggling with auto-stuff too, It will definitely need to go in the pkgfile somehow

wferi

I find making the systemd journal a syslog replacement too heavy handed, because it disallows using the same binary under different init systems. And init system choice is a system configuration question in Debian at least.

wferi · 2018-10-05T13:58:18Z

configure.ac

+fi
+
+# Add to LIBS for pkgconfig file
+LIBS+="$SYSTEMD_LIBS"


I think it would be cleaner to add @SYSTEMD_LIBS@ to Libs.private instead. Changing LIBS adds the library to every link command.

wferi · 2018-10-05T13:59:19Z

configure.ac

 	[ SOCKETDIR="$localstatedir/run" ])

+AC_ARG_ENABLE([systemd-journal],
+  [AS_HELP_STRING([--with-systemd-journal=DIR],[Use systemd journal instead of syslog])])


The =DIR part seems superfluous here.

oops, copy/paste laziness.

chrissie-c · 2018-10-05T14:34:11Z

Thanks for that - it never occurred to me that both might be available. Not being a Debian user. Do you have any suggestions on how it might be made switchable?

Having said that, it would need the systemd libs to be dragged in even on a non-systemd installation if it was made run-time switchable - which I suspect would be less than popular given some of the opinions on systemd! In such circumstances it might just be easier to build for syslog and leave it at that. It will still work on systemd systems but you wouldn't get the logging metadata of course?

I'm interested in opinions here.

kgaillot · 2018-10-05T15:01:35Z

Maybe overkill, but it would be possible to add build-time support for run-time selection. I.e. a configure option for system log that could be set to syslog, systemd, or both, and if both, that would enable a run-time option to select syslog or systemd. Distros could pick one option or even offer multiple variants to get around the dependency issue. (I hadn't thought about that before, but a distro like Debian could even offer a pacemaker-systemd package that depended on libqb-systemd, with systemd support enabled in both, and non-systemd packages without that support.)

wferi · 2018-10-05T15:22:18Z

Cleanest would be to make it an API choice in addition to file and syslog. The obvious drawback is that all applications would have to parse and act on an additional config option, so you get to_file, to_syslog and to_journal for Corosync. The ugly solution is to do a runtime selection based on an environment variable. The additional small library dependency is not a problem.

chrissie-c · 2018-10-08T09:23:41Z

Thanks. So if you're OK with libqb always linking with systemd-journal (depending on the configure flag of course) then I'll add a flag to qb_log_ctl that allows the application to choose syslog or journald. The default will be syslog.

wferi · 2018-10-08T10:44:23Z

That would be perfectly fine from the library packager's point of view (let's hope I don't forget to bump the symbol version info for this ABI extension). Yeah, it's a little awkward to pull in a new library for a single function, but requiring applications to pass in the address of sd_journal_send() would be even more so...
Please note that the current name of the unified library is libsystemd, with oldish systems requiring a fallback to libsystemd-journal.

jfriesse · 2018-10-08T11:08:57Z

@wferi Current code is already using unified libsystemd. IMHO we don't need to try to keep backwards compatibility with libsystemd-journal (I found it in Debian wheezy (already unsupported), jessie (only for some architectures), and in RHEL library still exists but only to keep "compatibility" and main library is libststemd (since 7.2 which is already EOL)).

wferi · 2018-10-08T14:37:37Z

@jfriesse Even better. I remembered the original patch using the legacy library, and Chrissie mentioned it again in her latest comment, so I decided to mention it again. I'm all for forgetting about the old name if it's acceptable for you.

chrissie-c · 2018-10-08T14:41:06Z

it was just a slip of the keyboard really, partly also cause by the fact that my desktop is running RHEL7 ;)

chrissie-c · 2018-10-16T08:55:20Z

I've force committed an updated patch that should have tidied everything up. see what you think

wferi · 2018-10-16T10:10:00Z

All those conditionals in the log_syslog.c functions aren't too pretty; what's the reason for not introducing a new QB_LOG_JOURNAL static target slot? You could define the single needed method in a log_journal.c conditional module without further compile- and runtime conditionals (apart from the initialization stuff). Maybe not very useful, but this would also enable using syslog and journal loggers concurrently.

wferi · 2018-10-16T10:11:15Z

tests/check_log.c

+/*
+ * You can still use syslog and journal in a normal application,
+ * but the syslog_override code doesn't work when -lsystemd is present
+ */


Is this some load order problem?

I tried moving the link order around with no luck. TBH I don't care enough to spend time on it as it's only a test program. If you do, then I'm happy to take patches ;-)

chrissie-c · 2018-10-16T10:11:44Z

because that's what seemed to be the consensus in earlier discussions?

chrissie-c · 2018-10-16T10:17:22Z

But also, I don't see the point in having both available at the same time. If systemd is installed then sending to syslog AND the journal is sending the message to the same place twice, it's just that one has the metadata and one doesn't.

wferi · 2018-10-16T11:28:26Z

I'm sorry for the confusion, it's entirely my fault: I overestimated the scope of qb_log_ctl(). In my opinion the implementation is crying for a new target type, both for efficiency and cleanliness, but I acknowledge that I may miss several points and our tastes may well differ as well. You're also right that having both active would send the messages to the same place twice in 100% of the actual configurations, though that's a question of system configuration (policy), and keeping policy out of libraries is considered good design. And there's also that remaining 0% of setups..:) Anyway, my main motivation for suggesting the change was code cleanliness first, efficiency second and flexibility fifth.

chrissie-c · 2018-10-16T13:11:46Z

No problem. I don't have a strong opinion on which way to add this in. My very first thought was to do as you suggest and add a whole new target type but it seemed overkill.

kgaillot · 2018-10-16T13:50:18Z

I like sharing the slot. I see the systemd journal more as an enhanced syslog variant than an independent mechanism, most obviously because it takes over the logger command and syslog() function, but also because it serves the same logical function (aggregating log output from daemons). It has a separate API if someone wants to use it, but that's on top of the traditional syslog API (i.e. more like an extension).

jfriesse · 2018-10-16T14:10:30Z

I'm also for qb_ctl mechanism instead of new target type. AFAIK when systemd is used as init system then journald is used and it owns /dev/log without any way to disable such behavior. Then syslog = journald. Contrary on systems without systemd, journald is not used and only syslog make sense. So it's basically on/off switch, not binary none, one or other or both.

wferi · 2018-10-17T08:11:25Z

lib/log.c

+		} else {
+			rc = -EINVAL;
+		}
+		break;


The current code silently ignores this operation if USE_JOURNAL is not defined. Wouldn't it be better to return some error instead?

Good point. commit added (with test)

wferi · 2018-10-17T08:37:08Z

The above arguments for disallowing parallel use hinge on /dev/log being a symlink to /run/systemd/journal/dev-log. This is arranged by the stock systemd-journald-dev-log.socket unit, so it's a strong standard, but not something entirely unavoidable.
But anyway, this does not change that from a higher, "semantic" point of view these arguments are sound, and I've got no reason to challenge them.
What bugs me is the ugly code for making t->close() and t->reload() no-ops instead of setting the ops pointers to NULL. The cost of that extra if in _syslog_logger() is pretty much amortized by the priority, buffering and formatting code (which could be factored out), but still I think getting rid of the use_journal flag and changing the ops pointers instead would result in cleaner code. I now realized that it does not matter whether journal logging is a new target type or a target option in this regard.

jfriesse · 2018-10-17T09:26:44Z

@wferi It's good to know that it's possible to disable journald /dev/log. I was trying to stop this behavior quite a long time ago (RHEL 7.0), because journald was throwing messages (what is also probably fixed) used by cts. So I wanted to replace it by syslog, but without success.

kgaillot · 2018-10-17T13:58:56Z

@jfriesse I recently had the same issue with journald rate-limiting messages from CTS runs; this was my fix:

mkdir -p /etc/systemd/journald.conf.d
echo RateLimitIntervalSec=2s >> /etc/systemd/journald.conf.d/cts.conf

By default it rate-limits after 10,000 messages in 30 seconds from a single package. That changes it to 10,000 messages in 2 seconds. (It's possible to disable it altogether, but I read that that causes performance issues.)

systemd journal can be configured as a logging option at ./configure time (--enable-systemd-journal). If libqb is buit with this then the syslog target can be switched to sending to the journal using qb_log_ctl(QB_LOG_SYSLOG, QB_LOG_CONF_USE_JOURNAL, 1);

chrissie-c · 2018-10-26T07:52:30Z

Thanks for all the helpful comments on this.

jfriesse reviewed Oct 2, 2018

View reviewed changes

jfriesse reviewed Oct 4, 2018

View reviewed changes

wferi reviewed Oct 5, 2018

View reviewed changes

wferi reviewed Oct 16, 2018

View reviewed changes

wferi reviewed Oct 17, 2018

View reviewed changes

chrissie-c added 2 commits October 26, 2018 08:44

log: Fail QB_LOG_CONF_USE_JOURNAL if USE_JOURNAL was not enabled

eba9cbe

chrissie-c merged commit b38614e into ClusterLabs:master Oct 26, 2018

log: Add configure-time option to use systemd journal instead of syslog #327

log: Add configure-time option to use systemd journal instead of syslog #327

Uh oh!

Conversation

chrissie-c commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfriesse Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wferi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrissie-c commented Oct 5, 2018

Uh oh!

kgaillot commented Oct 5, 2018

Uh oh!

wferi commented Oct 5, 2018

Uh oh!

chrissie-c commented Oct 8, 2018

Uh oh!

wferi commented Oct 8, 2018

Uh oh!

jfriesse commented Oct 8, 2018

Uh oh!

wferi commented Oct 8, 2018

Uh oh!

chrissie-c commented Oct 8, 2018

Uh oh!

chrissie-c commented Oct 16, 2018

Uh oh!

wferi commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrissie-c commented Oct 16, 2018

Uh oh!

chrissie-c commented Oct 16, 2018

Uh oh!

wferi commented Oct 16, 2018

Uh oh!

chrissie-c commented Oct 16, 2018

Uh oh!

kgaillot commented Oct 16, 2018

Uh oh!

jfriesse commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wferi commented Oct 17, 2018

Uh oh!

jfriesse commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kgaillot commented Oct 17, 2018

Uh oh!

chrissie-c commented Oct 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

jfriesse Oct 4, 2018 •

edited

Loading

jfriesse commented Oct 17, 2018 •

edited

Loading