pcp
[Top] [All Lists]

Re: [pcp] pmrep: improve command line parsing

To: Nathan Scott <nathans@xxxxxxxxxx>
Subject: Re: [pcp] pmrep: improve command line parsing
From: Marko Myllynen <myllynen@xxxxxxxxxx>
Date: Thu, 30 Jun 2016 11:04:55 +0300
Cc: pcp developers <pcp@xxxxxxxxxxx>
Delivered-to: pcp@xxxxxxxxxxx
In-reply-to: <1966538773.3111430.1467243521666.JavaMail.zimbra@xxxxxxxxxx>
Organization: Red Hat
References: <5767D307.9000002@xxxxxxxxxx> <5774061C.8070001@xxxxxxxxxx> <1966538773.3111430.1467243521666.JavaMail.zimbra@xxxxxxxxxx>
Reply-to: Marko Myllynen <myllynen@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0
Hi,

On 2016-06-30 02:38, Nathan Scott wrote:
> ----- Original Message -----
>> [...]
>> In general, of course, something so common that an application reads
>> its configuration from a file and then possibly overrides those values
>> with the ones the user provides on the command line shouldn't be this
>> hard. But I'm out of ideas so if noone can come up with a better
>> solution, I'm afraid we need to revert the previous patch.
> 
> Hmmm one other option just occurred to me reading this; you might find
> using the environment variables (PCP_ARCHIVE, PCP_HOST) provides a way
> to inject these values reliably & more simply.  i.e. grab source names
> from the config, inject em into appropriate env var (deciding whether
> to override env in the process or not, yet another wrinkle) ... it may
> become simpler that way, since pmGetOptions handles that internally.

This didn't help ...

> Or it may just get trickier still.  :)  Lemme know commit id if revert
> is the way to go here, thanks.

... but I now finally figured out how to do it and why it didn't work
before. Turns out that I assumed that pmSetOptionArchive() etc would
also set the context correctly, but that was not the case.

The below patch extends QA, exposes few methods to Python, uses
them to restore pmrep functionality as intended, and also adds support
for defining local context in the configuration file.

If you can see a more straightforward way to do this, please show how :)

---
 qa/1069                 | 10 ++++++++++
 qa/1069.out             | 16 ++++++++++++++++
 src/pmrep/pmrep.conf.5  |  7 ++++---
 src/pmrep/pmrep.py      | 49 +++++++++++++++++++++++++++++++++++++------------
 src/python/pcp/pmapi.py |  3 +++
 src/python/pmapi.c      | 29 +++++++++++++++++++++++++++++
 6 files changed, 99 insertions(+), 15 deletions(-)

diff --git a/qa/1069 b/qa/1069
index 20b849f..ddbc1be 100755
--- a/qa/1069
+++ b/qa/1069
@@ -158,6 +158,16 @@ echo "---"
 pmrep -s 5 $log2 -p -c $tmp.pconfig mem.util.free
 echo "---"
 pmrep -s 5 $log2 -t 3s -p -H -c $tmp.pconfig mem.util.free
+echo "---"
+echo '[source]' > $tmp.pconfig
+echo "source = $here/archives/rep" >> $tmp.pconfig
+echo 'kernel.all.uptime = uptime,,,,8' >> $tmp.pconfig
+pmrep -s 5 -c $tmp.pconfig -z -p -u :source
+echo "---"
+echo '[source]' > $tmp.pconfig
+echo 'source = wrong.example.com' >> $tmp.pconfig
+echo 'kernel.all.uptime = uptime,,,,8' >> $tmp.pconfig
+pmrep -s 5 $log2 -c $tmp.pconfig -p -u :source
 
 # success, all done
 echo "== done"
diff --git a/qa/1069.out b/qa/1069.out
index 9573b50..ed3a54e 100644
--- a/qa/1069.out
+++ b/qa/1069.out
@@ -198,4 +198,20 @@ Incompatible configuration file version (read v99, need 
v1).
 00:47:07     38220
 00:47:10     38220
 00:47:13     38220
+---
+            uptime
+               sec
+14:39:13    507689
+14:39:14    507690
+14:39:15    507691
+14:39:16    507692
+14:39:17    507693
+---
+            uptime
+               sec
+00:48:01  24924286
+00:49:01  24924346
+00:50:01  24924406
+00:51:01  24924466
+00:52:01  24924526
 == done
diff --git a/src/pmrep/pmrep.conf.5 b/src/pmrep/pmrep.conf.5
index 768c3b8..36aaf07 100644
--- a/src/pmrep/pmrep.conf.5
+++ b/src/pmrep/pmrep.conf.5
@@ -107,9 +107,10 @@ only currently supported value is \fB1\fR.
 source (string)
 .RS 4
 Indicates the source for metrics. Interpreted as a PCP archive if the
-string contains a slash (``/''), otherwise interpreted as a hostname.
-Corresponding command line paraters are \fB-a\fR and \fB-h\fR. Defaults
-to \fBlocal:\fR (see
+string contains a slash (``/''). If set to the special character ``@'',
+local DSO PMDA context is used. Otherwise interpreted as a hostname.
+Corresponding command line paraters are \fB-a\fR, \fB-h\fR, and
+\fB-L\fR. Defaults to \fBlocal:\fR (see
 .BR PCPIntro (1)).
 .RE
 .P
diff --git a/src/pmrep/pmrep.py b/src/pmrep/pmrep.py
index 430f894..05d69d6 100755
--- a/src/pmrep/pmrep.py
+++ b/src/pmrep/pmrep.py
@@ -385,10 +385,7 @@ class PMReporter(object):
         if value in ('false', 'False', 'n', 'no', 'No'):
             value = 0
         if name == 'source':
-            if '/' in value:
-                self.opts.pmSetOptionArchive(value)
-            else:
-                self.opts.pmSetOptionHost(value) # RHBZ#1289911
+            self.source = value
         elif name == 'samples':
             self.opts.pmSetOptionSamples(value)
             self.samples = self.opts.pmGetOptionSamples()
@@ -423,6 +420,7 @@ class PMReporter(object):
 
     def read_cmd_line(self):
         """ Read command line options """
+        pmapi.c_api.pmSetOptionFlags(pmapi.c_api.PM_OPTFLAG_DONE)  # Do later
         pmapi.c_api.pmSetOptionFlags(pmapi.c_api.PM_OPTFLAG_POSIX) # 
RHBZ#1289912
         if pmapi.c_api.pmGetOptionsFromList(sys.argv):
             raise pmapi.pmUsageErr()
@@ -516,17 +514,44 @@ class PMReporter(object):
                     self.metrics[m] = confmet[m]
 
     def connect(self):
-        """ Establish a PMAPI context to archive, host or local, via args """
-        optctx = self.opts.pmGetOptionContext()
-        if optctx == pmapi.c_api.PM_CONTEXT_ARCHIVE:
+        """ Establish a PMAPI context """
+        context = None
+
+        if pmapi.c_api.pmGetOptionArchives():
+            context = pmapi.c_api.PM_CONTEXT_ARCHIVE
+            self.opts.pmSetOptionContext(pmapi.c_api.PM_CONTEXT_ARCHIVE)
             self.source = self.opts.pmGetOptionArchives()[0]
-        elif optctx == pmapi.c_api.PM_CONTEXT_HOST:
+        elif pmapi.c_api.pmGetOptionHosts():
+            context = pmapi.c_api.PM_CONTEXT_HOST
+            self.opts.pmSetOptionContext(pmapi.c_api.PM_CONTEXT_HOST)
             self.source = self.opts.pmGetOptionHosts()[0]
-        elif optctx == pmapi.c_api.PM_CONTEXT_LOCAL:
+        elif pmapi.c_api.pmGetOptionLocalPMDA():
+            context = pmapi.c_api.PM_CONTEXT_LOCAL
+            self.opts.pmSetOptionContext(pmapi.c_api.PM_CONTEXT_LOCAL)
             self.source = None
-        else:
-            optctx = pmapi.c_api.PM_CONTEXT_HOST
-        self.context = pmapi.pmContext(optctx, self.source)
+
+        if not context:
+            if '/' in self.source:
+                context = pmapi.c_api.PM_CONTEXT_ARCHIVE
+                self.opts.pmSetOptionArchive(self.source)
+                self.opts.pmSetOptionContext(pmapi.c_api.PM_CONTEXT_ARCHIVE)
+            elif self.source != '@':
+                context = pmapi.c_api.PM_CONTEXT_HOST
+                self.opts.pmSetOptionHost(self.source)
+                self.opts.pmSetOptionContext(pmapi.c_api.PM_CONTEXT_HOST)
+            else:
+                context = pmapi.c_api.PM_CONTEXT_LOCAL
+                self.opts.pmSetOptionLocalPMDA()
+                self.opts.pmSetOptionContext(pmapi.c_api.PM_CONTEXT_LOCAL)
+                self.source = None
+
+        # All options set, finalize configuration
+        flags = self.opts.pmGetOptionFlags()
+        self.opts.pmSetOptionFlags(flags | pmapi.c_api.PM_OPTFLAG_DONE)
+        pmapi.c_api.pmEndOptions()
+
+        self.context = pmapi.pmContext(context, self.source)
+
         if pmapi.c_api.pmSetContextOptions(self.context.ctx, self.opts.mode, 
self.opts.delta):
             raise pmapi.pmUsageErr()
 
diff --git a/src/python/pcp/pmapi.py b/src/python/pcp/pmapi.py
index dad771d..e4ba79a 100644
--- a/src/python/pcp/pmapi.py
+++ b/src/python/pcp/pmapi.py
@@ -840,6 +840,9 @@ class pmOptions(object):
     def pmGetOptionFlags(self):
         return c_api.pmGetOptionFlags()
 
+    def pmSetOptionContext(self, context):
+        return c_api.pmSetOptionContext(context)
+
     def pmSetOptionFlags(self, flags):
         return c_api.pmSetOptionFlags(flags)
 
diff --git a/src/python/pmapi.c b/src/python/pmapi.c
index 96d1f2b..3ed96e2 100644
--- a/src/python/pmapi.c
+++ b/src/python/pmapi.c
@@ -486,6 +486,21 @@ setShortUsage(PyObject *self, PyObject *args, PyObject 
*keywords)
 }
 
 static PyObject *
+setOptionContext(PyObject *self, PyObject *args, PyObject *keywords)
+{
+    int context;
+    char *keyword_list[] = {"context", NULL};
+
+    if (!PyArg_ParseTupleAndKeywords(args, keywords,
+                       "i:pmSetOptionContext", keyword_list, &context))
+       return NULL;
+
+    options.context = context;
+    Py_INCREF(Py_None);
+    return Py_None;
+}
+
+static PyObject *
 setOptionFlags(PyObject *self, PyObject *args, PyObject *keywords)
 {
     int flags;
@@ -830,6 +845,14 @@ getOptionsFromList(PyObject *self, PyObject *args, 
PyObject *keywords)
 }
 
 static PyObject *
+endOptions(PyObject *self, PyObject *args, PyObject *keywords)
+{
+    __pmEndOptions(&options);
+    Py_INCREF(Py_None);
+    return Py_None;
+}
+
+static PyObject *
 setContextOptions(PyObject *self, PyObject *args, PyObject *keywords)
 {
     int sts, ctx, step, mode, delta;
@@ -1251,6 +1274,9 @@ static PyMethodDef methods[] = {
     { .ml_name = "pmSetShortUsage",
        .ml_meth = (PyCFunction) setShortUsage,
         .ml_flags = METH_VARARGS | METH_KEYWORDS },
+    { .ml_name = "pmSetOptionContext",
+       .ml_meth = (PyCFunction) setOptionContext,
+        .ml_flags = METH_VARARGS | METH_KEYWORDS },
     { .ml_name = "pmSetOptionFlags",
        .ml_meth = (PyCFunction) setOptionFlags,
         .ml_flags = METH_VARARGS | METH_KEYWORDS },
@@ -1269,6 +1295,9 @@ static PyMethodDef methods[] = {
     { .ml_name = "pmGetNonOptionsFromList",
        .ml_meth = (PyCFunction) getNonOptionsFromList,
         .ml_flags = METH_VARARGS | METH_KEYWORDS },
+    { .ml_name = "pmEndOptions",
+       .ml_meth = (PyCFunction) endOptions,
+        .ml_flags = METH_NOARGS },
     { .ml_name = "pmSetContextOptions",
        .ml_meth = (PyCFunction) setContextOptions,
         .ml_flags = METH_VARARGS | METH_KEYWORDS},

Thanks,

-- 
Marko Myllynen

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