Hi, On Mon, Apr 04, 2011 at 09:23:17PM +0200, Lars Ellenberg wrote: > On Sun, Apr 03, 2011 at 10:15:50AM +0200, Nhan Ngo Dinh wrote: > > Hello all, > > > > Please find the attached plugin. > > > > It requires vSphere SDK for Perl: > > http://www.vmware.com/support/developer/viperltoolkit/ > > > > Parameters: > > > > hostlist (e.g. hostname1=VMNAME;hostname2=VMNAME2), ip (vCenter IP), > > username, password > > > > Tested with 4.1 > > I don't know too much about vCenter, I'll assume that the interactions > with it are "correct", whatever that means. > > But I know my share of perl and pacemaker. > So here are a few comments. > > > > #!/usr/bin/perl > > # > > # External STONITH module for VMWare vCenter > > # > > # Author: Nhan Ngo Dinh > > # License: GNU General Public License (GPL) > > # > > > > use strict; > > use warnings; > > use Switch; > > I don't like that Switch.pm thingy ;-) > But never mind... > > > use VMware::VIRuntime; > > > > my $command = $ARGV[0]; > > my $targetHost; > > if ( defined $ARGV[1] ) { > > $targetHost = $ARGV[1]; > > } > > This end result of this is still the same as just writing > my $targetHost = $ARGV[1]; > > > > if (defined($ENV{'ip'})) { > > If you mean to test for existence of hash elements, please always use > "exists". The above will create an empty but from now on exported 'ip' > variable into to environment. > You likely want to say > > if (exists $ENV{ip}) { > > (and yes, you can give it more line noise, > if you feel more comfortable that way) > > If you actually want to say "if defined", > then you could also just do away with the if, > and assign directly. > > > $ENV{'VI_PORTNUMBER'} = 443; > > $ENV{'VI_PROTOCOL'} = "https"; > > $ENV{'VI_SERVER'} = $ENV{'ip'}; > > > Why would you only override VI_PORTNUMBER and VI_PROTOCOL > if the ip is defined? > > Why would you want to have your own "ip" environment, > and not expose VI_SERVER directly? > > > > } > > > > if (defined($ENV{'username'})) { > > $ENV{'VI_USERNAME'} = $ENV{'username'}; > > $ENV{'VI_PASSWORD'} = $ENV{'password'}; > > Opts::parse(); > > Opts::validate(); > > } else { > > Opts::parse(); > > No validate here? Why not? > > > } > > I suppose the Opts::validate() is supposed to throw an error (aka die()), > if there is insufficient information to contact the management entity? > If so, please document that. > > Probably you want to catch that via eval {}, > and log the error message with some context information > about where it comes from, before propagating the error. > > > > > my $ret = 255; > > my $hosts = {}; > > my $realTarget; > > > > if ( defined $ENV{'hostlist'} ) { > > my @lines; > > my $line; > > @lines = split(/;/, $ENV{'hostlist'}); > > foreach $line (@lines) { > > my @config = split(/=/, $line); > > $hosts->{$config[0]} = $config[1]; > > } > > if (defined $targetHost) { $realTarget = $hosts->{$targetHost}; } > > } > > Please add a comment what this is supposed to do, and why. > Not what it does, that's clear ;-) > > Also: clearly document that no whitespace is allowed, nowhere, > and hostname=VMNAME; is a hard requirement, even if hostname and VMNAME > should match. > Or change the code to allow that special case to be written as just > "hostname;" in the hostlist. > > Since you later do the reverse mapping in nested loops, > how about constructing both forward and backward mapping hashes here? > %host_to_vm = (); > %vm_to_host = (); > ... > > > sub getvms { > > Util::connect(); > > my $regex = ""; > > for (keys %$hosts) { > > if (length($regex) > 0) { $regex .= "|"; } > > $regex .= $hosts->{$_}; > > } > > my $vms = Vim::find_entity_views(view_type => "VirtualMachine", > > filter => { 'name' => qr/^($regex)/ }); > > I'm not sure if you really mean what you wrote there. > > My guess would be that you actually meant to say > my $regex = join "|" map { qr/\Q$_\E/ } values %$hosts; > my $vms = Vim::find_entity_views(view_type => "VirtualMachine", filter => { > 'name' => qr/^($regex)$/i }); > > What's the difference? > So it is written differently. > So what. You can also write that as a for (keys ...) {} no problem. > > Then, it does a case insensitive match (I'm not sure, but I assume > VMNAME is supposed to be matched case insensitive?) > > It does not omit the trailing $, > so VMNAM would not match both VMNAM and VMNAME ;-) > > Most importantly, it \Q-ot-\E-s the VMNAMEs, so VM.NA.ME won't match > VMxNAyME, but only VM.NA.ME. > > > Util::disconnect(); > > return $vms; > > } > > > > switch ($command) { > > case "reset" { > > Bah. Did I mention that I dislike switch statements in Perl? > Ah, never you mind ;) > > > Util::connect(); > > my $vm = Vim::find_entity_view(view_type => "VirtualMachine", > > filter => { name => $realTarget }); > > Unless this filter thing has a special mode where it internally does a > "$x eq $y" for scalars and "$x =~ $y" for explicitly designated qr// > Regexp objects, I'd suggest to here also do > filter => { name => qr/^\Q$realTarget\E$/i } > > > my $hostname = Vim::get_view(mo_ref => > > $vm->{"runtime"}->{"host"})->name; > > What is the hostname used for, now? > > > $vm->ResetVM(); > > Hm. Apparently the filter thingy did something different, > the return value seems to be a reference to some "manageable" VM object, > previously it was supposed to be a reference to an array of such objects? > > > Util::disconnect(); > > Do the Util::connect(), disconnect() once, not within each case.
There's otherwise a lot of code repetition for the on, off, and reset cases. Did you check that doing a reset on a VM which is off does the right thing, i.e. turns the VM on? Also, if doing off on a VM which is off doesn't exit with error. > I suggest doing > Util::connect(); > eval { > ... > } > if ($@) { > my $error = $@; > > log the error in a suitable way > > .... > $ret = 1; # or 255, or 42 or whatever feels "right". > } else { > $ret = 0; > } > Util::disconnect(); > > > > $ret = 0; > > } > > > > case "off" { > > Util::connect(); > > my $vm = Vim::find_entity_view(view_type => "VirtualMachine", > > filter => { name => $realTarget }); > > my $hostname = Vim::get_view(mo_ref => > > $vm->{"runtime"}->{"host"})->name; > > Again, useless (?) use of $hostname. > > > $vm->PowerOffVM(); > > Util::disconnect(); > > $ret = 0; > > } > > > > case "on" { > > Util::connect(); > > my $vm = Vim::find_entity_view(view_type => "VirtualMachine", > > filter => { name => $realTarget }); > > my $hostname = Vim::get_view(mo_ref => > > $vm->{"runtime"}->{"host"})->name; > > $vm->PowerOnVM(); > > Util::disconnect(); > > $ret = 0; > > } > > > > case "gethosts" { > > my $vms = getvms(); > > foreach my $vm (@$vms) { > > while (my ($k, $v) = each %$hosts) { > > if ($vm->name eq $v) { > > print $k . "\n"; > > } > > } > > } > > I'd always write "$k\n" instead of $k . "\n"; > I'd use the previously created vm_to_host hash. > > So that would become > foreach my $vm (@$vms) { print "$vm_to_host{$vm->name}\n" }; > > > > $ret = 0; > > } > > > > case "status" { > > $ret = system("ping -c1 $ENV{'ip'}"); > > #Util::connect(); > > #Util::disconnect(); > > #$ret = 0; > > Hm. > You I'd like some sort of "no-op" that actually excercises the > connection to the "VM manager" level, instead of pinging some ip, > which may even be misspelled accidentally (10.0.0.23 instead of > 10.0.0.32 ;-) > > So rather actually do an Util::connect(), so something with the Vim:: > blahfoo (e.g. chose a random hostname from the hostlist, and query it's > vms "power" status) A good idea. > > } > > > > case "getconfignames" { > > print "hostlist\nip\nusername\npassword\n"; > > $ret = 0; > > } > > > > case "getinfo-devid" { > > print "VMware vCenter STONITH device\n"; > > $ret = 0; > > } > > > > case "getinfo-devname" { > > print "VMware vCenter STONITH device\n"; > > $ret = 0; > > } > > > > case "getinfo-devdescr" { > > print "VMWare vCenter STONITH device\n"; > > $ret = 0; > > } > > > > case "getinfo-devurl" { > > print "http://www.vmware.com/\n"; > > $ret = 0; > > } > > > > case "getinfo-xml" { > > print "<parameters> > > <parameter name=\"hostlist\" unique=\"1\"> > > Did you know: > print q{blafoo <lala/> x="unquoted double quotes" possible here.}; > (mind the difference between q{} and qq{}; > also, there are "HERE" documents in perl as well...) > > If there are two ways to express something, > I tend to use the one with less line noise, > so I'd try to avoid all those \"\" > > (btw, I did not validate the xml). > > print q{<parameters> > <parameter name="hostlist" unique="1"> > > Is it unique? Why? > I think it would be legit to be able to stonith the same hosts > from more than one instance of stonith plugins. > > <content type="string"/> > <shortdesc lang="en">hostlist</shortdesc> > <longdesc lang="en"> > The list of hosts that the VMware vCenter STONITH device controls. > Format is "hostname1=VirtualMachineName1;hostname2=VirtualMachineName2" > > No whitespace allowed. > hostname is supposed to be "uname -n", which is what is passed > in as STONITH victim on the command line. > > [if I understood correctly] > > </longdesc> > </parameter> > <parameter name="ip" unique="1"> > > Again, why would that be unique? > Same for parameters below. > > <content type="string"/> > <shortdesc lang="en">ip</shortdesc> > <longdesc lang="en"> > The VMware vCenter IP address > </longdesc> > </parameter> > <parameter name="username" unique="1"> > <content type="string"/> > <shortdesc lang="en">username</shortdesc> > <longdesc lang="en"> > The username to access VMware vCenter > </longdesc> > </parameter> > <parameter name="password" unique="1"> > <content type="string"/> > <shortdesc lang="en">password</shortdesc> > <longdesc lang="en"> > The password to access VMware vCenter > > You probably want to mention that it would be a good idea to > secure all pacemaker communication paths against the potential > of sniffing, and lock down the permissions on any location that > could end up containing copies of the cib in either xml or plain > text, or risk funny DoS attacks ;-) > Then again, may that is obvious. > > That's a problem for all STONITH devices, though, unless they > somehow stored their credentials somewhere else. > > I think there is some bugzilla on that issue (protecting > sensitive information in the cib from appearing in logs, > or rather keep them out of the cib completely). > > Dejan? Yes, there is going to be a way to move parts of CIB to local files. > </longdesc> > </parameter> > </parameters> > }; > > > $ret = 0; > > } > > > > else { > > $ret = 1; > > } > > > > } > > > > exit($ret); > > Enough for today. > > Thank you for this contribution. > > If we now can have a few other "Reviewed-by:" or "Tested-by:" signatures, > that would be great. The latter in particular :) Cheers, Dejan > > -- > : Lars Ellenberg > : LINBIT | Your Way to High Availability > : DRBD/HA support and consulting http://www.linbit.com > _______________________________________________________ > Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > Home Page: http://linux-ha.org/ _______________________________________________________ Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/