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.
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)
> }
>
> 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?
</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.
--
: 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/
