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/
