[PATCH OSSTEST 00/11] osstest: add support to examine the needed memdisk flags for each host

[PATCH OSSTEST 00/11] osstest: add support to examine the needed memdisk flags for each host

[Roger Pau Monne]

Hello,

This builds on top of my previous osstest FreeBSD support series, and
expands the examine flight in order to test which memdisk options
should be used for each host. Hopefully all of this will be automatic
upon running a examine flight. The required options are detected by
ts-memdisk-try-append and stored into the database by
ts-examine-hostprops-save.

This has been tested except for ts-examine-hostprops-save because I
haven't run it in a flight with "real" blessing.

The series can be found at:

git://xenbits.xen.org/people/royger/osstest.git freebsd_v9

Which as said is already rebased on top of the previous FreeBSD
series.

Thanks, Roger.

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

[PATCH OSSTEST 01/11] netboot_memdisk: allow each host to have different append values

[Roger Pau Monne]

Some hosts require "append raw" [0] when booting with memdisk, while
others don't. This is based on the hardware/BIOS, and needs to be set
on a per-host basis.

In order to do this, add a new "MemdiskAppend" host property and make
use of it in the setup_netboot_memdisk helper.

[0] http://www.syslinux.org/wiki/index.php?title=MEMDISK#Memory_access_method

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Allow to manually pass append parameters.
---
 Osstest/TestSupport.pm | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index b858ac82..c24146c0 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -2672,8 +2672,11 @@ default local
 END
 }
 
-sub setup_netboot_memdisk ($$) {
-    my ($ho, $img) = @_;
+sub setup_netboot_memdisk ($$;$) {
+    my ($ho, $img, $append) = @_;
+
+    $append //= get_host_property($ho, "MemdiskAppend", "");
+    $append = $append ? "append $append" : "";
     setup_netboot_bootcfg($ho, <<END);
 serial 0 $c{Baud}
 timeout 5
@@ -2682,12 +2685,7 @@ label overwrite
         menu default
         kernel memdisk
         initrd $img
-        # NB: according to the memdisk syslinux wikipage [0]
-        # adding "append raw" is required in order to boot on
-        # some boxes, and in fact some hardware will not boot
-        # without it.
-        # [0] http://www.syslinux.org/wiki/index.php?title=MEMDISK#Memory_access_method
-        append raw
+        $append
 default overwrite
 END
 }
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 02/11] ts-freebsd-host-install: fix image permissions

[Roger Pau Monne]

Make sure images copied to the tftp path have the right permissions,
so use dd instead of cp, which will obviously not preserve the
original permissions.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 ts-freebsd-host-install | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ts-freebsd-host-install b/ts-freebsd-host-install
index 321763b0..483b9aec 100755
--- a/ts-freebsd-host-install
+++ b/ts-freebsd-host-install
@@ -76,7 +76,8 @@ targetpath=$4
 cd $basedir
 mkdir -p `dirname $sharedpath`
 if [ ! -f $sharedpath ]; then
-    cp $imagepath $sharedpath.tmp
+    rm $sharedpath.tmp
+    dd if=$imagepath of=$sharedpath.tmp
     mv $sharedpath.tmp $sharedpath
 fi
 rm -f $targetpath
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 03/11] sg-run-job: fix typo in the examine jobs

[Roger Pau Monne]

proc prep-job/host-examine-xen is declared twice, one of them should
be prep-job/host-examine-linux instead.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 sg-run-job | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sg-run-job b/sg-run-job
index b7ce963a..ed1ed3c8 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -671,7 +671,7 @@ proc prep-job/host-examine-xen {} { examine-host-prep }
 proc run-job/host-examine-xen {} { examine-host-examine xen }
 
 proc need-hosts/host-examine-linux {} { return {} }
-proc prep-job/host-examine-xen {} { examine-host-prep }
+proc prep-job/host-examine-linux {} { examine-host-prep }
 proc need-hosts/host-examine-linux {} { examine-host-examine debian }
 
 #---------- builds ----------
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Roger Pau Monne]

This is from the code in mg-hosts. Switch cmd_setprops to use the
newly introduced function.

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 Osstest/TestSupport.pm | 31 ++++++++++++++++++++++++++++++-
 mg-hosts               | 31 +------------------------------
 2 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index c24146c0..ff0de14b 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -80,7 +80,7 @@ BEGIN {
                       get_target_property get_host_native_linux_console
                       hostnamepath hostnamepath_list set_runtime_hostflag
                       power_state power_cycle power_cycle_sleep
-                      serial_fetch_logs
+                      serial_fetch_logs set_host_property
                       propname_massage propname_check
          
                       get_stashed open_unique_stashfile compress_stashed
@@ -1183,6 +1183,35 @@ sub get_host_property ($$;$) {
     return defined($val) ? $val : $defval;
 }
 
+sub set_host_property {
+    my $oldvalq = $dbh_tests->prepare(<<END);
+	SELECT val FROM resource_properties
+	      WHERE restype='host' and resname=? AND name=?
+END
+    my $rmvalq = $dbh_tests->prepare(<<END);
+	DELETE FROM resource_properties
+	      WHERE restype='host' and resname=? AND name=?
+END
+    my $newvalq = $dbh_tests->prepare(<<END);
+	INSERT INTO resource_properties (restype,resname,name,val)
+	                         VALUES ('host', ?,?,?)
+END
+    my ($host,$name,$oldval,$val) = @_;
+
+    if (defined $oldval) {
+        $oldvalq->execute($host,$name);
+        my $row = $oldvalq->fetchrow_hashref();
+        die if $row && !length $row->{'val'};
+        my $gotoldval = $row ? $row->{'val'} : '';
+        die "$host $name = '$gotoldval' != '$oldval'"
+        unless ($gotoldval eq $oldval || $gotoldval eq $val);
+    }
+    $rmvalq->execute($host,$name);
+    if (length $val) {
+        $newvalq->execute($host,$name,$val);
+    }
+}
+
 sub get_target_property ($$;$);
 sub get_target_property ($$;$) {
     my ($ho, $prop, $defval) = @_;
diff --git a/mg-hosts b/mg-hosts
index 5cdece56..3d1321a5 100755
--- a/mg-hosts
+++ b/mg-hosts
@@ -292,35 +292,6 @@ END
 }
 
 sub cmd_setprops () {
-    my $oldvalq = $dbh_tests->prepare(<<END);
-	SELECT val FROM resource_properties
-	      WHERE restype='host' and resname=? AND name=?
-END
-    my $rmvalq = $dbh_tests->prepare(<<END);
-	DELETE FROM resource_properties
-	      WHERE restype='host' and resname=? AND name=?
-END
-    my $newvalq = $dbh_tests->prepare(<<END);
-	INSERT INTO resource_properties (restype,resname,name,val)
-	                         VALUES ('host', ?,?,?)
-END
-
-    my $update = sub {
-	my ($host,$name,$oldval,$val) = @_;
-	if (defined $oldval) {
-	    $oldvalq->execute($host,$name);
-	    my $row = $oldvalq->fetchrow_hashref();
-	    die if $row && !length $row->{'val'};
-	    my $gotoldval = $row ? $row->{'val'} : '';
-	    die "$host $name = '$gotoldval' != '$oldval'"
-		unless ($gotoldval eq $oldval || $gotoldval eq $val);
-	}
-	$rmvalq->execute($host,$name);
-	if (length $val) {
-	    $newvalq->execute($host,$name,$val);
-	}
-    };
-
     update_hosts([qw(resources)], sub {
 	my ($host,$section) = @_;
 	my ($name,$oldval,$val);
@@ -331,7 +302,7 @@ END
 	} else {
 	    die "@$section ?";
 	}
-	$update->($host,$name,$oldval,$val);
+	set_host_property($host,$name,$oldval,$val);
     });
 }
 
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 05/11] mfi-common: move set_freebsd_runvars to mfi-common

[Roger Pau Monne]

So that it can also be used by make-hosts-flight. No functional change
intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 make-freebsd-flight | 31 -------------------------------
 mfi-common          | 31 +++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/make-freebsd-flight b/make-freebsd-flight
index 64dfe9a6..72695742 100755
--- a/make-freebsd-flight
+++ b/make-freebsd-flight
@@ -36,37 +36,6 @@ job_create_build_filter_callback () {
     :
 }
 
-set_freebsd_runvars () {
-    # Caller should have done if required:
-    # local freebsd_runvars
-    #
-    # Figure out where are the installer binaries. The order is the
-    # following:
-    #
-    # 1. Env variable FREEBSD_<arch>_BUILDJOB: use the output from a
-    # previous build-<arch>-freebsd.
-    #
-    # 2. Env variables FREEBSD_DIST, FREEBSD_VERSION: set before calling
-    # into make-flight, provide the path to the installer image, the sets
-    # to install and the version being installed.
-    #
-    # 3. Config file FreeBSDDist, FreeBSDVersion: same as 2. except that
-    # they are set on the config file.
-    #
-    envvar="FREEBSD_${arch^^}_BUILDJOB"
-    if [ -n "${!envvar}" ]; then
-        freebsd_runvars="freebsdbuildjob=${!envvar}"
-    elif [ -n "$FREEBSD_DIST" ] && [ -n "$FREEBSD_VERSION" ]; then
-        freebsd_runvars="freebsd_distpath=$FREEBSD_DIST/$arch \
-                         freebsd_version=$FREEBSD_VERSION"
-    else
-        distpath=`getconfig "FreeBSDDist"`
-        version=`getconfig "FreeBSDVersion"`
-        freebsd_runvars="freebsd_distpath=$distpath/$arch \
-                         freebsd_version=$version"
-    fi
-}
-
 for arch in "$arches"; do
     set_freebsd_runvars
     job_create_build build-$arch-freebsd build-freebsd          \
diff --git a/mfi-common b/mfi-common
index 4827c827..8a9546ab 100644
--- a/mfi-common
+++ b/mfi-common
@@ -113,6 +113,37 @@ set_hostos_runvars () {
   esac
 }
 
+set_freebsd_runvars () {
+    # Caller should have done if required:
+    # local freebsd_runvars
+    #
+    # Figure out where are the installer binaries. The order is the
+    # following:
+    #
+    # 1. Env variable FREEBSD_<arch>_BUILDJOB: use the output from a
+    # previous build-<arch>-freebsd.
+    #
+    # 2. Env variables FREEBSD_DIST, FREEBSD_VERSION: set before calling
+    # into make-flight, provide the path to the installer image, the sets
+    # to install and the version being installed.
+    #
+    # 3. Config file FreeBSDDist, FreeBSDVersion: same as 2. except that
+    # they are set on the config file.
+    #
+    local envvar="FREEBSD_${arch^^}_BUILDJOB"
+    if [ -n "${!envvar}" ]; then
+        freebsd_runvars="freebsdbuildjob=${!envvar}"
+    elif [ -n "$FREEBSD_DIST" ] && [ -n "$FREEBSD_VERSION" ]; then
+        freebsd_runvars="freebsd_distpath=$FREEBSD_DIST/$arch \
+                         freebsd_version=$FREEBSD_VERSION"
+    else
+        local distpath=`getconfig "FreeBSDDist"`
+        local version=`getconfig "FreeBSDVersion"`
+        freebsd_runvars="freebsd_distpath=$distpath/$arch \
+                         freebsd_version=$version"
+    fi
+}
+
 create_build_jobs () {
 
   local arch
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 06/11] TestSupport: introduce hostprop_putative_record

[Roger Pau Monne]

This is used to store tentative host properties in the runvars of a
job, with the expectation that at some point (ie: at the end of the
job) they will be turned into real properties stored in the database.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 Osstest/TestSupport.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index ff0de14b..6939c03f 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -82,6 +82,7 @@ BEGIN {
                       power_state power_cycle power_cycle_sleep
                       serial_fetch_logs set_host_property
                       propname_massage propname_check
+                      hostprop_putative_record
          
                       get_stashed open_unique_stashfile compress_stashed
                       dir_identify_vcs
@@ -1212,6 +1213,12 @@ END
     }
 }
 
+sub hostprop_putative_record ($$$) {
+    my ($ho, $prop, $val) = @_;
+
+    store_runvar("hostprop_$ho->{Name}_$prop", $val);
+}
+
 sub get_target_property ($$;$);
 sub get_target_property ($$;$) {
     my ($ho, $prop, $defval) = @_;
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 07/11] ts-freebsd-host-install: add option to test memdisk options

[Roger Pau Monne]

This is needed in order to figure out which memdisk options should be
used to boot the images on each specific box.

Note that upon success the script stores the tentative host property
in the runvars.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 ts-freebsd-host-install | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/ts-freebsd-host-install b/ts-freebsd-host-install
index 483b9aec..3b0ab970 100755
--- a/ts-freebsd-host-install
+++ b/ts-freebsd-host-install
@@ -41,6 +41,25 @@ use Osstest::TestSupport;
 
 tsreadconfig();
 
+if ($r{'arch'} !~ m/amd64/g) {
+    logm("Arch $r{'arch'} not supported!");
+    exit 0;
+}
+
+our $bootonly;
+our $memdisk_append;
+while (@ARGV && $ARGV[0] =~ m/^-/g) {
+    if ($ARGV[0] =~ m/^--memdiskappend=(.*)/) {
+        $memdisk_append = $1;
+    } elsif ($ARGV[0] eq "--testboot") {
+        $memdisk_append //= "";
+        $bootonly = 1;
+    } else {
+        die "Unknown argument $ARGV[0]";
+    }
+    shift @ARGV;
+}
+
 our ($whhost) = @ARGV;
 $whhost ||= 'host';
 our $ho= selecthost($whhost);
@@ -95,7 +114,7 @@ END
 
     # Setup the pxelinux config file
     logm("Booting from installer image at $pxeimg");
-    setup_netboot_memdisk($ho, $pxeimg);
+    setup_netboot_memdisk($ho, $pxeimg, $memdisk_append);
 }
 
 sub install () {
@@ -247,6 +266,12 @@ power_state($ho, 1);
 logm("Waiting for the installer to boot");
 await_tcp(get_timeout($ho,'reboot',$timeout), 5, $ho);
 
+if ($bootonly) {
+    hostprop_putative_record($ho, "MemdiskAppend", $memdisk_append)
+        if $memdisk_append;
+    exit 0;
+}
+
 # Next boot will be from local disk
 setup_netboot_local($ho);
 
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 08/11] ts-memdisk-try-append: introduce a script to test memdisk options

[Roger Pau Monne]

The intended usage is to run this script against every host in order
to record the possible needed memdisk flags.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 ts-memdisk-try-append | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100755 ts-memdisk-try-append

diff --git a/ts-memdisk-try-append b/ts-memdisk-try-append
new file mode 100755
index 00000000..9c6b56f2
--- /dev/null
+++ b/ts-memdisk-try-append
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2017 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/>.
+
+set -xe -o posix
+
+if ./ts-freebsd-host-install --testboot $@; then
+    exit 0
+elif ./ts-freebsd-host-install --testboot --memdiskappend="raw" $@; then
+    exit 0
+fi
+
+exit 1
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 09/11] ts-examine-hostprops-save: introduce a script to save properties

[Roger Pau Monne]

The introduce script turns the properties stored in the runvars using
the format hostprop_$hotname_$prop=$val into host properties stored in
the database.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 ts-examine-hostprops-save | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100755 ts-examine-hostprops-save

diff --git a/ts-examine-hostprops-save b/ts-examine-hostprops-save
new file mode 100755
index 00000000..ce9a643d
--- /dev/null
+++ b/ts-examine-hostprops-save
@@ -0,0 +1,39 @@
+#!/usr/bin/perl -w
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2017 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/>.
+
+use strict qw(vars);
+use DBI;
+use POSIX;
+
+unshift @INC, qw(.);
+use Osstest;
+use Osstest::TestSupport;
+
+tsreadconfig();
+
+exit 0 if intended_blessing() ne "real";
+
+logm("setting host properties");
+
+foreach my $k (sort keys %r) {
+    next unless $k =~ m/^hostprop_([^_]*)_([^_]*)$/;
+    my $hostname= $1;
+    my $prop=$2;
+
+    logm("recording for $hostname $prop=$r{$k}");
+    set_host_property($hostname, $prop, $r{$k});
+}
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 10/11] make-hosts-flight: set runvars for FreeBSD test

[Roger Pau Monne]

This is needed in order to run the memdisk test.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 make-hosts-flight | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/make-hosts-flight b/make-hosts-flight
index 0152dfe1..d5670857 100755
--- a/make-hosts-flight
+++ b/make-hosts-flight
@@ -69,10 +69,13 @@ hosts_iterate () {
     case $kern in
       xen|linux)
         local di_version=`getconfig_TftpDiVersion_suite $suite`
+        local freebsd_runvars
+        set_freebsd_runvars
         runvars+=" 
                    kernkind=pvops
                    all_host_di_version=$di_version
                    all_host_suite=$suite
+                   $freebsd_runvars
                  "
         ;;
     esac
-- 
2.11.0 (Apple Git-81)


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

[PATCH OSSTEST 11/11] sg-run-job: hook the memdisk test into examine

[Roger Pau Monne]

Hook the memdisk parameter detection and the saving of the host
properties into the examine jobs.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 sg-run-job | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sg-run-job b/sg-run-job
index ed1ed3c8..4df89410 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -658,11 +658,13 @@ proc examine-host-examine {install} {
 	examine-host-install-$install
 	run-ts .   =            ts-examine-serial-pre + host
 	run-ts .   reboot       ts-host-reboot        + host
+	run-ts .   =            ts-memdisk-try-append + host
     }
     run-ts !broken capture-logs ts-logs-capture       + host
     if {$ok} {
 	run-ts -.  =           ts-examine-serial-post + host
 	run-ts .   =           ts-examine-logs-save   + host
+	run-ts .   =           ts-examine-hostprops-save
     }
 }
 
-- 
2.11.0 (Apple Git-81)


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

Re: [PATCH OSSTEST 02/11] ts-freebsd-host-install: fix image permissions

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 02/11] ts-freebsd-host-install: fix image permissions"):
> Make sure images copied to the tftp path have the right permissions,
> so use dd instead of cp, which will obviously not preserve the
> original permissions.

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 OSSTEST 03/11] sg-run-job: fix typo in the examine jobs

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 03/11] sg-run-job: fix typo in the examine jobs"):
> proc prep-job/host-examine-xen is declared twice, one of them should
> be prep-job/host-examine-linux instead.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Thank you.

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 OSSTEST 04/11] TestSupport: introduce set_host_prop

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> This is from the code in mg-hosts. Switch cmd_setprops to use the
> newly introduced function.

I think this needs to be abstracted through jobdb.  Certainly these
SQL statements operating on the host_properties table are valid in
Executive mode only and shouldn't appear in TestSupport.pm.

If you are lucky then mg-hosts runs with $mjobdb set.  In which case
you make it call $mjobdb->set_host_prop.

I considered asking you to make a wrapper in TestSupport or
Osstest.pm.  But: actually, I think the wrapper in TestSupport ought
to take a $ho, not a hostname.  That would guarantee that a ts-*
script which used it had called selecthost and had the host allocated.

Whereas mg-hosts setprops is exceptional: it wants to be able to set
the property on a host without the user having allocated it.  I think
that justifies the "back door" entry via $mjobdb.

What do you think ?

Ian.

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

Re: [PATCH OSSTEST 05/11] mfi-common: move set_freebsd_runvars to mfi-common

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 05/11] mfi-common: move set_freebsd_runvars to mfi-common"):
> So that it can also be used by make-hosts-flight. No functional change
> intended.

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 OSSTEST 06/11] TestSupport: introduce hostprop_putative_record

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 06/11] TestSupport: introduce hostprop_putative_record"):
> This is used to store tentative host properties in the runvars of a
> job, with the expectation that at some point (ie: at the end of the
> job) they will be turned into real properties stored in the database.

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 OSSTEST 07/11] ts-freebsd-host-install: add option to test memdisk options

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 07/11] ts-freebsd-host-install: add option to test memdisk options"):
> This is needed in order to figure out which memdisk options should be
> used to boot the images on each specific box.
...
> +if ($r{'arch'} !~ m/amd64/g) {
> +    logm("Arch $r{'arch'} not supported!");

This clearly can't be right because presumably at least i386 would
work too.  I don't know why you need this check.

> +    exit 0;

WTF ?  You don't want ts-freebsd-host-install to exit 0 if it gets an
unknown architecture.

...  Oh I see.  You have misplaced this check, which should only be
effective when we are testing boot arguments.

Ian.

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

Re: [PATCH OSSTEST 07/11] ts-freebsd-host-install: add option to test memdisk options

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 07/11] ts-freebsd-host-install: add option to test memdisk options"):
...
> +if ($bootonly) {
> +    hostprop_putative_record($ho, "MemdiskAppend", $memdisk_append)
> +        if $memdisk_append;
> +    exit 0;

I think this should be a separate option --record-memdisk or
something.  Since you might want to do a boot test for other reasons
and having it mess with the host properties would be very unexpected.

Ian.

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

Re: [PATCH OSSTEST 08/11] ts-memdisk-try-append: introduce a script to test memdisk options

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 08/11] ts-memdisk-try-append: introduce a script to test memdisk options"):
> The intended usage is to run this script against every host in order
> to record the possible needed memdisk flags.

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

Except that I think my other comments are going to mean you need to
change this to 1. be conditional on arch 2. pass an extra
--store-memdisk-property or something argument.

Ian.

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

Re: [PATCH OSSTEST 09/11] ts-examine-hostprops-save: introduce a script to save properties

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 09/11] ts-examine-hostprops-save: introduce a script to save properties"):
> The introduce script turns the properties stored in the runvars using
> the format hostprop_$hotname_$prop=$val into host properties stored in
> the database.
...
> +exit 0 if intended_blessing() ne "real";

Can we at least print a message in this case ?
TBH I think it might be better to actually fail.

> +logm("setting host properties");
> +
> +foreach my $k (sort keys %r) {
> +    next unless $k =~ m/^hostprop_([^_]*)_([^_]*)$/;
> +    my $hostname= $1;
> +    my $prop=$2;
> +
> +    logm("recording for $hostname $prop=$r{$k}");
> +    set_host_property($hostname, $prop, $r{$k});

My earlier comments mean you're going to have to call selecthost().

That means you are going to need to record the current hostname and
compare it to the one you have in $ho, or something.  (Otherwise
you'll call selecthost for every prop which would produce a lot of
undesirable noise.)

Ian.

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

Re: [PATCH OSSTEST 11/11] sg-run-job: hook the memdisk test into examine

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 11/11] sg-run-job: hook the memdisk test into examine"):
> Hook the memdisk parameter detection and the saving of the host
> properties into the examine jobs.

Looking at this:

> +	run-ts .   =            ts-memdisk-try-append + host
>      }
>      run-ts !broken capture-logs ts-logs-capture       + host

ts-memdisk-try-append is going to leave the host in a weird state.
But then ts-logs-capture will not work quite right (at the very least,
it will have to power cycle the host; but actually I suspect it will
try to extract logs from the freebsd installer environment).

So I think this is in the wrong place in the sequence.  Maybe it
should come before we install Linux ?  What do you think ?

Ian.

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

Re: [PATCH OSSTEST 10/11] make-hosts-flight: set runvars for FreeBSD test

[Ian Jackson]

Roger Pau Monne writes ("[PATCH OSSTEST 10/11] make-hosts-flight: set runvars for FreeBSD test"):
> This is needed in order to run the memdisk test.

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

Ian.

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

Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Roger Pau Monne]

On Fri, Jul 28, 2017 at 04:39:04PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> > This is from the code in mg-hosts. Switch cmd_setprops to use the
> > newly introduced function.
> 
> I think this needs to be abstracted through jobdb.  Certainly these
> SQL statements operating on the host_properties table are valid in
> Executive mode only and shouldn't appear in TestSupport.pm.
> 
> If you are lucky then mg-hosts runs with $mjobdb set.  In which case
> you make it call $mjobdb->set_host_prop.
> 
> I considered asking you to make a wrapper in TestSupport or
> Osstest.pm.  But: actually, I think the wrapper in TestSupport ought
> to take a $ho, not a hostname.  That would guarantee that a ts-*
> script which used it had called selecthost and had the host allocated.
> 
> Whereas mg-hosts setprops is exceptional: it wants to be able to set
> the property on a host without the user having allocated it.  I think
> that justifies the "back door" entry via $mjobdb.
> 
> What do you think ?

That sounds fine. I agree that the approach taken here is too drastic.
For once, the set_host_property should take a $ho, and then it should
hide all this in mhostdb (so that it works in standalone mode).

IMHO, I think the right approach is to leave mg-hosts as it is now,
and implement a set_property in HostDB/{Executive/Static}.pm and
implement a helper in TestSupport that makes use of it
($mhostdb->set_property(...)), do you agree?

Thanks, Roger.

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

Re: [PATCH OSSTEST 07/11] ts-freebsd-host-install: add option to test memdisk options

[Roger Pau Monne]

On Fri, Jul 28, 2017 at 04:45:20PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH OSSTEST 07/11] ts-freebsd-host-install: add option to test memdisk options"):
> > This is needed in order to figure out which memdisk options should be
> > used to boot the images on each specific box.
> ...
> > +if ($r{'arch'} !~ m/amd64/g) {
> > +    logm("Arch $r{'arch'} not supported!");
> 
> This clearly can't be right because presumably at least i386 would
> work too.  I don't know why you need this check.

Yes, we could test memdisk with i386 also, except that osstest doesn't
generate i386 images yet.

> > +    exit 0;
> 
> WTF ?  You don't want ts-freebsd-host-install to exit 0 if it gets an
> unknown architecture.
>
> ...  Oh I see.  You have misplaced this check, which should only be
> effective when we are testing boot arguments.

Yes, we cannot test this for ARM (or else it's going to fail and
block the other steps of the examine job), that's why it returns 0.

So I think this should indeed be conditional on bootonly being set.

Roger.

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

Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Ian Jackson]

Roger Pau Monne writes ("Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> IMHO, I think the right approach is to leave mg-hosts as it is now,

Yes.

> and implement a set_property in HostDB/{Executive/Static}.pm and
> implement a helper in TestSupport that makes use of it
> ($mhostdb->set_property(...)), do you agree?

TBH, since this is only being called in the one
ts-set-host-properties-from-runvars script (or whatever you're calling
it), I think you can use $mjobdb-> directly.  That's not too bad a
layer violation.

I think your runvars should probably be named after the ident, not the
hostname.  That may involve rethinking your encoding, since idents can
contain _ (hostnames can contain - but not _).

Ian.

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

Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Roger Pau Monne]

On Tue, Aug 01, 2017 at 02:01:35PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> > IMHO, I think the right approach is to leave mg-hosts as it is now,
> 
> Yes.
> 
> > and implement a set_property in HostDB/{Executive/Static}.pm and
> > implement a helper in TestSupport that makes use of it
> > ($mhostdb->set_property(...)), do you agree?
> 
> TBH, since this is only being called in the one
> ts-set-host-properties-from-runvars script (or whatever you're calling
> it), I think you can use $mjobdb-> directly.  That's not too bad a
> layer violation.

In the new version that I've sent I've already added a helper to
TestSupport, it's just two lines of code so unless you feel really
annoyed by it I would probably leave it there.

> I think your runvars should probably be named after the ident, not the
> hostname.  That may involve rethinking your encoding, since idents can
> contain _ (hostnames can contain - but not _).

Does it make sense to have the hostname or the ident?

After all ts-set-host-properties-from-runvars is only going to save
properties for the host passed as 'host' ident, and I don't see much
reason for allowing it to support two different idents like src_host
or dst_host, or in general for having a test script that sets host
properties for multiple hosts.

Roger.

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

Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Ian Jackson]

Roger Pau Monne writes ("Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> On Tue, Aug 01, 2017 at 02:01:35PM +0100, Ian Jackson wrote:
> > TBH, since this is only being called in the one
> > ts-set-host-properties-from-runvars script (or whatever you're calling
> > it), I think you can use $mjobdb-> directly.  That's not too bad a
> > layer violation.
> 
> In the new version that I've sent I've already added a helper to
> TestSupport, it's just two lines of code so unless you feel really
> annoyed by it I would probably leave it there.

No, sure, the helper is probably a bit better.

> > I think your runvars should probably be named after the ident, not the
> > hostname.  That may involve rethinking your encoding, since idents can
> > contain _ (hostnames can contain - but not _).
> 
> Does it make sense to have the hostname or the ident?
> 
> After all ts-set-host-properties-from-runvars is only going to save
> properties for the host passed as 'host' ident, and I don't see much
> reason for allowing it to support two different idents like src_host
> or dst_host, or in general for having a test script that sets host
> properties for multiple hosts.

I don't think this can be right.

You have a helper function that you pass a $ho, and which records the
runvars, and which therefore pretends to arrange for the properties to
be set (later), for that $ho.  It should do as it implicity promises.

That means the second actually-modifying-the-db ts-* script must
process all of the specified runvars.  So it must call selecthost.  So
really it should have the ident.  So the runvars should be named after
the ident.

Ian.

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

Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Roger Pau Monne]

On Tue, Aug 01, 2017 at 03:13:33PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> > On Tue, Aug 01, 2017 at 02:01:35PM +0100, Ian Jackson wrote:
> > > TBH, since this is only being called in the one
> > > ts-set-host-properties-from-runvars script (or whatever you're calling
> > > it), I think you can use $mjobdb-> directly.  That's not too bad a
> > > layer violation.
> > 
> > In the new version that I've sent I've already added a helper to
> > TestSupport, it's just two lines of code so unless you feel really
> > annoyed by it I would probably leave it there.
> 
> No, sure, the helper is probably a bit better.
> 
> > > I think your runvars should probably be named after the ident, not the
> > > hostname.  That may involve rethinking your encoding, since idents can
> > > contain _ (hostnames can contain - but not _).
> > 
> > Does it make sense to have the hostname or the ident?
> > 
> > After all ts-set-host-properties-from-runvars is only going to save
> > properties for the host passed as 'host' ident, and I don't see much
> > reason for allowing it to support two different idents like src_host
> > or dst_host, or in general for having a test script that sets host
> > properties for multiple hosts.
> 
> I don't think this can be right.
> 
> You have a helper function that you pass a $ho, and which records the
> runvars, and which therefore pretends to arrange for the properties to
> be set (later), for that $ho.  It should do as it implicity promises.
> 
> That means the second actually-modifying-the-db ts-* script must
> process all of the specified runvars.  So it must call selecthost.  So
> really it should have the ident.  So the runvars should be named after
> the ident.

OK, I was calling selecthost using ARGS[0] ATM in the ts-save-props...
script, and I was passing 'host' to it from sg-run-job.

I can change ts-save-props... to instead call selecthost on every
ident that's found in the hostprops-<ident>-<prop> runvar.

After reading README osstest doesn't seem to have any limitation on
the characters that can be used for host idents, would you be fine
with me modifying it to add that '-' cannot be used in host idents,
and then storing the putative host properties using the following
runvar format:

hostprops-<ident>-<prop>=<value>

Then I will remove the host parameter to the ts-save-props... script
and call selecthost on every ident found in the runvars. This is going
to generate some noise on the log I guess, but it should be fine.

Roger.

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

Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Ian Jackson]

Roger Pau Monne writes ("Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> After reading README osstest doesn't seem to have any limitation on
> the characters that can be used for host idents, would you be fine
> with me modifying it to add that '-' cannot be used in host idents,
> and then storing the putative host properties using the following
> runvar format:
> 
> hostprops-<ident>-<prop>=<value>

Existing runvars use `_' so we have the choice of using a new
structured name delimiter, or massaging the ident.  I think using a
new delimiter is fine, but we should plan to have one new delimiter
for all future uses.

If we are going to have runvars which have a different delimiter than
`_', I think we should probabaly not choose `-'; the chance is too
high that this will be used somewhere else.

Indeed, we currently already have runvars `diversehosts_xtf-x86'
and `one.guest.osstest_tcpcheckport' in the history.

I did this
   select * from runvars where name ~* '[^-._0-9a-z]';
and got hits only in play flights containing things which are
obviously garbage generated by broken flight construction tools.

So we can choose any delimter except those.  I suggest `/' ?
I think if we are trying to encode pathnames in runvar names,
we are doing something wrong.

> Then I will remove the host parameter to the ts-save-props... script
> and call selecthost on every ident found in the runvars. This is going
> to generate some noise on the log I guess, but it should be fine.

Yes.

Ian.

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

Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop

[Roger Pau Monne]

On Tue, Aug 01, 2017 at 04:07:18PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("Re: [PATCH OSSTEST 04/11] TestSupport: introduce set_host_prop"):
> > After reading README osstest doesn't seem to have any limitation on
> > the characters that can be used for host idents, would you be fine
> > with me modifying it to add that '-' cannot be used in host idents,
> > and then storing the putative host properties using the following
> > runvar format:
> > 
> > hostprops-<ident>-<prop>=<value>
> 
> Existing runvars use `_' so we have the choice of using a new
> structured name delimiter, or massaging the ident.  I think using a
> new delimiter is fine, but we should plan to have one new delimiter
> for all future uses.
> 
> If we are going to have runvars which have a different delimiter than
> `_', I think we should probabaly not choose `-'; the chance is too
> high that this will be used somewhere else.
> 
> Indeed, we currently already have runvars `diversehosts_xtf-x86'
> and `one.guest.osstest_tcpcheckport' in the history.
> 
> I did this
>    select * from runvars where name ~* '[^-._0-9a-z]';
> and got hits only in play flights containing things which are
> obviously garbage generated by broken flight construction tools.
> 
> So we can choose any delimter except those.  I suggest `/' ?
> I think if we are trying to encode pathnames in runvar names,
> we are doing something wrong.

That seems fine to me.

Thanks, Roger.

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