[PATCH v1] OSSTest test-harness for livepatches.

[PATCH v1] OSSTest test-harness for livepatches.

[Konrad Rzeszutek Wilk]

Heya!

With this test-harness I am able to run the regressions tests on livepatches
on Xen 4.8. I only used the standalone tests. The incantations was:

To prep:
./sg-run-job build-amd64
./sg-run-job build-amd64-pvops

And then to run:
./sg-run-job test-amd64-amd64-livepatch

There are still some TODOs such as:
 - Test this on ARM (it should work out of the box),
 - Not sure how to hook this up to the real OSSTest?
 - It is missing checks to only run the testcases on Xen 4.8 or later.

Anyhow, please see the patches and provide feedback.

Thanks!

 Osstest/TestSupport.pm |  28 ++++++++------
 make-flight            |  12 ++++++
 mfi-common             |   9 +++++
 sg-run-job             |   5 +++
 ts-livepatch           | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 ts-xen-build           |  14 +++++++
 6 files changed, 159 insertions(+), 11 deletions(-)

Konrad Rzeszutek Wilk (7):
      ts-xen-build: Enable livepatch.
      ts-xen-build: Build the livepatch test-cases
      ts-xen-build: Install livepatch regressions tests.
      OssTest: Add target_cmd_root_rc which returns return code.
      ts-livepatch: Initial test-cases.
      sg-run-job: Add the test-livepatch.
      make-flight/mfi-common: Add them in the matrix.


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

[PATCH v1 1/7] ts-xen-build: Enable livepatch.

[Konrad Rzeszutek Wilk]

Livepatch compiles and works on x86/ARM{32|64} so we can
unconditionaly enable it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 ts-xen-build | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ts-xen-build b/ts-xen-build
index 4f1f71a..c567054 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -118,6 +118,8 @@ END
 		echo >>xen/.config CONFIG_EXPERT=y
 		echo >>xen/.config CONFIG_HVM_FEP=y
 		echo >>xen/.config CONFIG_VERBOSE_DEBUG=y
+		echo >>xen/.config CONFIG_LIVEPATCH=y
+		echo >>xen/.config CONFIG_FAST_SYMBOL_LOOKUP=y
 	fi
 END
                );
-- 
2.1.4


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

[PATCH v1 2/7] ts-xen-build: Build the livepatch test-cases

[Konrad Rzeszutek Wilk]

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 ts-xen-build | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ts-xen-build b/ts-xen-build
index c567054..9843c2e 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -165,6 +165,11 @@ END
 END
 	store_runvar("flaskpolicy", "xenpolicy-" . $xen_version);
     }
+    buildcmd_stamped_logged(600, 'xen', 'tests', '',<<END,'') if $dokconfig;
+            if test -d xen/test; then
+                $make_prefix make -C xen tests
+            fi
+END
 }
 
 sub divide () {
-- 
2.1.4


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

[PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests.

[Konrad Rzeszutek Wilk]

That come with the Xen git tree (see xen/test/).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 ts-xen-build | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ts-xen-build b/ts-xen-build
index 9843c2e..1b36b9c 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -170,6 +170,13 @@ END
                 $make_prefix make -C xen tests
             fi
 END
+    buildcmd_stamped_logged(600, 'xen', 'tests-install', '',<<END,'') if $dokconfig;
+        if test -d xen/test; then
+           mkdir -p dist/install/usr/lib/debug
+           livepatch_files=`find xen/test/livepatch -name '*.livepatch' -print`
+           cp \$livepatch_files dist/install/usr/lib/debug
+        fi
+END
 }
 
 sub divide () {
-- 
2.1.4


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

[PATCH v1 4/7] OssTest: Add target_cmd_root_rc which returns return code.

[Konrad Rzeszutek Wilk]

All the different target_cmd_* end up calling tcmdex
which has the unfortunate side-effect of calling 'die' if
the SSH sessions results in any return code not zero.

That is fine, except for tests where we want to get a non-zero
return value.

This patch moves the: die "status $r" from tcmdex
to all the other functions which use tcmdex.

Also we make tcmd behave in the normal old way (die "status $r")
or if returnrc is set to one, we will return the error code.

The only exposed function that does that is target_cmd_root_rc.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 Osstest/TestSupport.pm | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 888f0ac..92f043a 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -51,6 +51,7 @@ BEGIN {
                       get_runvar_default need_runvars
                       unique_incrementing_runvar next_unique_name
 
+                      target_cmd_root_rc
                       target_cmd_root target_cmd target_cmd_build
                       target_cmd_output_root target_cmd_output
                       target_cmd_inputfh_root sshuho
@@ -424,17 +425,17 @@ sub tcmdex {
     # We use timeout(1) as a backstop, in case $cmd doesn't die.  We
     # need $cmd to die because we won't release the resources we own
     # until all of our children are dead.
-    my $r= cmd($timeout,$stdin,$stdout,
+    cmd($timeout,$stdin,$stdout,
 	       'timeout',$timeout+30, $cmd,@$optsref,@args);
-    $r and die "status $r";
 }
 
 sub tgetfileex {
     my ($ruser, $ho,$timeout, $rsrc,$ldst) = @_;
     unlink $ldst or $!==&ENOENT or die "$ldst $!";
-    tcmdex($timeout,undef,undef,
+    my $r = tcmdex($timeout,undef,undef,
            'scp', sshopts(),
            sshuho($ruser,$ho).":$rsrc", $ldst);
+    $r and die "status $r";
 } 
 sub target_getfile ($$$$) {
     my ($ho,$timeout, $rsrc,$ldst) = @_;
@@ -448,16 +449,18 @@ sub target_getfile_root ($$$$) {
 sub tputfileex {
     my ($ruser, $ho,$timeout, $lsrc,$rdst, $rsync) = @_;
     my @args= ($lsrc, sshuho($ruser,$ho).":$rdst");
+    my $r = 0;
     if (!defined $rsync) {
-        tcmdex($timeout,undef,undef,
+        $r = tcmdex($timeout,undef,undef,
                'scp', sshopts(),
                @args);
     } else {
         unshift @args, $rsync if length $rsync;
-        tcmdex($timeout,undef,undef,
+        $r = tcmdex($timeout,undef,undef,
                'rsync', [ '-e', 'ssh '.join(' ',@{ sshopts() }) ],
                @args);
     }
+    $r and die "status $r";
 }
 sub target_putfile ($$$$;$) {
     # $ho,$timeout,$lsrc,$rdst,[$rsync_opt]
@@ -652,19 +655,22 @@ sub target_await_down ($$) {
 }    
 
 sub tcmd { # $tcmd will be put between '' but not escaped
-    my ($stdin,$stdout,$user,$ho,$tcmd,$timeout,$extrasshopts) = @_;
+    my ($stdin,$stdout,$returnrc,$user,$ho,$tcmd,$timeout,$extrasshopts) = @_;
     $timeout=30 if !defined $timeout;
     target_adjust_timeout($ho,\$timeout);
-    tcmdex($timeout,$stdin,$stdout,
+    my $r = tcmdex($timeout,$stdin,$stdout,
            'ssh', sshopts(), @{ $extrasshopts || [] },
            sshuho($user,$ho), $tcmd);
+    die "status $r" if $r and !$returnrc;
+    return $r;
 }
-sub target_cmd ($$;$$) { tcmd(undef,undef,'osstest',@_); }
-sub target_cmd_root ($$;$$) { tcmd(undef,undef,'root',@_); }
+sub target_cmd ($$;$$) { tcmd(undef,undef,0, 'osstest',@_); }
+sub target_cmd_root ($$;$$) { tcmd(undef,undef,0, 'root',@_); }
+sub target_cmd_root_rc ($$;$$) { tcmd(undef,undef,1, 'root',@_); }
 
 sub tcmdout {
     my $stdout= IO::File::new_tmpfile();
-    tcmd(undef,$stdout,@_);
+    tcmd(undef,$stdout,0,@_);
     $stdout->seek(0,0) or die "$stdout $!";
     my $r;
     { local ($/) = undef;
@@ -679,7 +685,7 @@ sub target_cmd_output_root ($$;$) { tcmdout('root',@_); }
 
 sub target_cmd_inputfh_root ($$$;$$) {
     my ($tho,$stdinfh,$tcmd,@rest) = @_;
-    tcmd($stdinfh,undef,'root',$tho,$tcmd,@rest);
+    tcmd($stdinfh,undef,0,'root',$tho,$tcmd,@rest);
 }
 
 sub poll_loop ($$$&) {
-- 
2.1.4


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

[PATCH v1 5/7] ts-livepatch: Initial test-cases.

[Konrad Rzeszutek Wilk]

There are 32 test-cases in there. Before we run
any of them we verify that the payload files are present
in /usr/lib/debug.

If so we go through all of the test-cases.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 ts-livepatch | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100755 ts-livepatch

diff --git a/ts-livepatch b/ts-livepatch
new file mode 100755
index 0000000..a996ee9
--- /dev/null
+++ b/ts-livepatch
@@ -0,0 +1,102 @@
+#!/usr/bin/perl -w
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+#
+# 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
+# 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/>.
+
+use strict qw(vars);
+use Osstest;
+use POSIX;
+use Osstest::TestSupport;
+
+tsreadconfig();
+
+my @livepatch_files = ("xen_hello_world.livepatch", "xen_replace_world.livepatch",
+			"xen_bye_world.livepatch", "xen_nop.livepatch");
+
+my @livepatch_tests = (
+	{cmd => "xen-livepatch list", rc => 0},
+	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256},
+	{cmd => "xen-livepatch revert xen_hello_world", rc => 256},
+	{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0},
+	{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 256},
+	{cmd => "xen-livepatch list | grep -q xen_hello_world", rc => 0},
+	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 0},
+	{cmd => "xen-livepatch revert xen_hello_world", rc => 0},
+	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256},
+	{cmd => "xen-livepatch unload xen_hello_world", rc => 0},
+	{cmd => "xen-livepatch unload xen_hello_world", rc => 256},
+	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256},
+	{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0},
+	{cmd => "xen-livepatch load xen_bye_world.livepatch", rc => 0},
+	{cmd => "xl info | grep xen_extra | grep -q \"Bye World\"", rc => 0},
+	{cmd => "xen-livepatch upload xen_replace xen_replace_world.livepatch", rc => 0},
+	{cmd => "xen-livepatch replace xen_replace", rc => 0},
+	{cmd => "xen-livepatch apply xen_hello_world", rc => 256},
+	{cmd => "xen-livepatch apply xen_bye_world", rc => 256},
+	{cmd => "xl info | grep xen_extra | grep -q \"Hello Again Wor\"", rc => 0},
+	{cmd => "xen-livepatch apply xen_replace", rc => 0},
+	{cmd => "xen-livepatch revert xen_replace", rc => 0},
+	{cmd => "xen-livepatch unload xen_replace", rc => 0},
+	{cmd => "xen-livepatch unload xen_hello_world", rc => 0},
+	{cmd => "xen-livepatch unload xen_bye_world", rc => 0},
+	{cmd => "xen-livepatch list | grep -q xen", rc => 256},
+	{cmd => "xen-livepatch load xen_nop.livepatch", rc => 0},
+	{cmd => "xen-livepatch revert xen_nop", rc => 0},
+	{cmd => "xen-livepatch apply xen_nop", rc => 0},
+	{cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc -l` == 1 ]", rc => 0},
+	{cmd => "xen-livepatch unload xen_nop", rc => 256},
+	{cmd => "xen-livepatch revert xen_nop", rc => 0},
+	{cmd => "xen-livepatch unload xen_nop", rc => 0},
+	);
+
+our $ho = selecthost('host');
+
+sub livepatch_test ()
+{
+    logm "Have $#livepatch_tests test-cases\n";
+    my $rc=0;
+    for my $i ( 0 .. $#livepatch_tests ) {
+        my $expected_rc = $livepatch_tests[$i]{rc};
+	my $cmd = "(cd /usr/lib/debug;$livepatch_tests[$i]{cmd})";
+	logm "Executing: '$cmd:' ..";
+	my $rc=target_cmd_root_rc($ho, $cmd);
+	if ( $rc != $expected_rc ) {
+		logm "FAILED (got $rc, expected: $expected_rc): \n";
+		return $rc;
+	}
+	logm ".. OK!\n";
+   }
+   return $rc
+}
+
+sub livepatch_check ()
+{
+    foreach my $file (@livepatch_files)
+    {
+        if (!target_file_exists($ho, "/usr/lib/debug/$$file")) {
+	    die "$file is missing!\n";
+        }
+    }
+    return 0
+}
+
+
+our $livepatch_result = livepatch_check();
+exit $livepatch_result if $livepatch_result;
+$livepatch_result = livepatch_test();
+
+logm("Livepatch test returned $livepatch_result");
+
+exit $livepatch_result;
-- 
2.1.4


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

[PATCH v1 6/7] sg-run-job: Add the test-livepatch.

[Konrad Rzeszutek Wilk]

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 sg-run-job | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sg-run-job b/sg-run-job
index c5cc3c6..7e326b9 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -421,6 +421,11 @@ proc run-job/test-xtf {} {
     run-ts . = ts-xtf-run
 }
 
+proc need-hosts/test-livepatch {} { return host }
+proc run-job/test-livepatch {} {
+    run-ts . = ts-livepatch host
+}
+
 proc test-guest-migr {g} {
     set to_reap [spawn-ts . = ts-migrate-support-check + host $g 1]
     set can_migrate [reap-ts $to_reap]
-- 
2.1.4


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

[PATCH v1 7/7] make-flight/mfi-common: Add them in the matrix.

[Konrad Rzeszutek Wilk]

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 make-flight | 12 ++++++++++++
 mfi-common  |  9 +++++++++
 2 files changed, 21 insertions(+)

diff --git a/make-flight b/make-flight
index d45a0e3..c2add66 100755
--- a/make-flight
+++ b/make-flight
@@ -485,6 +485,17 @@ do_xtf_tests () {
   done
 }
 
+do_livepatch_tests () {
+  if ! branch_wants_livepatch_tests; then
+      return
+  fi
+
+  job_create_test test-$xenarch$kern-$dom0arch-livepatch             \
+       test-livepatch xl $xenarch $dom0arch                          \
+       xen_boot_append='loglvl=all'                                  \
+       all_hostflags=$most_hostflags
+}
+
 do_multivcpu_tests () {
   if [ $xenarch != $dom0arch ]; then
     return
@@ -800,6 +811,7 @@ test_matrix_do_one () {
 
   do_xtf_tests
   do_openstack_tests
+  do_livepatch_tests
 }
 
 if [ x$buildflight = x ]; then
diff --git a/mfi-common b/mfi-common
index 76b93d1..08494f3 100644
--- a/mfi-common
+++ b/mfi-common
@@ -76,6 +76,15 @@ branch_wants_xtf_tests () {
   esac
 }
 
+branch_wants_livepatch_tests () {
+  case "$branch" in
+    osstest*) return 0;;
+    xen-*)    return 0;;
+    livepatch*)     return 0;;
+    *)        return 1;;
+  esac
+}
+
 job_create_build () {
   job_create_build_filter_callback "$@" || return 0
 
-- 
2.1.4


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

Re: [PATCH v1 1/7] ts-xen-build: Enable livepatch.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH v1 1/7] ts-xen-build: Enable livepatch."):
> Livepatch compiles and works on x86/ARM{32|64} so we can
> unconditionaly enable it.

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

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

Re: [PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests. [and 1 more messages]

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests."):
> That come with the Xen git tree (see xen/test/).

I think this and the "build them" patch should be combined.

> +    buildcmd_stamped_logged(600, 'xen', 'tests-install', '',<<END,'') if $dokconfig;

Can you keep the lines down to 75 characters or less please ?

> +        if test -d xen/test; then
> +           mkdir -p dist/install/usr/lib/debug
> +           livepatch_files=`find xen/test/livepatch -name '*.livepatch' -print`
> +           cp \$livepatch_files dist/install/usr/lib/debug

Should this not be in the xen.git Makefiles ?

Also, the result of this is that the tests end up in the tools output
because you haven't fixed `divide'.  Background: each osstest
invocation of ts-xen-build produces two primary deliverables: `' and
`xen' aka `dist' and `xendist'.

I think, but I'm not sure, that these patches contain hypervisor code
and should be in `xendist'.

Thanks,
Ian.

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

Re: [PATCH v1 4/7] OssTest: Add target_cmd_root_rc which returns return code.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH v1 4/7] OssTest: Add target_cmd_root_rc which returns return code."):
> All the different target_cmd_* end up calling tcmdex
> which has the unfortunate side-effect of calling 'die' if
> the SSH sessions results in any return code not zero.
...
> Also we make tcmd behave in the normal old way (die "status $r")
> or if returnrc is set to one, we will return the error code.

Why not add the $returnrc argument to tcmdex ?  That would mean just
adding an extra argument to the callers, rather than extra code.
(Also I would call it "$badstatusok" or something.)

> -sub target_cmd ($$;$$) { tcmd(undef,undef,'osstest',@_); }
> -sub target_cmd_root ($$;$$) { tcmd(undef,undef,'root',@_); }
> +sub target_cmd ($$;$$) { tcmd(undef,undef,0, 'osstest',@_); }
> +sub target_cmd_root ($$;$$) { tcmd(undef,undef,0, 'root',@_); }
> +sub target_cmd_root_rc ($$;$$) { tcmd(undef,undef,1, 'root',@_); }

Can you call this "target_cmd_root_status" please ?

Thanks,
Ian.

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

Re: [PATCH v1 5/7] ts-livepatch: Initial test-cases.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH v1 5/7] ts-livepatch: Initial test-cases."):
> There are 32 test-cases in there. Before we run
> any of them we verify that the payload files are present
> in /usr/lib/debug.
> 
> If so we go through all of the test-cases.

> +my @livepatch_files = ("xen_hello_world.livepatch", "xen_replace_world.livepatch",
> +			"xen_bye_world.livepatch", "xen_nop.livepatch");

Maybe use qw() ?

> +my @livepatch_tests = (
> +	{cmd => "xen-livepatch list", rc => 0},

I think the formatting and ease-of-editing of this list should be
improved.

You could make rc => 0 the default.  Then you wouldn't need to say it.
(But as I say below, you may not want to test rc anyway.)  Also please
use "status" rather than "rc" throughout.

In osstest we use StudlyCaps for hash keys of this kind.  In this case
I suggest "C" instead of "cmd" because the scope is so narrow.  Also,
we put spaces inside the { }, and indent by 4.  In this case you might
want to indent by less to make it fit better.

You might want to consider whether having an entry that's
just a string, rather than a hashref, ought to mean { C => $string }.
But that depends on whether you are going to want to give each test an
ID:

Is it ever possible to continue with the rest of the livepatch tests
after one of these command invocations has failed, or does it leave
the system in an undefined state ?  If it _is_ possible then it would
be possible to provide per-step results to osstest, but that's only
valuable if this would tell us something meaningfully interesting in
terms of regressions - eg if we might tolerate a regression of one
step but still care about the others.

> +	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256},

I'm afraid I think it is rather silly to do this greppery in shell,
when you are writing a Perl script.  Perl is much better at string
matching than shell.  Also your shell rune has bad error handling (for
example, the test will pass if "xl info" coredumps), which would be
tiresome to fix.

I suggest you provide a facility where each test case can provide a
subref to be called on the output from the command.  Then you would
use target_cmd_output_root_status (which you would have to create).

Something like

 { C => "xl info",
   OutputCheck => sub { m/xen_extra/ && !m/Hello World/ } },

where your actual driver code would call OutputCheck with $_ set,
and fail if the result was falseish.

The OutputCheck key would have to be optional so you wouldn't specify
it on each command.

> +	{cmd => "xen-livepatch revert xen_hello_world", rc => 256},

libxl exit statuses are not very good and should not be relied on,
really.  Instead, you should arrange to:
  * Run the command with LC_MESSAGES=C
  * Fail the test if the exit status is zero
  * Collect its stderr output (by appending 2>&1)
  * Match the stderr output against a regexp in the perl program

Something like:

 { C => "xen-livepatch revert xen_hello_world",
   Fails => qr{xl: cannot revert xen_hello_world which is not installed} },

or whatever the message actually is.  Your actual driver code would
see Fails in the hashref and DTRT.

> +	{cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc -l` == 1 ]", rc => 0},

You really really wanted to write this in shell, didn't you ? :-)
But getting the shell error handling right will be a pain...

> +sub livepatch_test ()
> +{

Style: { on same line as ().

> +    logm "Have $#livepatch_tests test-cases\n";

This is a lie because $#livepatch_tests is the highest populated array
index.  You mean  "Have ".(scalar @livepatch_tests)."..."
or sprintf "...%d...", (scalar @livepatch_tests)
or something.

> +    my $rc=0;
> +    for my $i ( 0 .. $#livepatch_tests ) {

Style: no spaces inside ( ).  But: please use

   foreach my $test (@livepatch_tests) {

(since I don't think you need the array index.  And the local variable
$test being effectively $livepatch_tests[$i] makes the rest of the
code much easier to read.)

> +        my $expected_rc = $livepatch_tests[$i]{rc};
> +	my $cmd = "(cd /usr/lib/debug;$livepatch_tests[$i]{cmd})";

YM "set -e; cd ...; $test->{C}".

The ( ) are redundant and you need the set -e in case the cd fails.

> +	logm "Executing: '$cmd:' ..";

target_cmd_* already log.  You can probably drop that.

> +	my $rc=target_cmd_root_rc($ho, $cmd);
> +	if ( $rc != $expected_rc ) {
> +		logm "FAILED (got $rc, expected: $expected_rc): \n";
> +		return $rc;

This bubbling up of the failure code from in the middle here seems
unnecessary.  Why not die, here ?

This bit of code will become more sophisticated if you take my
suggestions about OutputCheck and Fails, above.

> +sub livepatch_check ()
> +{
> +    foreach my $file (@livepatch_files)
> +    {

Style.

> +        if (!target_file_exists($ho, "/usr/lib/debug/$$file")) {
> +	    die "$file is missing!\n";

I'm not sure why you actually need to check this separately.  Won't
the later tests spot this ?  In which case this simply generates log
clutter.  But I don't object to the separate check.

> +our $livepatch_result = livepatch_check();
> +exit $livepatch_result if $livepatch_result;

Your livepatch_check already dies or returns 0.  I suggest you delete
the return 0 from it, and don't check separately.  In perl it is best
practice to convert fatal errors to exceptions (by calling die) ASAP.
That avoids a lot of error handling code in the rest of the call
stack.  Subroutines which are called only to check things, or only for
their side effects, don't need to explicitly return anything (and
their callers don't need to test the return value).

Thanks,
Ian.

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

Re: [PATCH v1 6/7] sg-run-job: Add the test-livepatch.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH v1 6/7] sg-run-job: Add the test-livepatch."):
> +proc need-hosts/test-livepatch {} { return host }
> +proc run-job/test-livepatch {} {
> +    run-ts . = ts-livepatch host

  +    run-ts . = ts-livepatch + host

Please.  I think that will make a prettier testid.

Otherwise this is fine.

Thanks,
Ian.

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

Re: [PATCH v1 7/7] make-flight/mfi-common: Add them in the matrix.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH v1 7/7] make-flight/mfi-common: Add them in the matrix."):
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Can you accompany this with a diff of the output of
   standalone-generate-dump-flight-runvars
before and after ?

You probably want
  AP_FETCH_PLACEHOLDERS=y eatmydata ./standalone-generate-dump-flight-runvars

Ian.

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

Re: [PATCH v1] OSSTest test-harness for livepatches.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH v1] OSSTest test-harness for livepatches."):
> With this test-harness I am able to run the regressions tests on livepatches
> on Xen 4.8. I only used the standalone tests. The incantations was:

Thanks.

> There are still some TODOs such as:
>  - Test this on ARM (it should work out of the box),

We can throw it at the infrastructure and see what comes out.

>  - Not sure how to hook this up to the real OSSTest?

I think you have supplied all the necessary moving parts.

>  - It is missing checks to only run the testcases on Xen 4.8 or later.

That could be done in make-flight.  But:

 * You need to make your ts-xen-build changes work with
   earlier Xens which do not have the `make tests' etc.

 * If you split your hypervisor test patches into their own dist (eg
   `xenlptdist' for `live patch test'), and do not generate that
   output when the Xen build didn't support it, the existing machinery
   will spot that your live tests are blocked on earlier Xen, and
   report that as a single non-regression test failure "never pass"
   for earlier Xens - and it will do that before it allocates
   a host.

Thanks,
Ian.

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

Re: [PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests. [and 1 more messages]

[Konrad Rzeszutek Wilk]

On Thu, Nov 17, 2016 at 11:49:19AM +0000, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("[PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests."):
> > That come with the Xen git tree (see xen/test/).
> 
> I think this and the "build them" patch should be combined.
> 
> > +    buildcmd_stamped_logged(600, 'xen', 'tests-install', '',<<END,'') if $dokconfig;
> 
> Can you keep the lines down to 75 characters or less please ?
> 
> > +        if test -d xen/test; then
> > +           mkdir -p dist/install/usr/lib/debug
> > +           livepatch_files=`find xen/test/livepatch -name '*.livepatch' -print`
> > +           cp \$livepatch_files dist/install/usr/lib/debug
> 
> Should this not be in the xen.git Makefiles ?

Jan didn't like it (as part of the normal 'install' stanza).

I could add it in xen/test/Makefile, but I had a hard time executing
anything inside 'xen' sub-directories by themselves, aka:

 make -C xen/test install

As the 'xen/test/livepatch/Makefile' does:
include $(XEN_ROOT)/Config.mk

(and other) and the XEN_ROOT is not available unless you run it from
within 'xen' directory.

Which means I would have to add a new top-level target, such as:

 make -C xen test_install

or such. But then it is not exactly sure where one would install
the "tests"? /usr/lib/debug? /usr/lib/xen/debug/ ?

I figured it would be easier if it was left unimplemented and folks
just copied the files out of there.

 
> 
> Also, the result of this is that the tests end up in the tools output
> because you haven't fixed `divide'.  Background: each osstest
> invocation of ts-xen-build produces two primary deliverables: `' and
> `xen' aka `dist' and `xendist'.
> 
> I think, but I'm not sure, that these patches contain hypervisor code
> and should be in `xendist'.

In the cover letter you mentioned that it may be good to have an
xenlptdist.tar.gz which would only contain the livepatch test-cases.

And then we could use the existence of that file as a check for
the hypervisor having the support?

If I squash this patch in this one:

diff --git a/ts-xen-build b/ts-xen-build
index 1b36b9c..1137947 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -170,11 +170,11 @@ END
                 $make_prefix make -C xen tests
             fi
 END
-    buildcmd_stamped_logged(600, 'xen', 'tests-install', '',<<END,'') if $dokconfig;
+    buildcmd_stamped_logged(600, 'xen', 'xenlpt-install', '',<<END,'') if $dokconfig;
         if test -d xen/test; then
-           mkdir -p dist/install/usr/lib/debug
+           mkdir -p dist/xenlptinstall/usr/lib/debug
            livepatch_files=`find xen/test/livepatch -name '*.livepatch' -print`
-           cp \$livepatch_files dist/install/usr/lib/debug
+           cp \$livepatch_files dist/xenlptinstall/usr/lib/debug
         fi
 END
 }

It should in theory (testing it now) do the right thing. Now just need
to figure out how to gate the execution of ts-livepatch on the existence
of that file (in a non-hackish way).

> 
> Thanks,
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH v1 5/7] ts-livepatch: Initial test-cases.

[Konrad Rzeszutek Wilk]

On Thu, Nov 17, 2016 at 12:21:58PM +0000, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("[PATCH v1 5/7] ts-livepatch: Initial test-cases."):
> > There are 32 test-cases in there. Before we run
> > any of them we verify that the payload files are present
> > in /usr/lib/debug.
> > 
> > If so we go through all of the test-cases.

Thank you!

Let me answer only to some as I am fixing the rest per your comments:
> 
> Is it ever possible to continue with the rest of the livepatch tests
> after one of these command invocations has failed, or does it leave
> the system in an undefined state ?  If it _is_ possible then it would

If the invocations have failed that could mean: 
 - We don't have livepatching enabled (somebody swapped the hypervisor?)
 - The livepatching did not work right and we have code in the
   hypervisor that patched something else. Undefined results.
 - It may be that the test-case failed to load due to dependencies
   issues - and while that means the system can recover - the test
   should fail immediately.

> be possible to provide per-step results to osstest, but that's only
> valuable if this would tell us something meaningfully interesting in
> terms of regressions - eg if we might tolerate a regression of one
> step but still care about the others.

At this stage I believe all of these test-cases should work. If they
don't we got big problems.
..
> > +	{cmd => "xen-livepatch revert xen_hello_world", rc => 256},
> 
> libxl exit statuses are not very good and should not be relied on,
> really.  Instead, you should arrange to:

But this is not libxl. The error values are very much defined
by xen-livepatch.

>   * Run the command with LC_MESSAGES=C

Aye.
>   * Fail the test if the exit status is zero

But some of the test-cases are suppose to return 0 - as the program
executed correctly.

>   * Collect its stderr output (by appending 2>&1)
>   * Match the stderr output against a regexp in the perl program

Right.

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

Re: [PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests. [and 1 more messages]

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests. [and 1 more messages]"):
> On Thu, Nov 17, 2016 at 11:49:19AM +0000, Ian Jackson wrote:
> > Konrad Rzeszutek Wilk writes ("[PATCH v1 3/7] ts-xen-build: Install livepatch regressions tests."):
> > > +        if test -d xen/test; then
> > > +           mkdir -p dist/install/usr/lib/debug
> > > +           livepatch_files=`find xen/test/livepatch -name '*.livepatch' -print`
> > > +           cp \$livepatch_files dist/install/usr/lib/debug
> > 
> > Should this not be in the xen.git Makefiles ?
> 
> Jan didn't like it (as part of the normal 'install' stanza).

I think I agree with Jan that it should not be included in the result
of `make install'.

But can it not be a new toplevel target ?  Jan, would that be OK with
you ?

Normally this kind of file-searching and -installing logic is best
done in the xen.git Makefiles (or indeed in the Makefiles of whatever
is being tested), because that way xen.git can change what these files
are, how they are laid out in the source code, and so on, without
disturbing osstest.

What you do above makes the xen.git source tree layout part of the
public interface.  (Or, perhaps, causes osstest to falsely assume that
it is part of the public interface...)

> Which means I would have to add a new top-level target, such as:
>  make -C xen test_install
> or such. But then it is not exactly sure where one would install
> the "tests"? /usr/lib/debug? /usr/lib/xen/debug/ ?

Yes, something like that.  I don't think the path matters very much.
I think something like /usr[/local]/lib/xen/debug is probably best.

> > Also, the result of this is that the tests end up in the tools output
> > because you haven't fixed `divide'.  Background: each osstest
> > invocation of ts-xen-build produces two primary deliverables: `' and
> > `xen' aka `dist' and `xendist'.
> > 
> > I think, but I'm not sure, that these patches contain hypervisor code
> > and should be in `xendist'.
> 
> In the cover letter you mentioned that it may be good to have an
> xenlptdist.tar.gz which would only contain the livepatch test-cases.

Yes.

> And then we could use the existence of that file as a check for
> the hypervisor having the support?

You mean the existence of that output.  Earlier I wrote:

 ] * If you split your hypervisor test patches into their own dist (eg
 ]   `xenlptdist' for `live patch test'), and do not generate that
 ]    output when the Xen build didn't support it, the existing machinery
 ]    will spot that your live tests are blocked on earlier Xen, and
 ]    report that as a single non-regression test failure "never pass"
 ]    for earlier Xens - and it will do that before it allocates
 ]    a host.

> It should in theory (testing it now) do the right thing. Now just need
> to figure out how to gate the execution of ts-livepatch on the existence
> of that file (in a non-hackish way).

What I mean is that you do not need to do anything.  Your new test job
will automatically be "started" on every Xen branch, after the build
jobs it refers to have completed.  But the first thing it does, before
it even goes as far as trying to find a host to run on, is to check
whether the builds it uses succeeded.

I think (and I asserted above) that if your xenlptdist output (or
whatever you want to call it) is not generated at all, by
ts-xen-build, this check will fail, and the attempt to run livepatch
tests will be abandoned.

Ie all will be as it should be.

Ian.

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

Re: [PATCH v1 5/7] ts-livepatch: Initial test-cases.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH v1 5/7] ts-livepatch: Initial test-cases."):
> Let me answer only to some as I am fixing the rest per your comments:
> 
> On Thu, Nov 17, 2016 at 12:21:58PM +0000, Ian Jackson wrote:
> > Is it ever possible to continue with the rest of the livepatch tests
> > after one of these command invocations has failed, or does it leave
> > the system in an undefined state ?  If it _is_ possible then it would
> 
> If the invocations have failed that could mean: 
>  - We don't have livepatching enabled (somebody swapped the hypervisor?)
>  - The livepatching did not work right and we have code in the
>    hypervisor that patched something else. Undefined results.
>  - It may be that the test-case failed to load due to dependencies
>    issues - and while that means the system can recover - the test
>    should fail immediately.

Right.

> > be possible to provide per-step results to osstest, but that's only
> > valuable if this would tell us something meaningfully interesting in
> > terms of regressions - eg if we might tolerate a regression of one
> > step but still care about the others.
> 
> At this stage I believe all of these test-cases should work. If they
> don't we got big problems.

OK.  So I think in that case your script should probably fail
completely, and not run any of the further test cases, if any test
fails.

> > > +	{cmd => "xen-livepatch revert xen_hello_world", rc => 256},
> > 
> > libxl exit statuses are not very good and should not be relied on,
> > really.  Instead, you should arrange to:
> 
> But this is not libxl. The error values are very much defined
> by xen-livepatch.

No, they clearly aren't.  I just looked at the code and main()
returns like this:
    return !!ret;

So that means that the exit status is either 0 meaning "all is good"
or 1 meaning "something, which might be anything at all, went wrong".

Tests should be arranged to figure out whether the test fails in the
expected way.  With programs whose exit status does not discriminate
between causes of failure, this has to be done by grepping error
messages :-/.

> >   * Run the command with LC_MESSAGES=C
> 
> Aye.
> 
> >   * Fail the test if the exit status is zero
> 
> But some of the test-cases are suppose to return 0 - as the program
> executed correctly.

I meant, if you expect the test to fail, you should insist that it
fails.

Ian.

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

Re: [PATCH v1 7/7] make-flight/mfi-common: Add them in the matrix.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH v1 7/7] make-flight/mfi-common: Add them in the matrix."):
> On Thu, Nov 17, 2016 at 12:24:20PM +0000, Ian Jackson wrote:
> > Can you accompany this with a diff of the output of
> >    standalone-generate-dump-flight-runvars
> > before and after ?
> 
> I get:
> 
> Repos must be configured in /home/konrad/.xen-osstest/config
> make-flight libvirt failed

It just means the config variable literally `Repos'.  From README:

  Repos              Full path to a temporary directory where repositories
                     can be cloned. This is needed for anything which uses
                     cr-daily-branch, including "./standalone make-flight"
                     and "standalone-generate-dump-flight-runvars".

Ian.

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

Re: [PATCH v1 7/7] make-flight/mfi-common: Add them in the matrix.

[Konrad Rzeszutek Wilk]

On Thu, Nov 17, 2016 at 12:24:20PM +0000, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("[PATCH v1 7/7] make-flight/mfi-common: Add them in the matrix."):
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Can you accompany this with a diff of the output of
>    standalone-generate-dump-flight-runvars
> before and after ?

I get:

Repos must be configured in /home/konrad/.xen-osstest/config
make-flight libvirt failed


Any ideas which repos I should configure?

And I did do ./sg-run-job build-amd64-libvirt

right beforehand?

> 
> You probably want
>   AP_FETCH_PLACEHOLDERS=y eatmydata ./standalone-generate-dump-flight-runvars

That is what I did.
> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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