Hi there AurÃlien,
----- Original Message -----
> Hi,
>
> I've made a simple PMDA for CIFS, the kernel module that lets you
> mount remote SMB share on your local file system.
Awesome. Thanks for sharing it!
> The PMDA parses a virtual file in /proc/fs/cifs/Stats which is created
> when the cifs module is loaded and a share is mounted.
>
> There are system-wide metrics which only have 1 instance, there are
> per-mount metric (1 instance per mount) which also have their
> respective "total" (sum) metric (1 instance).
>
> I have attached a example of the Stats file and `pminfo -df cifs` on a
> system with 2 shares after few operations.
>
> This is my first PMDA, I've used the PMDA Perl module. I've only made
> the Install/Remove and pmdacifs scripts: it's really just 3 files. I'm
> still working on it/discussing with other Samba devs but I was
> thinking it could ultimately be merged back to the original repo.
Its a solid first effort. I agree it's a generally useful PMDA and
thus certainly a candidate for merging into PCP.
> I would like some feedback on what I already have and what I should do
> next. If I understand this correctly I also need a PMID for my PMDA,
> so how does that work?
What we need to do is allocate a "domain number" for your PMDA. The
list of allocated numbers lives in src/pmns/stdpmid.pcp - I've marked
121 as reserved for CIFS now. Some more details about this process &
the terminology I'm using is in the recently-revised Programmers Guide.
http://oss.sgi.com/projects/pcp/doc/pcp-programmers-guide.pdf
The content in chapters 1 & 2 would be worth reading for your case I
think. Its quite C-centric, currently, but the Perl API is built
fairly closely on the C API; you'll be right at home since you have
clearly already worked out alot of the concepts yourself.
So, next step after that - I've reviewed the new code (below), if you
can incorporate those suggestions, or we can have a discussion about
'em here via email, or on #pcp on freenode.net. Then get some tests
in place (see below), update the build/makefiles, and we'll be ready
to merge it all in at that point.
One big thing I noticed during review - looking at the kernel code
(fs/cifs/cifs_debug.c::cifs_stats_proc_show) that backs these metrics
there appears to be very different Stats file formats depending on
whether the server is using smb1 or smb2 operation vectors (refer to
server->ops->print_stats near end of cifs_stats_proc_show). That'll
end up in either smb2_print_stats or smb1_print_stats, and the PMDA
needs to handle both formats it seems (evidently its possible to have
both active at once, for different mount points). If I'm reading the
new PMDA code correctly (and the sample procfs file you forwarded),
I think its handling just the v1 case and not v2?
Also, some special handling may be needed for a "DISCONNECTED" state?
Anyway ... in general, well worth reading through that kernel code.
There's another review comment later about metric types - that, too,
will need close inspection of the kernel code, since they are not all
the same sizes, nor do they all appear to be of the same signed-ness;
eg the operation counts are signed-32-bit counters, the bytes counts
are unsigned-64bit counters.
> I also have to add some documentation. Is documenting each metric in
> the add_metric() call enough?
*nod* - good idea. I'd also recommend using pod to produce a man page
like the other perl PMDAs do - see src/pmdas/samba/pmdasamba.pl; from
around line 127 onward - for an example.
It'd also be helpful to have one or two automated tests that exercise
the new code. I think the gluster PMDA test would be a good template
for your case - see qa/553 in the PCP source tree.
> My git repo [1] and browseable version [2]. My work is on the pmda-cifs
> branch.
In general its a really good start! Inline comments follow ...
diff --git a/src/pmdas/cifs/Install b/src/pmdas/cifs/Install
...
+STATFILE=/proc/fs/cifs/Stats
+
+if ! [ -f $STATFILE ]; then
+ echo "$STATFILE doesn't exist. Are you sure the cifs module is loaded?"
+ exit 1
+elif ! grep -q '1) ' $STATFILE; then
+ echo "No mount statistics found in $STATFILE. You need to mount a share."
+ exit 1
+fi
My understanding from scanning thru the kernel code suggests these stats are
compile-time conditional on CONFIG_CIFS_STATS. So, you could further refine
your diagnostics here to help the punter installing the code:
- if `dirname $STATFILE` does not exist, the kernel module isn't loaded
- else if $STATFILE does not exist, kernel support not compiled in
- else, your second case above. I would turn that one into just a warning
rather than a fatal error, as mounts may come and go during the (relatively
long) life cycle of pmdacifs.
diff --git a/src/pmdas/cifs/pmdacifs.pl b/src/pmdas/cifs/pmdacifs.pl
...
+use strict;
+use warnings;
+use PCP::PMDA;
+use Data::Dumper;
This could be commented out (the above, final "use" statement) since
the diagnostic that uses it is also commented out. Possibly a tiny
little bit less memory used by pmdacifs, etc.
+my $dom = 'cifs';
I'd suggest just using this "inline" and not having a global variable.
Up to you though, if you like it as-is thats fine. In general, I try
to minimise globals just for simplicity, but that just personal taste.
+my $pmid = 200; # TODO: ask for pmid upstream in pcp mailing list
121 is all yours ;)
+my $procfile = '/proc/fs/cifs/Stats';
If you check out test qa/553, it uses a trick here - in your case, if
you allow $procfile to be optionally changed via an environment var,
you can plug in lots of different test cases quickly & easily.
+my %stat;
+my $pmda;
+my $mount_items_indom;
+my @root_items;
+my @mount_items;
+my @mount_paths;
+my @total_items;
+my $nb_mount;
+my $nb_item;
+my @indom_desc;
Ah - here's a couple of tips that might help you with managing all
the state you have to keep:
- if metric nodes encode some of the search pattern, you might find
the pmda_pmid_name() interface useful - src/pmdas/kvm/pmdakvm.pl
has an example.
- if you have a possibly complex instance domain (mounts, here) and
need to map from extenal instance name to another data structure
holding eg values, then a hash-based indom can be a better option
than array-based. %timeslices in src/pmdas/simple/pmdasimple.pl
is an example.
+main();
+
+sub main {
+ parse_stat_file();
+
+ @root_items = qw/numsession numoperation/;
+ @mount_items = keys %{$stat{permount}[0]{data}};
+ @mount_paths = map {$_->{path}} @{$stat{permount}};
+ @total_items = map {"total.$_"} @mount_items;
+ $nb_mount = @{$stat{permount}};
+ $nb_item = @mount_items;
+ my $byte_unit = pmda_units(1,0,0, PM_SPACE_BYTE,0,0);
+ my $count_unit = pmda_units(0,0,0,0,0,0);
+ my @item = (0, 0, 0);
+
+ $pmda = PCP::PMDA->new($dom, $pmid);
+ $pmda->log("added pmda id $pmid");
The pmda library code will automatically log on startup, so
I'd recommend not doing this initial log().
+
+ # cluster 0 for global OS stats
+ # cluster 1 for permount stat
+ # cluster 2 for sum of mount stat (total)
Good approach.
+ $pmda->add_metric($pmid, PM_TYPE_U32, PM_INDOM_NULL,
+ PM_SEM_COUNTER, $unit, $path, '', '');
As per earlier comment, I think most of the counters are signed
32 bit in the kernel (atomics).
+ $pmda->log("add_metric ($cluster, $item[$cluster]) $pmid, $path");
(debug, comment out by default)
+ for (@mount_items) {
+ my $path = "$dom.$_";
+ my $unit = /bytes/i ? $byte_unit : $count_unit;
+ my $cluster = 2;
+ my $pmid = pmda_pmid($cluster, $item[$cluster]);
+ my $indom = $item[$cluster];
The indom ID should be the same for all mount points, and not the
cluster - the pminfo output suggests this is not quite right (will
result in lots of unnecessary extra traffic between client tools
and pmcd).
+ $pmda->add_metric($pmid, PM_TYPE_U32, $indom,
+ PM_SEM_COUNTER, $unit, $path, '', '');
U32 again. Also the bytes metrics are U64 in the kernel, that
needs to be reflected here.
+ $pmda->log("add_metric ($cluster, $item[$cluster]) $pmid, $path INDOM
$indom");
+ $item[$cluster]++;
+ }
+
+ # build description of each instance (mount)
+ # use share name
+ for my $i (0 .. $nb_mount-1) {
+ $indom_desc[$i*2] = $i;
+ $indom_desc[$i*2+1] = $mount_paths[$i];
+ }
As above, might be easier to use a hash-indom - personally, not a
big fan of array-based (they're a bit old-school now & hard to use).
+ # $pmda->log("built desc ". Dumper(\@indom_desc));
(only need Data::Dumper for this commented out diagnostic, as mentioned).
+ # create indom for each per mount stat and pass a copy of the
+ # description array to describe each instance
+ for my $indom (0 .. $nb_item-1) {
+ $pmda->add_indom($indom, [@indom_desc], '', '');
+ # $pmda->log("add_indom $indom");
+ }
Should be just one indom, shared by all per-mount metrics.
You'll also need a replace_indom call as part of the stats
file refresh - mount points can come and go, and changes in
the instance domain need to be reflected here dynamically.
See that pmdasimple.perl example for that too.
On reflection - consider having a fixed set of add_metrics()
calls for this reason too. If there are no mounts when the
PMDA starts up, there would be lots of metrics not added to
the metrictable.
+sub cifs_fetch_callback {
+ my ($cluster, $item, $inst) = @_;
+
+ # $pmda->log("fetch_cb: $cluster, $item, $inst");
+
+ if ($cluster == 0) {
+ if ($inst != PM_IN_NULL) {
+ return (PM_ERR_INST, 0);
+ }
+ return ($stat{$root_items[$item]}, 1);
Need to do error handling here - $item could be outside of
the range of @root_items and that will cause the PMDA to
issue a perl stack trace and exit.
+ }
+ elsif ($cluster == 1) {
+ if ($inst != PM_IN_NULL) {
+ return (PM_ERR_INST, 0);
+ }
+ return ($stat{total}{$mount_items[$item]}, 1);
Ditto.
+ }
+ elsif ($cluster == 2) {
+ if ($inst == PM_IN_NULL) {
+ return (PM_ERR_INST, 0);
+ }
+ return ($stat{permount}[$inst]{data}{$mount_items[$item]}, 1);
Ditto. Also need to check $inst (differently if you switch to using
hash-based instances).
+ }
+
+ $pmda->log("fetch_cb: not supposed to be here...");
But it can happen, if a client tool specifically crafts a pmFetch(3) to
request a metric from your PMDA with such a PMID. Just need to guard
against it ...
+ return (PM_ERR_PMID, 0);
+}
... that's the right action to take (no diagnostic needed).
Let me know if any of those comments are too cryptic. I (and many
other PCP developers) are on IRC if you are after a quick response
or have shorter questions, or just to say hello. :) Email works
just fine too.
cheers.
--
Nathan
|