Merge pull request #2103 from mojaves-redhat/pr-block-stats-flags

virt plugin: support virDomainBlockStatsFlags

diff --git a/src/collectd.conf.in b/src/collectd.conf.in
index 1cc86af..982cba9 100644 (file)
--- a/src/collectd.conf.in
+++ b/src/collectd.conf.in
@@ -1459,6 +1459,7 @@
 #      InterfaceFormat name
 #      PluginInstanceFormat name
 #      Instances 1
+#      ExtraStats "disk"
 #</Plugin>
 
 #<Plugin vmem>

diff --git a/src/collectd.conf.pod b/src/collectd.conf.pod
index 6931066..9b32a78 100644 (file)
--- a/src/collectd.conf.pod
+++ b/src/collectd.conf.pod
@@ -8076,6 +8076,16 @@ How many read instances you want to use for this plugin. The default is one,
 and the sensible setting is a multiple of the B<ReadThreads> value.
 If you are not sure, just use the default setting.
 
+=item B<ExtraStats> B<string>
+
+Report additional extra statistics. The default is no extra statistics, preserving
+the previous behaviour of the plugin. If unsure, leave the default. If enabled,
+allows the plugin to reported more detailed statistics about the behaviour of
+Virtual Machines. The argument is a space-separated list of selectors.
+Currently supported selectors are:
+B<disk> report extra statistics like number of flush operations and total
+service time for read, write and flush operations.
+
 =back
 
 =head2 Plugin C<vmem>

diff --git a/src/virt.c b/src/virt.c
index 830db51..5a4604c 100644 (file)
--- a/src/virt.c
+++ b/src/virt.c
@@ -37,6 +37,12 @@
 /* Plugin name */
 #define PLUGIN_NAME "virt"
 
+#ifdef LIBVIR_CHECK_VERSION
+#if LIBVIR_CHECK_VERSION(0, 9, 5)
+#define HAVE_BLOCK_STATS_FLAGS 1
+#endif
+#endif
+
 static const char *config_keys[] = {"Connection",
 
                                     "RefreshInterval",
@@ -54,6 +60,7 @@ static const char *config_keys[] = {"Connection",
                                     "PluginInstanceFormat",
 
                                     "Instances",
+                                    "ExtraStats",
 
                                     NULL};
 #define NR_CONFIG_KEYS ((sizeof config_keys / sizeof config_keys[0]) - 1)
@@ -158,6 +165,12 @@ enum bd_field { target, source };
 /* InterfaceFormat. */
 enum if_field { if_address, if_name, if_number };
 
+/* ExtraStats */
+#define EX_STATS_MAX_FIELDS 8
+#define EX_STATS_DISK "disk"
+enum ex_stats { ex_stats_none = 0, ex_stats_disk = 1 };
+static enum ex_stats extra_stats = ex_stats_none;
+
 /* BlockDeviceFormatBasename */
 _Bool blockdevice_format_basename = 0;
 static enum bd_field blockdevice_format = target;
@@ -168,6 +181,65 @@ static time_t last_refresh = (time_t)0;
 
 static int refresh_lists(struct lv_read_instance *inst);
 
+struct lv_block_info {
+  virDomainBlockStatsStruct bi;
+
+  long long rd_total_times;
+  long long wr_total_times;
+
+  long long fl_req;
+  long long fl_total_times;
+};
+
+static void init_block_info(struct lv_block_info *binfo) {
+  if (binfo == NULL)
+    return;
+
+  binfo->bi.rd_req = -1;
+  binfo->bi.wr_req = -1;
+  binfo->bi.rd_bytes = -1;
+  binfo->bi.wr_bytes = -1;
+
+  binfo->rd_total_times = -1;
+  binfo->wr_total_times = -1;
+  binfo->fl_req = -1;
+  binfo->fl_total_times = -1;
+}
+
+#ifdef HAVE_BLOCK_STATS_FLAGS
+
+#define GET_BLOCK_INFO_VALUE(NAME, FIELD)                                      \
+  do {                                                                         \
+    if (!strcmp(param[i].field, NAME)) {                                       \
+      binfo->FIELD = param[i].value.l;                                         \
+      continue;                                                                \
+    }                                                                          \
+  } while (0)
+
+static int get_block_info(struct lv_block_info *binfo,
+                          virTypedParameterPtr param, int nparams) {
+  if (binfo == NULL || param == NULL)
+    return -1;
+
+  for (int i = 0; i < nparams; ++i) {
+    /* ignore type. Everything must be LLONG anyway. */
+    GET_BLOCK_INFO_VALUE("rd_operations", bi.rd_req);
+    GET_BLOCK_INFO_VALUE("wr_operations", bi.wr_req);
+    GET_BLOCK_INFO_VALUE("rd_bytes", bi.rd_bytes);
+    GET_BLOCK_INFO_VALUE("wr_bytes", bi.wr_bytes);
+    GET_BLOCK_INFO_VALUE("rd_total_times", rd_total_times);
+    GET_BLOCK_INFO_VALUE("wr_total_times", wr_total_times);
+    GET_BLOCK_INFO_VALUE("flush_operations", fl_req);
+    GET_BLOCK_INFO_VALUE("flush_total_times", fl_total_times);
+  }
+
+  return 0;
+}
+
+#undef GET_BLOCK_INFO_VALUE
+
+#endif /* HAVE_BLOCK_STATS_FLAGS */
+
 /* ERROR(...) macro for virterrors. */
 #define VIRT_ERROR(conn, s)                                                    \
   do {                                                                         \
@@ -306,6 +378,37 @@ static void submit_derive2(const char *type, derive_t v0, derive_t v1,
   submit(dom, type, devname, values, STATIC_ARRAY_SIZE(values));
 } /* void submit_derive2 */
 
+static void disk_submit(struct lv_block_info *binfo, virDomainPtr dom,
+                        const char *type_instance) {
+  char flush_type_instance[DATA_MAX_NAME_LEN];
+
+  ssnprintf(flush_type_instance, sizeof(flush_type_instance), "flush-%s",
+            type_instance);
+
+  if ((binfo->bi.rd_req != -1) && (binfo->bi.wr_req != -1))
+    submit_derive2("disk_ops", (derive_t)binfo->bi.rd_req,
+                   (derive_t)binfo->bi.wr_req, dom, type_instance);
+
+  if ((binfo->bi.rd_bytes != -1) && (binfo->bi.wr_bytes != -1))
+    submit_derive2("disk_octets", (derive_t)binfo->bi.rd_bytes,
+                   (derive_t)binfo->bi.wr_bytes, dom, type_instance);
+
+  if (extra_stats & ex_stats_disk) {
+    if ((binfo->rd_total_times != -1) && (binfo->wr_total_times != -1))
+      submit_derive2("disk_time", (derive_t)binfo->rd_total_times,
+                     (derive_t)binfo->wr_total_times, dom, type_instance);
+
+    if (binfo->fl_req != -1)
+      submit(dom, "total_requests", flush_type_instance,
+             &(value_t){.derive = (derive_t)binfo->fl_req}, 1);
+    if (binfo->fl_total_times != -1) {
+      derive_t value = binfo->fl_total_times / 1000; // ns -> ms
+      submit(dom, "total_time_in_ms", flush_type_instance,
+             &(value_t){.derive = value}, 1);
+    }
+  }
+}
+
 static int lv_config(const char *key, const char *value) {
   if (virInitialize() != 0)
     return 1;
@@ -500,6 +603,23 @@ static int lv_config(const char *key, const char *value) {
     return 0;
   }
 
+  if (strcasecmp(key, "ExtraStats") == 0) {
+    char *localvalue = sstrdup(value);
+    if (localvalue != NULL) {
+      char *exstats[EX_STATS_MAX_FIELDS];
+      int numexstats =
+          strsplit(localvalue, exstats, STATIC_ARRAY_SIZE(exstats));
+      for (int i = 0; i < numexstats; i++) {
+        if (strcasecmp(exstats[i], EX_STATS_DISK) == 0) {
+          DEBUG(PLUGIN_NAME " plugin: enabling extra stats for '%s'",
+                EX_STATS_DISK);
+          extra_stats |= ex_stats_disk;
+        }
+      }
+      sfree(localvalue);
+    }
+  }
+
   /* Unrecognised option. */
   return -1;
 }
@@ -527,6 +647,40 @@ static void lv_disconnect(void) {
   WARNING(PLUGIN_NAME " plugin: closed connection to libvirt");
 }
 
+static int lv_domain_block_info(virDomainPtr dom, const char *path,
+                                struct lv_block_info *binfo) {
+#ifdef HAVE_BLOCK_STATS_FLAGS
+  virTypedParameterPtr params = NULL;
+  int nparams = 0;
+  int rc = -1;
+  int ret = virDomainBlockStatsFlags(dom, path, NULL, &nparams, 0);
+  if (ret < 0 || nparams == 0) {
+    VIRT_ERROR(conn, "getting the disk params count");
+    return -1;
+  }
+
+  params = calloc(nparams, sizeof(virTypedParameter));
+  if (params == NULL) {
+    ERROR("virt plugin: alloc(%i) for block=%s parameters failed.", nparams,
+          path);
+    return -1;
+  }
+  ret = virDomainBlockStatsFlags(dom, path, params, &nparams, 0);
+  if (ret < 0) {
+    VIRT_ERROR(conn, "getting the disk params values");
+    goto done;
+  }
+
+  rc = get_block_info(binfo, params, nparams);
+
+done:
+  virTypedParamsFree(params, nparams);
+  return rc;
+#else
+  return virDomainBlockStats(dom, path, &(binfo->bi), sizeof(binfo->bi));
+#endif /* HAVE_BLOCK_STATS_FLAGS */
+}
+
 static int lv_read(user_data_t *ud) {
   time_t t;
   struct lv_read_instance *inst = NULL;
@@ -640,29 +794,29 @@ static int lv_read(user_data_t *ud) {
 
   /* Get block device stats for each domain. */
   for (int i = 0; i < state->nr_block_devices; ++i) {
-    struct _virDomainBlockStats stats;
+    struct block_device *bdev = &(state->block_devices[i]);
+    struct lv_block_info binfo;
+    init_block_info(&binfo);
 
-    if (virDomainBlockStats(state->block_devices[i].dom,
-                            state->block_devices[i].path, &stats,
-                            sizeof stats) != 0)
+    if (lv_domain_block_info(bdev->dom, bdev->path, &binfo) < 0)
       continue;
 
-    char *type_instance = NULL;
-    if (blockdevice_format_basename && blockdevice_format == source)
-      type_instance = strdup(basename(state->block_devices[i].path));
-    else
-      type_instance = strdup(state->block_devices[i].path);
-
-    if ((stats.rd_req != -1) && (stats.wr_req != -1))
-      submit_derive2("disk_ops", (derive_t)stats.rd_req, (derive_t)stats.wr_req,
-                     state->block_devices[i].dom, type_instance);
+    char *type_instance = bdev->path;
+    char *path = NULL;
+    if (blockdevice_format_basename && blockdevice_format == source) {
+      path = strdup(bdev->path);
+      if (path == NULL) {
+        WARNING(PLUGIN_NAME
+                " plugin: error extracting the basename for '%s', skipped",
+                bdev->path);
+        continue;
+      }
+      type_instance = basename(path);
+    }
 
-    if ((stats.rd_bytes != -1) && (stats.wr_bytes != -1))
-      submit_derive2("disk_octets", (derive_t)stats.rd_bytes,
-                     (derive_t)stats.wr_bytes, state->block_devices[i].dom,
-                     type_instance);
+    disk_submit(&binfo, bdev->dom, type_instance);
 
-    sfree(type_instance);
+    sfree(path);
   } /* for (nr_block_devices) */
 
   /* Get interface stats for each domain. */