mcelog plugin #2003

krzysztofx · 2016-10-26T13:10:38Z

Plugin for collecting machine check exceptions reported by mcelog
The plugin at current stage does the following:
• Checks mcelog server liveliness, reports a failure if it’s not running of if it fails.
• Retrieve Memory Corrected and Uncorrected Errors from mcelog Unix socket (client protocol)
• Publishes collected data as notifications and statistics(values)

rubenk · 2016-10-26T17:11:41Z

Thanks for your contribution @krzysztofx!
I'll leave some comments inline.

rubenk · 2016-10-26T17:13:24Z

README

@@ -926,6 +931,11 @@ Prerequisites
     `varnish' plugin.
     <http://varnish-cache.org>

+  * mcelog


I don't think you need to add this block in this section, since it is mcelog is not a library and needed for building collectd.

right, it's not compilation prerequisite

rubenk · 2016-10-26T17:13:29Z

README

    - mbmon
      Motherboard sensors: temperature, fan speed and voltage information,
      using mbmon(1).

+    - mcelogmon


The plugin is called mcelog

I'll fix that

rubenk · 2016-10-26T17:14:32Z

configure.ac

@@ -6314,6 +6314,7 @@ AC_PLUGIN([match_regex],         [yes],                     [The regex match])
 AC_PLUGIN([match_timediff],      [yes],                     [The timediff match])
 AC_PLUGIN([match_value],         [yes],                     [The value match])
 AC_PLUGIN([mbmon],               [yes],                     [Query mbmond])
+AC_PLUGIN([mcelog],              [yes],                     [Machine Check Exceptions notifications])


This always builds and installs mcelog, but I think it is Linux-specific?

right, I'll fix that

rubenk · 2016-10-26T17:15:30Z

src/Makefile.am

+mcelog_la_SOURCES = mcelog.c
+mcelog_la_LDFLAGS = $(PLUGIN_LDFLAGS)
+mcelog_la_LIBADD =
+if BUILD_WITH_LIBSOCKET


I don't think you need this, it is mainly for Solaris iirc, and mcelog seems Linux-only.

libsocket linkage removed

octo

Hi @krzysztofx,

thank you very much for your patch! I'm afraid the worker thread needs some more work, the indentation levels get out of hand and checking for else-blocks becomes impossible for many of the very long if statements. I also think that the function is too long and at least the parsing code should be moved to its own function.

Best regards,
—octo

octo · 2016-10-26T19:10:55Z

src/mcelog.c

+typedef struct mcelog_config_s mcelog_config_t;
+typedef struct mcelog_memory_rec_s mcelog_memory_rec_t;
+
+mcelog_config_t g_mcelog_config;


The static keyword is missing here. Also, please initialize the struct where it's declared, e.g.:

static mcelog_config_t g_mcelog_config = { .socket_path = "/var/run/mcelog-client", .logfile = "/var/log/mcelog", .unix_sock = { .sun_family = AF_UNIX, .sun_path = "/var/run/mcelog-client", }, .sock_fd = -1, };

I'd like to provide way to reinitalize g_mcelog_config struct, so how do you see usage of compound literals there ? I mean:

static mcelog_config_t g_mcelog_config ; // skippded lines static void mcelog_config_init_default(void) { g_mcelog_config = (mcelog_config_t){ .logfile = "/var/log/mcelog", .unix_sock = { .sun_family = AF_UNIX, .sun_path = "/var/run/mcelog-client", }, .tid = 0, .sock_fd = -1, };

octo · 2016-10-26T19:24:51Z

src/mcelog.c

+#define POLL_TIMEOUT 1000 /* ms */
+
+struct mcelog_config_s {
+  char socket_path[PATH_MAX];   /* mcelog client socket */


Why is the path stored twice? Once here and once in unix_sock.sun_path.

octo · 2016-10-26T19:25:53Z

src/mcelog.c

+  CDTIME_T_TO_TIMEVAL(interval, &socket_timeout);
+
+  g_mcelog_config.unix_sock.sun_family = AF_UNIX;
+  g_mcelog_config.sock_fd = -1;


This is a dead assignment.

octo · 2016-10-26T20:49:41Z

src/mcelog.c

+static const char corrected_err[] = "corrected memory errors:";
+static const char uncorrected_err[] = "uncorrected memory errors:";
+
+static void mcelog_config_init_default(void) {


Please use a struct initializer, as mentioned above, to zero the struct. In particular, bzero() is not portable.

octo · 2016-10-26T20:50:34Z

src/mcelog.c

+    if (strcasecmp("McelogClientSocket", child->key) == 0) {
+      if (cf_util_get_string_buffer(child, g_mcelog_config.unix_sock.sun_path,
+                                    sizeof(g_mcelog_config.unix_sock.sun_path) -
+                                        1) < 0) {


Why do you subtract 1 here? cf_util_get_string_buffer() guarantees null termination.

octo · 2016-10-26T21:04:32Z

src/mcelog.c

+                DEBUG("%s: done", MCELOG_PLUGIN);
+                break;
+              }
+              if (!memcmp(buf, socket_str, strlen(socket_str))) {


This has a high risk of reading uninitialized memory from buf. Please use strncmp() instead.

octo · 2016-10-26T21:09:16Z

src/mcelog.c

+      pthread_exit(NULL);
+    }
+    pthread_mutex_unlock(&mcelog_thread_lock);
+    int poll_ret = poll(&poll_fd, 1, POLL_TIMEOUT);


It might be worth sending this thread a signal (using pthread_kill() from the shutdown() callback) instead of busy looping here.

You mean adding signal handler to thread function poll_worker ? Or just terminating thread asynchronously sending SIGTERM ? I have noticed that some plugins use that approach but I think this is not applicable here since we have to do some cleanup like calling fclose and so on.

I will terminate thread with thread with pthread_cancel and put cleanup code into callback function set by pthread_cancel_push. Is it ok from your POV ?

octo · 2016-10-26T21:10:14Z

src/mcelog.c

+  mcelog_thread_shutdown = 1;
+  pthread_mutex_unlock(&mcelog_thread_lock);
+  pthread_join(g_mcelog_config.tid, NULL);
+  pthread_mutex_destroy(&mcelog_thread_lock);


Please don't destroy mutexes that you initialized with the static initializer.

octo · 2016-10-26T21:10:53Z

src/mcelog.c

+  pthread_mutex_unlock(&mcelog_thread_lock);
+  pthread_join(g_mcelog_config.tid, NULL);
+  pthread_mutex_destroy(&mcelog_thread_lock);
+  shutdown(g_mcelog_config.sock_fd, 0);


Please check that sock_fd is in fact a valid file descriptor before calling shutdown().

octo · 2016-10-26T21:11:33Z

src/mcelog.c

+static _Bool mcelog_thread_shutdown = 0;
+static pthread_mutex_t mcelog_thread_lock = PTHREAD_MUTEX_INITIALIZER;
+
+static const char socket_str[] = "SOCKET";


Having these const char[] up here doesn't really improve the parsing code below. Please simply use string literals in the worker thread.

I will change it

krzysztofx · 2016-10-27T09:29:46Z

@octo @rubenk thanks for your reviews. I'll respond your comments soon.

krzysztofx · 2016-11-14T10:06:43Z

Hi @octo @rubenk
I've pushed patch addressing your comments and also fix for build failure on epel5 setup.
Please let me know if you will find out anything else that could be improved.

rubenk · 2016-11-19T13:09:21Z

README

-      and reported to software) reported by mcelog and generate appropriate
-      notifications when machine check errors are detected.
+    - mcelog
+      Logs and accounts machine check exceptions (in particular memory, IO, and


I liked the original text better. The plugin doesn't actually log MCE's right?

It logs only the amount of corrected/uncorrected memory related errors per memory SOCKET and CHANNEL. No MCE error codes are logged if that's what you mean. Data source is equivalent to output of 'mcelog --client', for example:

[root@silpixa00392165 kmatczax]# mcelog --client Memory errors SOCKET 0 CHANNEL any DIMM any corrected memory errors: 0 total 0 in 24h uncorrected memory errors: 8 total 8 in 24h SOCKET 0 CHANNEL 0 DIMM any corrected memory errors: 0 total 0 in 24h uncorrected memory errors: 8 total 8 in 24h

I'll revert to previous mcelog plugin description.

krzysztofx · 2016-12-06T09:52:15Z

Hi

I've committed fix for typedef redefinition. All check passed but now I see merge conflict with no details given. Please let me know if I could do anything to fix that, or if any other changes in plugin code are still required.

krzysztofx · 2016-12-06T22:29:39Z

@rubenk @octo
Would you mind If I rebase that PR against master branch to fix the conflict ?

krzysztofx · 2016-12-08T15:09:25Z

Rebase done

krzysztofx · 2016-12-16T09:54:45Z

Hi
@rubenk @octo
Sorry for pinging , but is there anything that could be done to get that plugin merged? If yes, please let me know.
I'm developing extension for that plugin that collects all MCE exceptions, so it would be easier to base on already merged version.

rubenk · 2016-12-19T12:29:15Z

Could you re-order or squash some of your commits? You now have multiple commits with the same commit messages.

rubenk · 2016-12-19T12:30:30Z

src/Makefile.am

+pkglib_LTLIBRARIES += mcelog.la
+mcelog_la_SOURCES = mcelog.c
+mcelog_la_LDFLAGS = $(PLUGIN_LDFLAGS)
+mcelog_la_LIBADD =


this is not needed.

rubenk · 2016-12-19T12:31:39Z

src/collectd.conf.pod

@@ -3322,6 +3322,28 @@ TCP-Port to connect to. Defaults to B<411>.

 =back

+=head2 Plugin C<mcelog>
+
+The C<mcelog plugin> uses mcelog to retrieve machine check exceptions, etc.


What does the 'etc.' stand for? It doesn't add much information, I'd either leave it out or extend the sentence.

rubenk · 2016-12-19T12:33:09Z

src/collectd.conf.pod

+mcelog server is running. When the server is running, the plugin will tail the
+specified logfile to retrieve machine check exception information and send a
+notification with the details from the logfile. The plugin will use the mcelog
+client protocol to retrieve memory errors.


I'd prefer 'retrieve machine check exceptions' instead of 'memory errors'.

At the moment mcelog's client/server protocol returns only memory related MCE's, see http://www.mcelog.org/protocol.html. That's why I used 'memory errors' term. Would you like to extend it to all MCE, or maybe use term 'memory related machine check exceptions' ?

I see, thanks for the explanation. The latter sounds good.

rubenk · 2016-12-19T12:33:25Z

src/collectd.conf.pod

+=over 4
+
+=item B<McelogClientSocket> I<Path>
+Connect to mcelog client socket using the UNIX domain socket at I<Path>.


Connect to the mcelog client socket

rubenk · 2016-12-19T12:33:46Z

src/collectd.conf.pod

+
+=item B<McelogLogfile> I<Path>
+
+mcelog file to parse. Defaults to B<"/var/log/mcelog">.


The mcelog logfile to parse.

krzysztofx · 2016-12-19T12:37:12Z

@rubenk
Thanks for your review. I'll squash redundant commits and address your comments soon

Thanks
Krzysztof

rubenk · 2016-12-19T12:40:20Z

src/mcelog.c

+
+  /* synchronization via write lock since sock_fd may be changed here */
+  pthread_rwlock_wrlock(&self->lock);
+  self->sock_fd = socket(PF_UNIX, SOCK_STREAM, 0);


Could you use SOCK_CLOEXEC here?

Oh, and while you're at it, add SOCK_NONBLOCK to the flags, so you don't have to do the fcntl later.

regarding SOCK_NONBLOCK please see d212c7f. It failed on epel5

Ah right, sorry for the confusion. The situation has changed now though, after we released 5.7 we dropped support for RHEL5 from master. Jenkins is still doing build on RHEL5 but those will be cleaned up shortly and you can ignore them.

rubenk · 2016-12-19T12:43:26Z

src/mcelog.c

+
+  /* Set socket timeout option */
+  if (setsockopt(self->sock_fd, SOL_SOCKET, SO_SNDTIMEO,
+                 (char *)&socket_timeout, sizeof(socket_timeout)) < 0)


I think the (char *) cast is unneeded.

rubenk · 2016-12-19T12:49:45Z

src/mcelog.c

+static void *poll_worker(__attribute__((unused)) void *arg) {
+  char errbuf[MCELOG_BUFF_SIZE];
+  mcelog_thread_running = 1;
+  FILE **pp_file = calloc(1, sizeof(FILE *));


Please use sizeof on the var, not the type, so sizeof(*pp_file)

rubenk · 2016-12-19T12:51:31Z

src/mcelog.c

+      continue;
+
+    mcelog_memory_rec_t memory_record;
+    memset(&memory_record, 0, sizeof(memory_record));


Please use designated initialiser here, so mcelog_memory_rec_t memory_record = {0}

krzysztofx · 2016-12-23T00:38:19Z

My apologies for squashing by mistake all previous commits into c8f0822. I hope it doesn't make it much harder to review. If you think I could somehow revert that rebase and squash only specific commits please let me know.
All recent comments have been addressed in 6016e1d

Now I see merge conflict with the master branch. Do you agree for rebase ?

Thanks
Krzysztof

krzysztofx · 2016-12-23T09:34:09Z

pull-requests-github_trigger-aggregation failed due to:

mcelog.c:437:5: error: missing braces around initializer [-Werror=missing-braces]
    mcelog_memory_rec_t memory_record = {0};

Looks like this job has more restrictive compiler settings than others . Should I put additional braces like {{0}} or ignore that fail ?

rubenk · 2016-12-23T16:50:41Z

No, that's not it, the compiler settings are the same. This job is the first that runs and the other ones are only run if this one succeeds.

This looks like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119.

Perhaps you could try changing the order of the members of the mcelog_memory_rec_s struct around to work around it? My guess is that this won't happen if the first member is an int instead of char[]

krzysztofx · 2016-12-25T16:53:34Z

@rubenk
Changing order of struct members helped for missing braces warning. Now as expected I got failures on epel5 builds due to unknown socket flags:

mcelog.c: In function 'socket_reinit':
mcelog.c:163: error: 'SOCK_CLOEXEC' undeclared (first use in this function)
mcelog.c:163: error: (Each undeclared identifier is reported only once
mcelog.c:163: error: for each function it appears in.)
mcelog.c:163: error: 'SOCK_NONBLOCK' undeclared (first use in this function)
make[3]: *** [mcelog.lo] Error 1

So according to previous comments this is acceptable, right ?
Please let me know if I should rebase to master.

thanks
Krzysztof

rubenk · 2016-12-25T19:34:29Z

Thanks @krzysztofx, yes, these errors on EPEL5 are perfectly alright.
Please rebase and then I'll merge.

Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

Change-Id: I0ddfd6dcd911ab46fcbc032081a2980b1f1f549b Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

Change-Id: I77a8ca1d5854ebda8533483cdd5ea08c3c07a059 Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

Change-Id: I42b4b392da942808d29b73a13cd5e0bbcef9b6b8 Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

krzysztofx · 2016-12-25T22:32:14Z

@rubenk
Rebase done

Thanks
Krzysztof

krzysztofx changed the title ~~Feat ras~~ mcelog plugin Oct 26, 2016

rubenk reviewed Oct 26, 2016

View reviewed changes

octo added the New plugin label Oct 26, 2016

octo requested changes Oct 26, 2016

View reviewed changes

octo added the Pending feedback label Oct 27, 2016

octo added Pending contributor action Pending feedback and removed Pending feedback Pending contributor action labels Nov 10, 2016

rubenk reviewed Nov 19, 2016

View reviewed changes

octo added Pending contributor action Pending feedback and removed Pending feedback Pending contributor action labels Dec 1, 2016

krzysztofx force-pushed the feat_ras branch from 8b128e4 to 02b9700 Compare December 8, 2016 15:02

rubenk reviewed Dec 19, 2016

View reviewed changes

krzysztofx force-pushed the feat_ras branch from b2e938a to c8f0822 Compare December 22, 2016 23:42

Maryam Tahhan and others added 4 commits December 25, 2016 20:23

fixed rebase conflicts

4ebf1f9

Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

PR comments addressed

36b8935

Change-Id: I0ddfd6dcd911ab46fcbc032081a2980b1f1f549b Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

workaround for zero initializer false warning

fa4bfcc

Change-Id: I77a8ca1d5854ebda8533483cdd5ea08c3c07a059 Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

post rebase issues fixed

9e9647b

Change-Id: I42b4b392da942808d29b73a13cd5e0bbcef9b6b8 Signed-off-by: Krzysztof Matczak <krzysztofx.matczak@intel.com>

krzysztofx force-pushed the feat_ras branch from 7afa05e to 9e9647b Compare December 25, 2016 21:24

rubenk merged commit 38dd976 into collectd:master Dec 26, 2016

vmytnykx mentioned this pull request Dec 26, 2016

OVS events plugin #1971

Merged

krzysztofx deleted the feat_ras branch February 17, 2017 09:59

octo added this to the 5.8 milestone Oct 11, 2017

maryamtahhan mentioned this pull request Oct 12, 2017

Next collectd release #2420

Closed

19 tasks


		=item B<McelogLogfile> I<Path>

		mcelog file to parse. Defaults to B<"/var/log/mcelog">.

mcelog plugin #2003

mcelog plugin #2003

Uh oh!

Conversation

Uh oh!

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!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for F438 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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!