monami-devel — Discussion of MonAMI development, core and plugins

[MonAMI-devel] further MySQL plugin improvements, do you want patches?

[Justin S. <gre...@gm...>]

Hi,

I've made some additional changes to the MySQL plugin because:
a) For charting purposes, calculating the delta between the current and
previously collected sample for any given row is necessary.
b) This lookup is expensive and cumbersome in SQL, particularly when a table
contains rows from more than one host.
c) There is no good way in MySQL to optimize this lookup.  It performs very
poorly for more than a few rows.  It is more efficient to look up the value
with an index read at the time the value is inserted.

In order to facilitate looking up the previous sample, I've added a new
column "prev_collected DATETIME NULL" which contains the `collected` value
from the previous sample.  This requires a read from the reporting table
before it is written into.  Issue b) above presents another problem, in that
a column (for us a mapped FQDN value called `hostname`) must be used as an
additional expression in the where clause.

I've added a new configuration directive to the MySQL plugin config file
called 'keyfield'.  This value may be set to the column which in when added
to `collected` yields a unique row.  An index is also automatically created
on `collected`, or `keyfield`,`collected`, when 'keyfield' is/is not
defined, respectively.

If there is no 'keyfield' value in the config file, the following lookup is
performed before the INSERT:
select max(collected)
  from reporting_table
where collected < SAMPLE_TIMESTAMP /*calculated in the plugin*/

If there is a keyfield value in the config file, then a similar lookup is
performed (in this example 'keyfield=hostname'):
select max(collected)
  from reporting_table
where collected < SAMPLE_TIMESTAMP
    and hostname = '%s' /* value is replaced with value of the hostname
field from the datatree*/

Are you interested in receiving these patches?

Re: [MonAMI-devel] Building RPMs from CVS

[Paul M. <p.m...@ph...>]

On Friday 03 July 2009 15:01:08 Stephen Childs wrote:
> Are there instructions anywhere for doing this? I would like to create
> some RPMs based on the current CVS version of MonAMI.

Not really, but (in general) the process is fairly simple: run configure to 
generate the RPM package (.spec) file, copy this somewhere safe.  Do a "make 
release" to generate a tar-ball. Then do the usual "rpmbuild -ba" (or similar) 
with the spec file and the tar-ball.  This can probably be made into a make 
target, further simplifying the process.

One problem is that there's no direct support for building a "CVS version"  
(i.e., a version with today's date embedded, so people know it's not a proper 
release).  The current build system thinks all versions are "real" releases.  
To hack around this, I do:

  sed -ie "s/\(AC_INIT.*([^,]*\), *\([^,]*\), *\(.*\)/\1, \2cvs$(date 
+%Y%m%d), \3 foo/" configure.ac
  autoconf

before starting the process  (and mentally remind myself to implement 
something less ugly each time ;-)

There's also currently a chicken-and-egg problem in determining the packaging 
of the dpm plugin: the plugin might or might not be built (depending on 
availability of the dpm library), but the packaging of the plugins is hard-
coded.  I'm still trying to figure out how best to solve this one.

HTH,

Paul.

Re: [MonAMI-devel] Fwd: Re: [PATCH] don't add token total size to datatree

[Paul M. <p.m...@ph...>]

Hi Stephen,

On Friday 03 July 2009 15:54:43 Stephen Childs wrote:
> > [mg-frame-dpm.php to plot only "used" and "free" metrics for tokens]
> "\.tokens\.$token", ""
>
> I've tried things like "[free|used]" but that doesn't work.

The pattern matching is regular expression (the preg_match() function in PHP).  
So, "(free|used)" should work.

The function matching_rrds() builds a RE by adding a dot if the pre or post 
filters are specified.  Having both means one has an RE like:

  ${pre}\.(.)+\.${post}\.rrd$

Where ${pre} and ${post} are the arguments to the build_series() method.

So, the filenames must match 
	.tokens.$token.<something>.free.rrd
or
	.tokens.$token.<something>.used.rrd

HTH,

Paul.

Re: [MonAMI-devel] Fwd: Re: [PATCH] don't add token total size to datatree

[Stephen C. <ch...@cs...>]

Paul Millar wrote:
> In the mean time, how about approaching the problem from the other side: fix 
> mg-frame-dpm.php so it plots only used and free, so avoid including the total 
> metric?

It looks like it would be in dpm-graph.php?

What pre- and post- patterns should I use to just match "free" and "used"?

The existing patterns are:

"\.tokens\.$token", ""

I've tried things like "[free|used]" but that doesn't work.

Stephen


-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

[MonAMI-devel] Building RPMs from CVS

[Stephen C. <ch...@cs...>]

Are there instructions anywhere for doing this? I would like to create 
some RPMs based on the current CVS version of MonAMI.

Stephen
-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

Re: [MonAMI-devel] [PATCH] Further tweak to DPM plugin

[Paul M. <p.m...@ph...>]

On Tuesday 09 June 2009 13:22:09 Stephen Childs wrote:
> -                  "GROUP BY fs, poolname;" ) != 0) {
> +                  "GROUP BY fs, poolname, host;" ) != 0) {

Done; thanks!

Paul.

PS.
If possible, please send a complete diffs (generated from the CVS parent 
directory) preferably with a short CHANGELOG entry.

[MonAMI-devel] [PATCH] Further tweak to DPM plugin

[Stephen C. <ch...@cs...>]

This also needs to be added to the SQL query in dpm_get_fsspace to avoid 
FS with the same names in the same pools being aggregated:

-                  "GROUP BY fs, poolname;" ) != 0) {
+                  "GROUP BY fs, poolname, host;" ) != 0) {

-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

[MonAMI-devel] Fwd: Re: [PATCH] don't add token total size to datatree

[Paul M. <p.m...@ph...>]

----------  Forwarded Message  ----------

Subject: Re: [MonAMI-devel] [PATCH] don't add token total size to datatree
Date: Thursday 28 May 2009
From: Paul Millar <pau...@de...>
To: mon...@li..., Ste...@cs...

On Wednesday 27 May 2009 11:12:24 Stephen Childs wrote:
> This doesn't need to be explicitly published as it can be derived from
> used and free. If it is put in the datatree it ends up getting plotted
> along with used and free, which generates ridiculous graphs.

Kind-of, yes.

I absolutely agree plotting used, free and total on the same graph looks 
ridiculous and I also that many monitoring systems are able to calculate the 
total from used and free (but, perhaps not all).

However, I'm a little reluctant to remove the metric too quickly.  I think we 
should first check with Tomas Kouba (who added support for these metrics) if he 
still needs "total".  Also, it would probably be a good idea to fire off an 
email on the monami-users mailing list to check that nobody's using these 
total metrics.

In the mean time, how about approaching the problem from the other side: fix 
mg-frame-dpm.php so it plots only used and free, so avoid including the total 
metric?

Also, monami-core should be updated to support the '*' wildcard (it's on my 
TODO list ;-).  That way, one could veto only the "total" metrics:

	[sample]
	select = dpm, !dpm.tokens.*.total

Cheers,

Paul.

-------------------------------------------------------

[MonAMI-devel] Fwd: Re: [PATCH] make dpm fs names canonical by prepending server name

[Paul M. <p.m...@ph...>]

----------  Forwarded Message  ----------

Subject: Re: [MonAMI-devel] [PATCH] make dpm fs names canonical by prepending 
server name
Date: Thursday 28 May 2009
From: Paul Millar <pau...@de...>
To: mon...@li..., Ste...@cs...

On Wednesday 27 May 2009 10:36:02 Stephen Childs wrote:
> Paul Millar wrote:
> > A couple of issues...
>
> Modified patch attached should address all the issues you raised.

Sorry, a couple more points:   I'm happy to commit the code, but I wanted to 
just double-check the code's doing what you want it to.

The patch/code currently refrains from adding a metric if the database returns 
a row with the host field that isn't the FQDN.  I don't know DPM that well: is 
it possible that the host field could contain only a short name?

Code like:

	[...]
	if (dot = strchr(row[3], '.') )
		*dot = '\0';

	/* add leaf node to appropriate branch */
	len = strlen(row[0]) + strlen(row[3]) + 1;
	[...]

would support the host field containing either short- or FQDN- hostname.

(Useful tip:  with gcc and -Wall, you should get a warning from the single '=' 
as it's all too easy to accidentally omit the second equals when writing a 
equality test.  Writing the comparison with an extra set of parentheses:

	if( (dot = strchr(row[3], '.')))

suppresses this warning.)


+      message( log_error, "problem with filesystem name in 
dpm_get_fsspace()\n");

If you want to refrain from adding a metric when the host field isn't a FQDN, 
you should give a better error message.  IMHO, "problem" is a rather vague 
term and I guess the issue is with the hostname rather than filesystem name.

> P.S. thanks for the C tips, it's rare that I get a chance to write any
> these days so I'm very rusty!

I know.  Everyone seems to be happy with python, PHP, ruby, groovy, ...; it's 
rare to have good honest C code to play with!

Cheers,

Paul.


-------------------------------------------------------

Re: [MonAMI-devel] [PATCH] add support for dpm querying pool information

[Paul M. <p.m...@ph...>]

On Wednesday 27 May 2009 11:10:48 Stephen Childs wrote:
[patch]

Committed.

As an aside, the following:

void dpm_get_poolspace(dpm_handle_t *h, void *root)
{
  [...]
  char dpm_errbuf[256];
  [...]

  if (dpm_seterrbuf (dpm_errbuf, sizeof(dpm_errbuf)) == 0) {

allocates the error buffer off of the stack.  This is potentially hazardous if 
dpm code tries to write an error message after the function returns. 

I think I have a solution for this using some pthreads single-shot in dpm-
plugin monami_open() function.

Also, there's a funky XSTR() macro for the BYTES_IN_MEGABYTE 
BYTES_IN_MEGABYTE_STR thing.

Cheers,

Paul.

Re: [MonAMI-devel] [PATCH] select cns_db before querying fsspace

[Paul M. <p.m...@ph...>]

On Wednesday 27 May 2009 11:13:13 Stephen Childs wrote:
> a mini-patch ensuring that the right DB is selected before querying mysql.

Committed.

Ta,

Paul.

Re: [MonAMI-devel] Choosing lib/lib64 with autoconf

[Paul M. <p.m...@ph...>]

On Wednesday 27 May 2009 10:33:39 Stephen Childs wrote:
> It appears that the variable build_cpu I was using to check the arch
> requires the AC_CANONICAL_BUILD macro to be executed (see patch below).
>
> This works but requires the files config.sub and config.guess from
> automake to be included in the monami distro. Do you think this is OK or
> is there a better way to decide lib/lib64?

I think the *real* solution would be for the DPM client RPM to include a 
dpm_config script (like mysql_config, pbs_config, net-snmp-config, etc ...) that 
can be queried for information like this.  Without this, we have to second-
guess the file's location; e.g., the people packaging DPM could (in theory) put 
64-bit libraries in $PREFIX/lib.  I've submitted a bug in Savannah to track 
this:

	https://savannah.cern.ch/bugs/?50953

In the absence of this script, I think you're on the right track here; 
however, I think you want the AC_CANONICAL_HOST (and $host_cpu variable) 
rather than AC_CANONICAL_BUILD:

https://www.linuxquestions.org/questions/slackware-14/explaining-build-host-
target-509739/

Paul.

Re: [MonAMI-devel] Batch of patches

[Paul M. <p.m...@ph...>]

On Wednesday 27 May 2009 09:59:52 Stephen Childs wrote:
> That's great. I am just fixing up the patches with issues and will send on
> new versions.

Cool.

> > I've also added support in autoconf for building MonAMI if the DPM
> > libraries / include files are somewhere other than /opt/lcg and added the
> > libdpm.so dependency to the DPM plugin RPM.
>
> Could you set the default include to the standard location
> (/opt/lcg/include/dpm) please?

Oops!  Sorry about that.  It should now be fixed.

Cheers,

Paul.

[MonAMI-devel] [PATCH] select cns_db before querying fsspace

[Stephen C. <ch...@cs...>]

a mini-patch ensuring that the right DB is selected before querying mysql.

Stephen
-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

[MonAMI-devel] [PATCH] don't add token total size to datatree

[Stephen C. <ch...@cs...>]

This doesn't need to be explicitly published as it can be derived from 
used and free. If it is put in the datatree it ends up getting plotted 
along with used and free, which generates ridiculous graphs.

Stephen
-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

Re: [MonAMI-devel] [PATCH] add support for dpm querying pool information

[Stephen C. <ch...@cs...>]

Paul Millar wrote:
> +#include <dpm_api.h>
> 
> The first is the most trivial one:  for purely cosmetic reasons, I'd put 
> #include <dpm_api.h> nearer where <mysql/mysql.h> is included.  I try to 
> include headers in order: first system headers, then system-wide installed 
> headers, then headers from MonAMI-core, then plugin-local headers.  There's no 
> particular strong requirement for this, though, just my personal style ;-)

Done.

> +void dpm_get_poolspace(dpm_handle_t *h, void *root)
> 
> You should also add this new function to the getvalues.h header file.

Done.

> +  /* get pool size from DPM */
> +  if ( (dpm_getpools (&npools, &dpm_pools)) ==0) {
> 
> Although MonAMI-core redirects stderr and stdout to /dev/null, I think it 
> would be cleaner to redirect the output so it doesn't attempt to write to 
> stdout or stderr (using dpm_seterrbuf() function).

Done.

> Also, it would be good to adjust the timeout options so dpm_getpools() will 
> always return ... eventually.  The default timeout (1 week) is perhaps too 
> long:
> 	https://twiki.cern.ch/twiki/bin/view/LCG/DpmApi

This timeout seems to apply to the command-line dpns tools? It isn't 
mentioned in the API documentation.


> +    message( log_info,"DPM reports %d pools\n", npools);
> 
> IMHO, log_info is too high for this; logging at log_debug would be better.

Absolutely, it just slipped through ...

> It should be possible to use one of the macros to generate the metric without 
> building a string and then parsing it.  For example:
> 
>  result_pool_free = DATATREE_NEW_UINT32( "free", (dpm_pools[i].free)/1000000),
> 		DATATREE_ATTR_NORMAL, "MB");

Done.

> +  } else {
> +    message( log_error,"Could not get information from DPM\n");
> +  }
> 
> It would be nice to be a bit more forthcoming with the error message.  The 
> value of serrno may give a hint to the end-user where the problem lies:

Done.

I have also explicitly selected the "cns_db" before querying mysql.

Stephen

-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

Re: [MonAMI-devel] [PATCH] make dpm fs names canonical by prepending server name

[Stephen C. <ch...@cs...>]

Paul Millar wrote:
> Hi Stephen,
> 
> A couple of issues...

Modified patch attached should address all the issues you raised.

Stephen
P.S. thanks for the C tips, it's rare that I get a chance to write any 
these days so I'm very rusty!


-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

[MonAMI-devel] Choosing lib/lib64 with autoconf

[Stephen C. <ch...@cs...>]

It appears that the variable build_cpu I was using to check the arch 
requires the AC_CANONICAL_BUILD macro to be executed (see patch below).

This works but requires the files config.sub and config.guess from 
automake to be included in the monami distro. Do you think this is OK or 
is there a better way to decide lib/lib64?

(see http://www.delorie.com/gnu/docs/autoconf/autoconf_127.html)



Index: configure.ac
===================================================================
RCS file: /cvsroot/monami/MonAMI/configure.ac,v
retrieving revision 1.68
diff -u -r1.68 configure.ac
--- configure.ac	27 May 2009 07:21:35 -0000	1.68
+++ configure.ac	27 May 2009 08:30:31 -0000
@@ -11,9 +11,9 @@
  AC_ARG_WITH(dot-dir, AC_HELP_STRING([--with-dot-dir=<dir>],[look for dot 
(from GraphViz) in directory <dir>]))
  AC_ARG_WITH(pbs-config-dir, 
AC_HELP_STRING([--with-pbs-config-dir=<dir>],[look for pbs-config script 
in <dir>]))
  AC_ARG_WITH(net-snmp-config-dir, 
AC_HELP_STRING([--with-net-snmp-config-dir=<dir>],[look for 
net-snmp-config script in <dir>]))
-AC_ARG_WITH(dpm-dir, AC_HELP_STRING([--with-dpm-dir=<dir>],[look for DPM 
libraries and header files under <dir>]), [], [with_dpm_dir=/opt/dpm])
-
+AC_ARG_WITH(dpm-dir, AC_HELP_STRING([--with-dpm-dir=<dir>],[look for DPM 
libraries and header files under <dir>]), [], [with_dpm_dir=/opt/lcg])

+AC_CANONICAL_BUILD

  # Checks for programs.
  AC_PROG_AWK
@@ -153,7 +153,7 @@
   [AC_SUBST([libfluidsynth], [fluidsynth]) 
AC_DEFINE([HAVE_LIBFLUIDSYNTH], [], [Whether fluidsynth library is 
available])])

  # Look for DPM libraries
-if test "x$build_cpu" == "x86_64"
+if test "x$build_cpu" == "xx86_64"
  then
    dpm_libs="-L $with_dpm_dir/lib64"
  else

-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

Re: [MonAMI-devel] Batch of patches

[Stephen C. <ch...@cs...>]

This is the include path patch:


Index: configure.ac
===================================================================
RCS file: /cvsroot/monami/MonAMI/configure.ac,v
retrieving revision 1.68
diff -u -r1.68 configure.ac
--- configure.ac	27 May 2009 07:21:35 -0000	1.68
+++ configure.ac	27 May 2009 08:02:31 -0000
@@ -11,7 +11,7 @@
  AC_ARG_WITH(dot-dir, AC_HELP_STRING([--with-dot-dir=<dir>],[look for dot 
(from GraphViz) in directory <dir>]))
  AC_ARG_WITH(pbs-config-dir, 
AC_HELP_STRING([--with-pbs-config-dir=<dir>],[look for pbs-config script 
in <dir>]))
  AC_ARG_WITH(net-snmp-config-dir, 
AC_HELP_STRING([--with-net-snmp-config-dir=<dir>],[look for 
net-snmp-config script in <dir>]))
-AC_ARG_WITH(dpm-dir, AC_HELP_STRING([--with-dpm-dir=<dir>],[look for DPM 
libraries and header files under <dir>]), [], [with_dpm_dir=/opt/dpm])
+AC_ARG_WITH(dpm-dir, AC_HELP_STRING([--with-dpm-dir=<dir>],[look for DPM 
libraries and header files under <dir>]), [], [with_dpm_dir=/opt/lcg])


-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

Re: [MonAMI-devel] Batch of patches

[Stephen C. <ch...@cs...>]

Paul Millar wrote:
  Stephen,
> 
> Just to let you know, apart from the patches I replied to with some comments, 
> I've added the patches you've sent.  On of the patches appeared to be a 
> duplicate of the previous one ("Simply increases the default size of pie graph 
> from 250 to 450"), so I committed one and ignored the second one.
> 
> I've also added the "-ldpm" that was missing from definition of @dpm_libs@ 
> (without it, the linker doesn't link against libdpm.so but assumes 
> dpm_getpools() will be resolved at run-time, so would fail).

That's great. I am just fixing up the patches with issues and will send on 
new versions.

> I've also added support in autoconf for building MonAMI if the DPM libraries / 
> include files are somewhere other than /opt/lcg and added the libdpm.so 
> dependency to the DPM plugin RPM.

Could you set the default include to the standard location 
(/opt/lcg/include/dpm) please? That's where the dpm rpms put the include 
files. As you can see the default value you put in doesn't work:

gcc -g -O2 -fPIC  -I. -I/home/childss/MonAMI/libsrc/built/include 
-I/home/childss/MonAMI/src/../src -I. -I/usr/include/mysql -g -pipe -m64 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-fno-strict-aliasing -I/opt/dpm/include/dpm  -c -o getvalues.o getvalues.c
getvalues.c:42:21: dpm_api.h: No such file or directory



Stephen


-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs

[MonAMI-devel] Fwd: Re: [PATCH] make dpm fs names canonical by prepending server name

[Paul M. <p.m...@ph...>]

[sent again, but this time from the correct email address]

---

Hi Stephen,

A couple of issues...

On Tuesday 26 May 2009 13:27:26 Stephen Childs wrote:
-    
-    /* add leaf node to appropriate branch */
-    datatree_add_node_with_path( filesystems_root_branch, row[0], 
result_fs_used, 0);
+    /* just get the short hostname */
+    start = row[3];
+    dot = strchr( start, '.');
+    if (dot) {
+      len = dot-start;
+      memcpy(shorthost, row[3], len);
+      shorthost[len]='\0';

I believe you're not checked that len < sizeof( shorthost), which may lead to 
a buffer overrun.

Unless we need the FQDN later, I think this would be simpler (and safer) to 
leave the string in row[3] and simply truncate it at the first dot;  something 
like:
	if( (dot = strchr( row[3], '.')))
	  *dot = '\0';


+      message( log_info, "shorthost %s len %d\n", shorthost, len);

This is really debugging the dpm plugin, so I'd have gone for log_debug 
instead.


+      /* add leaf node to appropriate branch */
+      len = sizeof(row[0]) + sizeof(shorthost) + 1;

I think you mean strlen() rather than sizeof().


+      if ( ( len < sizeof(canon_fs)) ) {
+       snprintf(canon_fs, len, "%s.%s", shorthost, row[0]);

Hmmm, I think sizeof(canon_fs) would be safer snprintf() limit against a 
miscalculated len:

	 snprintf( canon_fs, sizeof(canon_fs), [...]

HTH,

Paul.

[MonAMI-devel] Batch of patches

[Paul M. <p.m...@ph...>]

Hi Stephen,

Just to let you know, apart from the patches I replied to with some comments, 
I've added the patches you've sent.  On of the patches appeared to be a 
duplicate of the previous one ("Simply increases the default size of pie graph 
from 250 to 450"), so I committed one and ignored the second one.

I've also added the "-ldpm" that was missing from definition of @dpm_libs@ 
(without it, the linker doesn't link against libdpm.so but assumes 
dpm_getpools() will be resolved at run-time, so would fail).

I've also added support in autoconf for building MonAMI if the DPM libraries / 
include files are somewhere other than /opt/lcg and added the libdpm.so 
dependency to the DPM plugin RPM.

Cheers,

Paul.

Re: [MonAMI-devel] [PATCH] add support for dpm querying pool information

[Paul M. <p.m...@ph...>]

Hi Stephen,

On Tuesday 26 May 2009 13:17:39 Stephen Childs wrote:
> Uses the DPM library to query the "free" and "used" values for DPM pools
> and adds them to the datatree.

Some comments:

+#include <dpm_api.h>

The first is the most trivial one:  for purely cosmetic reasons, I'd put 
#include <dpm_api.h> nearer where <mysql/mysql.h> is included.  I try to 
include headers in order: first system headers, then system-wide installed 
headers, then headers from MonAMI-core, then plugin-local headers.  There's no 
particular strong requirement for this, though, just my personal style ;-)


+void dpm_get_poolspace(dpm_handle_t *h, void *root)

You should also add this new function to the getvalues.h header file.


+  /* get pool size from DPM */
+  if ( (dpm_getpools (&npools, &dpm_pools)) ==0) {

Although MonAMI-core redirects stderr and stdout to /dev/null, I think it 
would be cleaner to redirect the output so it doesn't attempt to write to 
stdout or stderr (using dpm_seterrbuf() function).

Also, it would be good to adjust the timeout options so dpm_getpools() will 
always return ... eventually.  The default timeout (1 week) is perhaps too 
long:
	https://twiki.cern.ch/twiki/bin/view/LCG/DpmApi


+    message( log_info,"DPM reports %d pools\n", npools);

IMHO, log_info is too high for this; logging at log_debug would be better.


+      sprintf(res_str,"%ld", (dpm_pools[i].free)/1000000);
+      message( log_info,"Pool %s free %s\n", dpm_pools[i].poolname, res_str);
+      result_pool_free = datatree_new_result_from_string( "free",
+							   UINT32_STR,
+							   DATATREE_ATTR_NORMAL,
+							   "MB",
+							   res_str);

It should be possible to use one of the macros to generate the metric without 
building a string and then parsing it.  For example:

 result_pool_free = DATATREE_NEW_UINT32( "free", (dpm_pools[i].free)/1000000),
		DATATREE_ATTR_NORMAL, "MB");

+  } else {
+    message( log_error,"Could not get information from DPM\n");
+  }

It would be nice to be a bit more forthcoming with the error message.  The 
value of serrno may give a hint to the end-user where the problem lies:

http://grid-deployment.web.cern.ch/grid-
deployment/documentation/LFC_DPM/dpm/html/dpm_getpools.html

Cheers,

Paul.

Re: [MonAMI-devel] [PATCH] add support for displaying pools in frontend

[Paul M. <p.m...@ph...>]

Hi Stephen,

On Tuesday 26 May 2009 13:05:49 Stephen Childs wrote:
> Modifications to dpm-graph.php mg-frame-dpm.php to display a table
> summarising pool usage.

I've not committed this patch as there's a couple of loose ends.

It looks like the new $format variables are meant as configuration.  If so, 
here are some comments (my 2c-worth ;-)

	the variable declaration should be immediately preceeded by a
	comment describing what they do and what are the valid values.

	if not awkward, it's better for the variables to appear near the top
	of the file (c.f. multiple-graphs.php).  For example, they could
	be class fields.

	the documentation (such as it exists) in ganglia/README.txt should
	mention the options in mg-frame-dpm.php.

	Also, would it make sense to add support "GB" as well?  (Just a thought.)
 
+      $this->draw_pie( $this->colour,
+               "pools\.${name}", "", /*free(.+)",*/
+               "${name}", "Pie chart of pool ${name} by free/used");

Is the comment meant to be here?

Cheers,

Paul.

[MonAMI-devel] [PATCH] make dpm fs names canonical by prepending server name

[Stephen C. <ch...@cs...>]

-- 
Dr. Stephen Childs,
Research Fellow, EGEE Project,    phone:                    +353-1-8961797
Computer Architecture Group,      email:        Stephen.Childs @ cs.tcd.ie
Trinity College Dublin, Ireland   web: http://www.cs.tcd.ie/Stephen.Childs