ipmi plugin: Implemented IPMI LAN connection and plugin improved #2024

rpv-tomsk · 2016-11-05T05:19:58Z

Changes:

Added instances support and IPMI LAN connection option
Added a notification on connection state change (notify about connection issues)
Removed the sensor removal in case of reading errors
Added the lock to do not request new readings until previous reading is complete
Disabled SDRs local cache
Added the check for sensor 'is readable' flag
Added the check for sensor event reading type (if sensor is discrete or not)
Added checks for sensor states 'scanning disabled' and 'initial update in progress'
Changes in error reporting

Please review and merge.

Superseedes #2023

rpv-tomsk · 2016-11-08T08:58:21Z

ping

3add3287 · 2017-08-04T05:13:23Z

Is this ever going to get merged into master?

rpv-tomsk · 2017-08-04T05:25:38Z

I will rebase this when needed by somebody, just ping me.

3add3287 · 2017-08-04T05:26:39Z

Ping ! ;-)

rpv-tomsk · 2017-08-04T07:47:49Z

Rebased.

Please, put your feedbacks here. That will increase chanses this to be merged.

3add3287 · 2017-08-04T07:52:19Z

Wow, thanks for rebasing it so quickly!
Now the question is: what does it take the powers that be to merge it into collectd master?

* Added instances support * Removed the sensor removal in case of reading errors * Added the lock to do not request new readings until previous reading is complete * Disabled SDRs local cache * Added the check for sensors 'is readable' flag * Added the check for sensor event reading type (if sensor is discrete or not) * Added checks for sensor states 'scanning disabled' and 'initial update in progress' * Changes in error reporting

rpv-tomsk · 2017-10-07T18:35:30Z

Patch rebased to current master, updated with respect of changes recently added to ipmi.c
Unfortunately, I can't do nothing with CI build failure. That is a 'timestamp is in future' problem.

maryamtahhan · 2017-10-08T09:56:25Z

src/ipmi.c

+    INFO("ipmi plugin: %s", msg);
+    break;
+  case IPMI_LOG_WARNING:
+    NOTICE("ipmi plugin: %s", msg);


Should this not also be a warning?

Hi, Maryam!

Thanks for your feedback.

The OpenIPMI/ipmi_log.h has the following explanation of error reporting levels:

/* General information about something that happens. */ IPMI_LOG_INFO, /* An internal error (not reported directly to the user) occurred, but it's not a big deal, the system can still operate. */ IPMI_LOG_WARNING, /* An internal error (not reported directly to the user) occured, and it will affect the operation of the system. This system will still operation, but probably in a degraded mode. */ IPMI_LOG_SEVERE, /* An internal error occured, and it is dangerous for the system to keep operating. */ IPMI_LOG_FATAL, /* When returning errors to the user, this will often be used to report the general cause of the error if the cause would be difficult to determine from just the error return code. */ IPMI_LOG_ERR_INFO,

By this explanation, and by typical use of Collectd WARNING() (), I think NOTICE() suits better.
Of course, I can be wrong...

It would be nice if Collectd will have the similar table of error logging level explanation. (I didn't find it.)

Fair enough

maryamtahhan · 2017-10-08T10:02:12Z

src/ipmi.c

    } else {
+      char errbuf[128] = {0};


are you sure 128 is big enough? most other errbufs use 1024... I would also suggest replacing the magic # with a #defined value

#define ERR_BUF_SIZE 1024

That was inspired by OpenIPMI examlpes code:

https://github.com/wrouesnel/openipmi/blob/master/sample/waiter_sample.c#L285
https://github.com/wrouesnel/openipmi/blob/master/cmdlang/ipmish.c#L595
https://github.com/wrouesnel/openipmi/blob/master/sample/eventd.c#L53
https://github.com/wrouesnel/openipmi/blob/master/include/OpenIPMI/ipmi_err.h#L161

I agree with your suggestion, that would be better to remove the magic.

The sources for ipmi_get_error_string return values are:

strerror() + 4 bytes of prefix ("OS: ")

ipmi_get_cc_string() (ipmi_ccodes[] array) + 6 bytes prefix - fits 128 bytes

rmcpp_error_codes[] + 7 bytes prefix - fits 128 bytes

sol_error_codes[] + 5 bytes prefix - fits 128 bytes

"Success (No error)" string - fits 128 bytes

"Unknown: Unknown" string - fits 128 bytes

The longest is strerror(). Will 1020 (1024 - 4) be enough for strerror()?

maryamtahhan · 2017-10-08T10:03:52Z

src/ipmi.c

    return;
  }

  vl.values = &(value_t){.gauge = value};
  vl.values_len = 1;

+  if (st->host != NULL)
+    sstrncpy(vl.host, st->host, sizeof(vl.host));


I think vl.host gets filled at a higher level usually? not sure if it's explicitly needed

Yes, you are right - vl.host can be filled by hostname_g at higher level.
But in this case we need to set vl.host of the metric to the value set in configuration, not to the hostname_g value.

maryamtahhan · 2017-10-08T10:07:59Z

src/ipmi.c

+    c_ipmi_error("domain_connection_change_handler", err);
+
+  if (!still_connected)
+    return;


would a simple info/debug message not be useful here?

At practice, that will be reported by a c_ipmi_error() call at several lines above.
Also, that will be reported by c_ipmi_log() handler too.

An example:

[2017-10-08 19:09:49] ipmi plugin: one 0 ipmi_lan.c(lost_connection): Connection 0 to the BMC is down [2017-10-08 19:09:49] ipmi plugin: one 0 ipmi_lan.c(lost_connection): All connections to the BMC are down [2017-10-08 19:09:49] ipmi plugin: domain_connection_change_handler failed for `one`: OS: Connection timed out

I have added a code to send Collectd notification in case of connection lost/restored and will publish it in updated PR.

maryamtahhan · 2017-10-08T10:15:43Z

src/ipmi.c

+  sfree(st);
+} /* void c_ipmi_free_instance */
+
+void c_ipmi_add_instance(c_ipmi_instance_t *instance) {


what about using a llist_t for instances?

Is there is a strong opinion/rules about this?
Why would you prefer a list instead of chain?

There's no clear-cut advise/guideline for or against the generic linked list. I'd say that if you need to look up by (string) key or you'd need a double-linked list, then the generic implementation can abstract away some complexity.

HTH, —octo

I'd say that if you need to look up by (string) key or you'd need a double-linked list

There is no such need here.

* Do not request reading unless domain is connected * Report instance name in error messages

rpv-tomsk · 2017-10-09T05:59:48Z

Updated.

rpv-tomsk · 2017-10-09T07:18:19Z

Also I'm going to add an option to disable all analog sensors polling.
So, the Collectd will handle only SEL and will send notifications only, without metrics.
That will be an good use-case of Collectd as IPMI events translator.

I have an quite old Intel SR2500 platform wich IPMI MC is stuck due to network polling. Now it does not respond to any requests - remote or local and server needs to be completely powered off (physically unplugged from outlet) to reboot MC.

octo

Hi @rpv-tomsk,

thanks for all you work on this! The code looks really good! I left quite some review comments, but the only blocking ones are:

pthread_t is a struct on some platforms and we need to change some initialization and can't simply compare with 0.
The init() callback may be called more than once and the function needs to be able to handle this.
The config needs to mention the legacy support.

Details for all of these are inline.

Best regards,
—octo

octo · 2017-10-09T19:42:46Z

src/collectd.conf.pod

+The B<ipmi plugin> allows to monitor server platform status using the Intelligent
+Platform Management Interface (IPMI). Local and remote interfaces are supported.
+
+The plugin configuration consists of one or more B<Instance> blocks which


This should mention that Instance blocks take a string argument. Maybe a synopsis of this plugin's config would be useful.

octo · 2017-10-09T20:00:04Z

src/ipmi.c

+#if COLLECT_DEBUG
+  case IPMI_LOG_DEBUG_START:
+  case IPMI_LOG_DEBUG:
+    fprintf(stderr, "ipmi plugin: %s\n", msg);


Why not DEBUG()?

octo · 2017-10-09T20:07:22Z

src/ipmi.c

    } else {
+      char errbuf[ERR_BUF_SIZE] = {0};
+      ipmi_get_error_string(err, errbuf, sizeof(errbuf) - 1);
+
      if (IPMI_IS_IPMI_ERR(err))


Do we need to differentiate these cases when we're printing the same info message in all cases?

These messages are different in the second line of output format:

"%s." vs "%s (%#x)." and in a source of error code (IPMI_GET_OS_ERR(err) / IPMI_GET_SOL_ERR(err)).

The ipmi_get_error_string() does not always put error code into its output, so that appended locally for unified and more detailed message.

octo · 2017-10-09T20:37:46Z

src/ipmi.c

-    return -1;
-  }
+  if (st->connaddr != NULL) {
+    char *ip_addrs[1] = {NULL}, *ports[1] = {NULL};


At your option: I find both of the below easier to read:

You can either initialize the arrays:

char *ip_addrs[] = { strdup(st->connaddr), };

or simply use pointers:

char *ip_addr = strdup(st->connaddr); ipmi_ip_setup_con(&ip_addr, /* … */);

Thanks a lot for this note!

I think there will be a tiny memleak with these strdup().
Also, their return value is not checked and ... they are unneded at all.

I was wondering about the strdup() ;) If they're not needed, all the better, you can just pass &st->connaddr to ipmi_open_domain().

octo · 2017-10-10T05:33:55Z

src/ipmi.c

  }

-  sensor_list_remove_all();
+  os_handler->free_os_handler(os_handler);


Please set os_handler = NULL; just to be on the save side.

octo · 2017-10-10T05:53:17Z

src/ipmi.c

  }

-  ipmi_open_option_t open_option[1] = {[0] = {.option = IPMI_OPEN_OPTION_ALL,


At your option:

ipmi_open_option_t opts[] = { {.option = IPMI_OPEN_OPTION_ALL, .ival = 1}, #ifdef IPMI_OPEN_OPTION_USE_CACHE /* OpenIPMI-2.0.17 and later */ {.option = IPMI_OPEN_OPTION_USE_CACHE, .ival = 1}, #endif }; ipmi_open_domain(/* … */, opts, STATIC_ARRAY_SIZE(opts), /* … */);

That looks excellent, but I can't compile this...
Code:

ipmi_open_option_t opts[] = { {.option = IPMI_OPEN_OPTION_ALL, .ival = 1}, #ifdef IPMI_OPEN_OPTION_USE_CACHE /* OpenIPMI-2.0.17 and later: Disable SDR cache in local file */ {.option = IPMI_OPEN_OPTION_USE_CACHE, .ival = 0}, #endif };

provides an error message:

src/ipmi.c:899: error: unknown field 'ival' specified in initializer cc1: warnings being treated as errors src/ipmi.c:899: error: missing braces around initializer src/ipmi.c:899: error: (near initialization for 'opts[0].<anonymous>') src/ipmi.c:902: error: unknown field 'ival' specified in initializer

I'm completely unfamiliar with a such syntax, sorry...

Oh, the struct has an unnamed union. This should work:

ipmi_open_option_t opts[] = { {.option = IPMI_OPEN_OPTION_ALL, {.ival = 1}}, #ifdef IPMI_OPEN_OPTION_USE_CACHE /* OpenIPMI-2.0.17 and later */ {.option = IPMI_OPEN_OPTION_USE_CACHE, {.ival = 0}}, #endif }; ipmi_open_domain(/* … */, opts, STATIC_ARRAY_SIZE(opts), /* … */);

Thanks for update!

octo · 2017-10-10T05:57:16Z

src/ipmi.c


-  ipmi_init(os_handler);
+    ip_addrs[0] = strdup(st->connaddr);
+    ports[0] = strdup(IPMI_LAN_STD_PORT_STR);


Most other plugins with an Address option also have a Port option to let the user override the default. Would that make sense for the ipmi plugin, too?

octo · 2017-10-10T05:58:51Z

src/ipmi.c

@@ -476,6 +605,8 @@ static int sensor_threshold_event_handler(
    enum ipmi_value_present_e value_present, unsigned int raw_value,
    double value, void *cb_data, ipmi_event_t *event) {

+  c_ipmi_instance_t *st = (c_ipmi_instance_t *)cb_data;


Nit, at your option: you can implicitly cast void* to any other pointer type, i.e.:

c_ipmi_instance_t *st = cb_data;

Here and below.

octo · 2017-10-10T06:04:11Z

src/ipmi.c

    notification_t n = {NOTIF_OKAY, cdtime(), "", "", "ipmi", "", "", "", NULL};

-    sstrncpy(n.host, hostname_g, sizeof(n.host));
+    sstrncpy(n.host, (st->host != NULL) ? st->host : hostname_g,


This repeats at least two more times. Would it make sense to have a function that initializes a notification_t based on a c_ipmi_instance_t *? I.e.:

static notification_t notification_init(c_ipmi_instance_t const *st); /* … */ notification_t n = notification_init(st);

What do you think about notification_init() from daemon/common.c?
It used only once, in virt plugin.

octo · 2017-10-10T06:09:13Z

src/collectd.conf.pod

+
+If a IPMI connection state changes after initialization time of a minute
+a notification is sent. Defaults to B<false>.
+


Please add a note about legacy support, i.e. that any option other than Instance will trigger legacy config handling which skipts the <Instance> block and that this support will go away in the next major version of collectd.

rpv-tomsk · 2017-10-10T10:00:06Z

Hi, Florian!

Thanks for a review! I made some changes and push it here.
I will add Port option support later, most other suggestions was accepted and code improved.

octo · 2017-10-10T11:52:10Z

src/ipmi.c

-    return -1;
+  if (st->connaddr != NULL) {
+    status = ipmi_ip_setup_con(
+        &st->connaddr, (char * [1]){IPMI_LAN_STD_PORT_STR}, 1, st->authtype,


Nit, at your option: I find &(char *){IPMI_LAN_STD_PORT_STR} easier to read.

I tried to find this syntax, but with no success.
Thanks, amended.

octo · 2017-10-10T11:54:33Z

src/ipmi.c

+  c_ipmi_instance_t *st;
+  char callback_name[3 * DATA_MAX_NAME_LEN];
+
+  os_handler = ipmi_posix_thread_setup_os_handler(SIGIO);


init() is called every time the "kstat" structure changes, e.g. when devices are added to or removed from the system. As I said, it's a legacy and a bit broken and we should fix it, but it's out of scope for this PR.

octo

Looks great, thank you!

octo · 2017-10-10T11:59:04Z

src/ipmi.c

+
+static notification_t c_ipmi_notification_init(c_ipmi_instance_t const *st,
+                                               int severity) {
+  notification_t n = {severity, cdtime(), "", "", "ipmi", "", "", "", NULL};


At your option: this would be a bit nicer with a named struct initializer:

notification_t n = { .severity = severity, .time = cdtime(), .plugin = "ipmi", };

All the fields that are not explicitly initialized are guaranteed to be set to zero / NULL.

octo · 2017-10-10T13:57:57Z

rpv-tomsk merged commit 0587ef9 into collectd:master

First ~~post~~ merge 🎉

rpv-tomsk · 2017-10-10T14:05:33Z

Yeah! Many thanks to you for all what you have done for this!
And thanks for Collectd project itself!

maryamtahhan · 2017-10-10T16:40:28Z

@rpv-tomsk congrats on the first merge :) Keep up the good work!

rpv-tomsk · 2017-10-11T06:48:59Z

Thanks, Maryam!

octo added the Pending feedback label Nov 5, 2016

rpv-tomsk force-pushed the master-ipmi branch 2 times, most recently from 00c365b to cac7575 Compare November 5, 2016 07:29

rpv-tomsk force-pushed the master-ipmi branch from cac7575 to e50e41b Compare August 4, 2017 07:44

rpv-tomsk force-pushed the master-ipmi branch from e50e41b to 689b592 Compare October 7, 2017 17:49

ipmi plugin: Code cleanup

f80142a

octo added this to the 5.8 milestone Oct 8, 2017

octo added the Feature label Oct 8, 2017

maryamtahhan reviewed Oct 8, 2017

View reviewed changes

ipmi plugin: Improvements

c9df8a2

* Do not request reading unless domain is connected * Report instance name in error messages

rpv-tomsk force-pushed the master-ipmi branch from 71c07dd to 0d9be8e Compare October 9, 2017 06:01

Pavel Rochnyack added 2 commits October 9, 2017 13:01

ipmi plugin: Notify about IPMI connection state change

a407ce8

ipmi plugin: Added a define for errors / messages buffer size

0d9be8e

rpv-tomsk force-pushed the master-ipmi branch 2 times, most recently from fcacd70 to 2ff2422 Compare October 9, 2017 19:21

ipmi plugin: Use cf_util_get_boolean()

2ff2422

octo reviewed Oct 10, 2017

View reviewed changes

rpv-tomsk force-pushed the master-ipmi branch from 8bce656 to 4207aaf Compare October 10, 2017 10:08

ipmi plugin: Fixed remarks found while review

4207aaf

ipmi plugin: Refactored to use c_ipmi_notification_init_()

eeca485

octo reviewed Oct 10, 2017

View reviewed changes

rpv-tomsk force-pushed the master-ipmi branch from a5ae096 to e04af9b Compare October 10, 2017 11:57

ipmi plugin: style

e04af9b

octo approved these changes Oct 10, 2017

View reviewed changes

rpv-tomsk merged commit 0587ef9 into collectd:master Oct 10, 2017

rpv-tomsk deleted the master-ipmi branch July 13, 2019 07:40

		}

		ipmi_open_option_t open_option[1] = {[0] = {.option = IPMI_OPEN_OPTION_ALL,


		If a IPMI connection state changes after initialization time of a minute
		a notification is sent. Defaults to B<false>.

ipmi plugin: Implemented IPMI LAN connection and plugin improved #2024

ipmi plugin: Implemented IPMI LAN connection and plugin improved #2024

Conversation

rpv-tomsk commented Nov 5, 2016 • edited

rpv-tomsk commented Nov 8, 2016

3add3287 commented Aug 4, 2017

rpv-tomsk commented Aug 4, 2017

3add3287 commented Aug 4, 2017

rpv-tomsk commented Aug 4, 2017

3add3287 commented Aug 4, 2017

rpv-tomsk commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamtahhan Oct 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpv-tomsk Oct 9, 2017 • edited

Choose a reason for hiding this comment

rpv-tomsk commented Oct 9, 2017

rpv-tomsk commented Oct 9, 2017

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpv-tomsk commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo commented Oct 10, 2017

rpv-tomsk commented Oct 10, 2017

maryamtahhan commented Oct 10, 2017

rpv-tomsk commented Oct 11, 2017

rpv-tomsk commented Nov 5, 2016 •

edited

maryamtahhan Oct 8, 2017 •

edited

rpv-tomsk Oct 9, 2017 •

edited

octo Oct 10, 2017 •

edited