Hi,
> I don't know too much about vCenter, I'll assume that the interactions
> with it are "correct", whatever that means.
As Dejan stated after, there is still something that has to be fixed: if
a VM is off, the reset won't work. I'll fix this.
As you probably noticed :), the code has not been revised, this is
mainly why there are so unclean and redundant sections. BTW I do not
want to hide my perl ignorance! Many tricky statement (e.g. the use of defined)
are mainly a result of a debugging phase not well cleaned up.
> > use Switch;
>
> I don't like that Switch.pm thingy ;-)
> But never mind...
Yes, it sounded a little bit tricky to me, but... you know... I thought it was
perl-addicted stuff... It seems that I was wrong :)
> > 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];
Absolutely yes.
> > 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?
VI_PORTNUMBER and VI_PROTOCOL are optional parameters but they're
needed when "ip" is set.
It doesn't make any difference.
> 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 understand that there are better ways to do it.
validate() triggers interactive authentication if VI_USERNAME/VI_PASSWORD
are not set and this should be avoided (the script should be also callable
without parameters)
I should add remarks on that.
> >
> > 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 = ();
> ...
Yes, that mapping hashes would be better
> > 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.
>
Yes, this includes also regex escapes, it should be better. However I'm
not sure about case insensitive matching, I would prefer leaving it so.
The missing trailing $ is something that was left behind during the
evolution of the code (at the beginning my VMs were named with the same
prefix and with a number suffix).
> > Util::disconnect();
> > return $vms;
> > }
> >
> > switch ($command) {
> > case "reset" {
>
> Bah. Did I mention that I dislike switch statements in Perl?
> Ah, never you mind ;)
C'mon I come from C...
> > 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 }
I agree with you, except that I don't have anything about that from the (poor)
official VMware perl documentation (see regular expression vs plain queries).
I'll test it.
> > 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?
This is something that disagrees with the VMware documentation. The
suggested method is:
$vm->ResetVM(host => $hostname);
Sadly, it doesn't work. However if I call Vim::get_view with the
HostSystem manageable VM object before ResetVM, it works, but I have to strip
off
"host" from the parameters. In vCenter, reset/off/on can be issued only if the
HostSystem is explicited. Here's how.
> > Util::disconnect();
>
> Do the Util::connect(), disconnect() once, not within each case.
Yes, I agree with you. Since there are commands that are executed without
environment
variables (e.g. getconfignames), we cannot issue validate, connect and
disconnect. It
is better to write two code sections (with and without connection).
> 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();
I think that this should be done in any case.
> > $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)
Yes, I agree. I came up to use this because I've seen something similar in
external/vmware.
> > 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...)
I want to continue feeling the pain of backslashes :)
> 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).
I did.
> 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.
I used it with clone, but may be I didn't catch the meaning of unique.
> <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]
Yes, no whitespaces.
> </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?
VMware perl library allows to use a .vimrc file in the user home, I wanted to
use that
one but finally I preferred explicit configuration, even if less secure (of
course,
I've also assessed the security of the cluster network), because the corosync
init
script that launches pacemaker is run as root but doesn't have $HOME set.
Thank you very much for the revision.
Nhan
_______________________________________________________
Linux-HA-Dev: [email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
