pcp
[Top] [All Lists]

Re: IB pmda and writing out the default config file

To: Max Matveev <makc@xxxxxxxxx>
Subject: Re: IB pmda and writing out the default config file
From: Martin Hicks <mort@xxxxxxx>
Date: Tue, 16 Jun 2009 13:22:24 -0500
Cc: pcp@xxxxxxxxxxx
In-reply-to: <18990.24528.172761.247069@xxxxxxxxxxxx>
References: <20090608171828.GA14199@xxxxxxxxxxxxxxxxxxxxxxxxx> <18990.24528.172761.247069@xxxxxxxxxxxx>
User-agent: Mutt/1.5.17 (2007-11-01)
On Tue, Jun 09, 2009 at 11:12:48PM +1000, Max Matveev wrote:
> 
> Perhaps a better way to deal with this would be to add some keywords
> to the config file to indicate local HCAs and to complain bitterly if
> they're not found.

In the end, I still don't love this solution because it requires manual
intervention from the user to change or remove the config file when they
change an HCA (or in my case when a diskless blade gets replaced in a
cluster).

How about this patch.  It rejigs things a lot, but essentially:

- No config file will be created unless the '-w' flag is passed to
  pmdaib. (this is a change)

- By default, if no config file is found, only local ports are monitored.
  (before we would always write out a config file containing only local
   ports)

- If a config file exists then it is the preferred source of information
  and perhaps not even local ports will be monitored. (before you could
  have edited the automatically created file to remove the local ports)


So in the case of a user who is using a custom config file (either to
name the instances differently, or to monitor remote ports) who replaces
an HCA, it will be up to them to edit the config file that they created.

For those who are using the ib pmda in its simplest mode with no config
file, no changes will be necessary when an HCA is replaced but they have
no guarantee of static device naming if they add an additional HCA.



diff --git a/src/pmdas/ib/daemon.c b/src/pmdas/ib/daemon.c
index 2a71151..12540cc 100644
--- a/src/pmdas/ib/daemon.c
+++ b/src/pmdas/ib/daemon.c
@@ -33,7 +33,8 @@ usage(void)
     fputs("Options:\n"
           "  -d domain  use domain (numeric) for metrics domain of PMDA\n"
          "  -l logfile write log into logfile rather than using default log 
name\n"
-         "  -c path to configuration file\n",
+         "  -c path to configuration file\n"
+         "  -w write the basic configuration file\n",
           stderr);
     exit(1);
 }
@@ -46,19 +47,22 @@ main(int argc, char **argv)
     pmdaInterface dispatch;
     char helppath[MAXPATHLEN];
     char *confpath = NULL;
-    char *p;
     int opt;
+    int writeconf = 0;
 
     __pmSetProgname(argv[0]);
     snprintf(helppath, sizeof(helppath), "%s%c" "ib" "%c" "help", 
                pmGetConfig("PCP_PMDAS_DIR"), sep, sep);
     pmdaDaemon(&dispatch, PMDA_INTERFACE_3, pmProgname, IB, "ib.log", 
helppath);
 
-    while ((opt = pmdaGetOpt(argc, argv, "D:c:d:l:?", &dispatch, &err)) != 
EOF) {
+    while ((opt = pmdaGetOpt(argc, argv, "D:c:d:l:w?", &dispatch, &err)) != 
EOF) {
        switch (opt) {
        case 'c':
            confpath = optarg;
            break;
+       case 'w':
+           writeconf = 1;
+           break;
        default:
            err++;
        }
@@ -69,7 +73,7 @@ main(int argc, char **argv)
     }
 
     pmdaOpenLog(&dispatch);
-    ibpmda_init(confpath, &dispatch);
+    ibpmda_init(confpath, writeconf, &dispatch);
     pmdaConnect(&dispatch);
     pmdaMain(&dispatch);
 
diff --git a/src/pmdas/ib/dso.c b/src/pmdas/ib/dso.c
index 33f10a8..4e506f8 100644
--- a/src/pmdas/ib/dso.c
+++ b/src/pmdas/ib/dso.c
@@ -37,5 +37,5 @@ ib_init (pmdaInterface * dispatch)
             pmGetConfig("PCP_PMDAS_DIR"), sep, sep);
     pmdaDSO(dispatch, PMDA_INTERFACE_3, "ibpmda", helppath);
 
-    ibpmda_init(NULL, dispatch);
+    ibpmda_init(NULL, 0, dispatch);
 }
diff --git a/src/pmdas/ib/ib.c b/src/pmdas/ib/ib.c
index ccfb079..c027f77 100644
--- a/src/pmdas/ib/ib.c
+++ b/src/pmdas/ib/ib.c
@@ -141,6 +141,70 @@ typedef struct port_state_s {
        char pcap[IB_ALLPORTCAPSTRLEN]; 
 } port_state_t;
 
+static char confpath[MAXPATHLEN];
+static int portcount;
+/* Line number while parsing the config file */
+static FILE *fconf;
+static int lcnt;
+
+#define print_parse_err(loglevel, fmt, args...) \
+    if (fconf) { \
+       __pmNotifyErr(loglevel, "%s(%d): " fmt, fconf, lcnt, args); \
+    } else { \
+       __pmNotifyErr(loglevel, fmt, args); \
+    }
+
+static void
+monitor_guid(pmdaIndom *itab, char *name, long long guid, int rport,
+           char *local, int lport)
+{
+    int inst;
+    hca_state_t *hca = NULL;
+    port_state_t *ps;
+    
+    if (pmdaCacheLookupName(itab[IB_HCA_INDOM].it_indom, local, NULL, 
+                          (void**)&hca) != PMDA_CACHE_ACTIVE) {
+       print_parse_err(LOG_ERR, "unknown HCA '%s' in 'via' clause\n", local);
+       return;
+    }
+
+    if ((lport >= UMAD_CA_MAX_PORTS) || (lport < 0)) {
+       print_parse_err(LOG_ERR, 
+                      "port number %d is out of bounds for HCA %s\n",
+                       lport, local);
+       return;
+    }
+    
+    if (hca->lports[lport].hndl == NULL) {
+       print_parse_err(LOG_ERR, 
+                      "port %s:%d has failed initialization\n",
+                       local, lport);
+       return;
+    }
+    
+    if ((ps = (port_state_t *)calloc(1, sizeof(port_state_t))) == NULL) {
+       __pmNotifyErr (LOG_ERR, "Out of memory to save state for %s\n", name);
+       return;
+    }
+
+    ps->guid = guid;
+    ps->remport = rport;
+    ps->lport = hca->lports + lport;
+    ps->portid.lid = -1;
+    ps->timeout = 1000;
+
+    if ((inst = pmdaCacheStore(itab[IB_PORT_INDOM].it_indom,
+                           PMDA_CACHE_ADD, name, ps)) < 0) {
+       __pmNotifyErr(LOG_ERR, "Cannot add %s to the cache - %s\n",
+                       name, pmErrStr(inst));
+       free (ps);
+       return;
+    }
+
+    portcount++;
+}
+
+
 static int
 foreachport(hca_state_t *hst, void (*cb)(hca_state_t *, umad_port_t *, void *),
            void *closure)
@@ -166,7 +230,6 @@ foreachport(hca_state_t *hst, void (*cb)(hca_state_t *, 
umad_port_t *, void *),
 static void
 printportconfig (hca_state_t *hst, umad_port_t *port, void *arg)
 {
-    FILE *fconf = arg;
     uint64_t hguid = port->port_guid;
 
     __ntohll((char *)&hguid);
@@ -176,6 +239,20 @@ printportconfig (hca_state_t *hst, umad_port_t *port, void 
*arg)
             hst->ca.ca_name, port->portnum);
 }
 
+static void
+monitorport(hca_state_t *hst, umad_port_t *port, void *arg)
+{
+    pmdaIndom *itab = arg;
+    uint64_t hguid = port->port_guid;
+    char name[128];
+
+    __ntohll((char *)&hguid);
+    sprintf(name, "%s:%d", port->ca_name, port->portnum);
+
+    monitor_guid(itab, name, hguid, port->portnum, port->ca_name, 
port->portnum);
+}
+
+
 static int mgmt_classes[] = {IB_SMI_CLASS, IB_SMI_DIRECT_CLASS, 
                              IB_SA_CLASS, IB_PERFORMANCE_CLASS};
 static void
@@ -196,74 +273,10 @@ openumadport (hca_state_t *hst, umad_port_t *port, void 
*arg)
     lp->hndl = hndl;
 }
 
-int
-ib_load_config(const char *confpath, pmdaIndom *itab, unsigned int nindoms)
+static void
+parse_config(pmdaIndom *itab)
 {
-    char hcas[IBPMDA_MAX_HCAS][UMAD_CA_NAME_LEN];
-    hca_state_t *st = NULL;
-    int i, n;
-    int lcnt = 0;
     char buffer[2048];
-    FILE *fconf;
-    int (*closef)(FILE *) = fclose;
-    int portcount = 0;
-
-    if (nindoms <= IB_CNT_INDOM) {
-       return (-EINVAL);
-    }  
-
-    if (umad_init()) {
-       return (-EIO);
-    } 
-
-    if ((n = umad_get_cas_names(hcas, ARRAYSZ(hcas)))) {
-       if ((st = calloc (n, sizeof(hca_state_t))) == NULL) {
-           return (-ENOMEM);
-       }
-    } else {
-       return (0);
-    }
-
-    for (i=0; i < n; i++) {
-       if (umad_get_ca(hcas[i], &st[i].ca) == 0) {
-           int e = pmdaCacheStore (itab[IB_HCA_INDOM].it_indom, PMDA_CACHE_ADD,
-                                   st[i].ca.ca_name, &st[i].ca);
-
-           if (e >= 0) {
-               foreachport (st+i, openumadport, NULL);
-           } else {
-               __pmNotifyErr (LOG_ERR, 
-                               "Cannot add instance for %s to the "
-                               "cache - %s\n",
-                                st[i].ca.ca_name, pmErrStr(e));
-           }
-       }
-    }
-
-    /* Process config file - if the executable bit is set then assume
-     * that user wants it to be a script and run it, otherwise try loading
-     * it. If it cannot be loaded then create a default one and 
-     * process it again */
-    if (access(confpath, X_OK)) {
-       /* Not an executable, just read it */
-       if ((fconf = fopen (confpath, "r")) == NULL) {
-           if ((fconf = fopen (confpath, "w+")) != NULL) {
-               for (i=0; i < n; i++) {
-                   foreachport (st+i, printportconfig, fconf);
-               }
-               fseek (fconf, 0, SEEK_SET);
-           }
-       }
-    } else {
-       fconf = popen(confpath, "r");
-       closef = pclose;
-    }
-
-    if (fconf == NULL) {
-       __pmNotifyErr (LOG_CRIT, "Cannot open configuration file %s\n",
-                       confpath);
-       return (-ENOENT);
-    }  
 
     while ((fgets(buffer, sizeof(buffer)-1, fconf)) != NULL) {
        char *p;
@@ -285,64 +298,98 @@ ib_load_config(const char *confpath, pmdaIndom *itab, 
unsigned int nindoms)
            int rport;
            char local[128];
            int lport;
-           hca_state_t *hca = NULL;
-           port_state_t *ps;
 
            if (sscanf(p, "%[^ \t]%llx%d via %[^:]:%d",
                        name, &guid, &rport, local, &lport) != 5) {
                __pmNotifyErr (LOG_ERR, "%s(%d): cannot parse the line\n",
                               confpath, lcnt);
-           } else if (pmdaCacheLookupName(itab[IB_HCA_INDOM].it_indom,
-                                          local, NULL, 
-                                          (void**)&hca) != PMDA_CACHE_ACTIVE) {
-               __pmNotifyErr (LOG_ERR, 
-                              "%s(%d): unknown HCA '%s' in 'via' clause\n",
-                              confpath, lcnt, local);
-           } else if ((lport >= UMAD_CA_MAX_PORTS) || (lport < 0)) {
-               __pmNotifyErr (LOG_ERR, 
-                              "%s(%d): port number %d is out of bounds for "
-                              "HCA %s\n",
-                               confpath, lcnt, lport, local);
-           } else if (hca->lports[lport].hndl == NULL) {
-               __pmNotifyErr (LOG_ERR, 
-                              "%s(%d): port %s:%d has failed initialization\n",
-                               confpath, lcnt, local, lport);
-           } else if ((ps = (port_state_t *)calloc(1, sizeof(port_state_t))) 
== NULL) {
-               __pmNotifyErr (LOG_ERR, "Out of memory to save state for %s\n",
-                               name);
-           } else {
-               int inst;
-               ps->guid = guid;
-               ps->remport = rport;
-               ps->lport = hca->lports + lport;
-               ps->portid.lid = -1;
-               ps->timeout = 1000;
-
-               if ((inst = pmdaCacheStore (itab[IB_PORT_INDOM].it_indom,
-                                           PMDA_CACHE_ADD, name, ps)) < 0) {
-                       __pmNotifyErr (LOG_ERR, 
-                                      "Cannot add %s to the cache - %s\n",
-                                       name, pmErrStr(inst));
-                       free (ps);
-               } else {
-                       portcount++;
-               }
+               continue;
            }
+  
+           monitor_guid(itab, name, guid, rport, local, lport);
        }
     }
-    (*closef) (fconf);
+}
+
+int
+ib_load_config(const char *cp, int writeconf, pmdaIndom *itab, unsigned int 
nindoms)
+{
+    char hcas[IBPMDA_MAX_HCAS][UMAD_CA_NAME_LEN];
+    hca_state_t *st = NULL;
+    int i, n;
+    int (*closef)(FILE *) = fclose;
+
+    if (nindoms <= IB_CNT_INDOM)
+       return -EINVAL;
+
+    if (umad_init())
+       return -EIO;
+
+    if ((n = umad_get_cas_names(hcas, ARRAYSZ(hcas)))) {
+       if ((st = calloc (n, sizeof(hca_state_t))) == NULL)
+           return -ENOMEM;
+    } else
+       /* No HCAs */
+       return 0;
+
+    /* Open config file - if the executable bit is set then assume that
+     * user wants it to be a script and run it, otherwise try loading it.
+     */
+    strcpy(confpath, cp);
+    if (access(confpath, F_OK) == 0) {
+       if (writeconf) {
+           __pmNotifyErr(LOG_ERR,
+                   "Config file exists and writeconf arg was given to pmdaib.  
"
+                   "Using existing config file");
+           writeconf = 0;
+       }
+
+       if (access(confpath, X_OK)) {
+           /* Not an executable, just read it */
+           fconf = fopen (confpath, "r");
+       } else if (!writeconf) {
+           fconf = popen(confpath, "r");
+           closef = pclose;
+       } 
+    } else if (writeconf) {
+       fconf = fopen(confpath, "w");
+    }
+
+    for (i=0; i < n; i++) {
+       if (umad_get_ca(hcas[i], &st[i].ca) == 0) {
+           int e = pmdaCacheStore(itab[IB_HCA_INDOM].it_indom, PMDA_CACHE_ADD,
+                                   st[i].ca.ca_name, &st[i].ca);
+
+           if (e < 0) {
+               __pmNotifyErr(LOG_ERR, 
+                       "Cannot add instance for %s to the cache - %s\n",
+                        st[i].ca.ca_name, pmErrStr(e));
+               continue;
+           }
+
+           foreachport(st+i, openumadport, NULL);
+           if (fconf == NULL || writeconf)
+               /* No config file - monitor local ports */
+               foreachport(st+i, monitorport, itab);
+           if (writeconf)
+               foreachport(st+i, printportconfig, fconf);
+       }
+    }
+
+    if (fconf) {
+       parse_config(itab);
+       (*closef)(fconf);
+    }
 
     if (!portcount) {
-       __pmNotifyErr (LOG_INFO, 
-                      "No ports found in configuration file %s\n",
-                      confpath);
+       __pmNotifyErr(LOG_INFO, "No IB ports found to monitor");
     }
-               
+
     itab[IB_CNT_INDOM].it_set = (pmdaInstid 
*)calloc(ARRAYSZ(mad_cnt_descriptors), 
                                                     sizeof(pmdaInstid));
 
     if (itab[IB_CNT_INDOM].it_set == NULL) {
-       return (-ENOMEM);
+       return -ENOMEM;
     }
 
     itab[IB_CNT_INDOM].it_numinst = ARRAYSZ(mad_cnt_descriptors);
@@ -351,8 +398,8 @@ ib_load_config(const char *confpath, pmdaIndom *itab, 
unsigned int nindoms)
        itab[IB_CNT_INDOM].it_set[i].i_name = mad_cnt_descriptors[i].name;
 
     }
-       
-    return (0);
+
+    return 0;
 }
 
 static char *
@@ -869,7 +916,7 @@ ib_store(pmResult *result, pmdaExt *pmda)
                    return (PM_ERR_INST);
                }
                break;
-           
+
            case METRIC_ib_control_hiwat:
                if ((id < 0) ||
                    (id > pmda->e_indoms[IB_CNT_INDOM].it_numinst)) {
@@ -884,7 +931,7 @@ ib_store(pmResult *result, pmdaExt *pmda)
            }
         }
     }
-    return (0);
+    return 0;
 }
 
 
diff --git a/src/pmdas/ib/ibpmda.h b/src/pmdas/ib/ibpmda.h
index 153743b..fae2c3c 100644
--- a/src/pmdas/ib/ibpmda.h
+++ b/src/pmdas/ib/ibpmda.h
@@ -22,10 +22,10 @@
 #include "domain.h"
 
 
-void ibpmda_init (const char *configpath, pmdaInterface *);
+void ibpmda_init (const char *configpath, int, pmdaInterface *);
 
 int ib_fetch_val(pmdaMetric *, unsigned int, pmAtomValue *);
-int ib_load_config(const char *, pmdaIndom *, unsigned int);
+int ib_load_config(const char *, int, pmdaIndom *, unsigned int);
 void ib_rearm_for_update(void *);
 void ib_reset_perfcounters (void *);
 int ib_store(pmResult *, pmdaExt *);
diff --git a/src/pmdas/ib/pmda.c b/src/pmdas/ib/pmda.c
index 6a8e02d..d5557b3 100644
--- a/src/pmdas/ib/pmda.c
+++ b/src/pmdas/ib/pmda.c
@@ -297,7 +297,7 @@ ib_fetch(int numpmid, pmID pmidlist[], pmResult **resp, 
pmdaExt *pmda)
 }
 
 void
-ibpmda_init(const char *confpath, pmdaInterface *dp)
+ibpmda_init(const char *confpath, int writeconf, pmdaInterface *dp)
 {
     char defconf[MAXPATHLEN];
     int sep = __pmPathSeparator();
@@ -320,7 +320,7 @@ ibpmda_init(const char *confpath, pmdaInterface *dp)
     }
 
 
-    if ((dp->status = ib_load_config (confpath, indomtab, ARRAYSZ(indomtab))))
+    if ((dp->status = ib_load_config(confpath, writeconf, indomtab, 
ARRAYSZ(indomtab))))
        return;
 
     for (i=0; i < ARRAYSZ(indomtab); i++) {


<Prev in Thread] Current Thread [Next in Thread>