[OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys

[OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys

[Ian Jackson]

No functional change with existing callers.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
---
 Osstest/TestSupport.pm | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 9362a865..d2558f31 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -769,25 +769,32 @@ sub teditfileex {
 	if $install;
 }
 
-# Replace a Key=Value style line in a config file.
+# Replace Key=Value style line(s) in a config file.
 #
 # To be used as 3rd argument to target_editfile(_root) as:
 #    target_editfile_root($ho, "/path/to/a/file",
 #			 sub { target_editfile_kvp_replace($key, $value) });
-sub target_editfile_kvp_replace ($$)
+sub target_editfile_kvp_replace
 {
-    my ($key,$value) = @_;
-    my $prnow;
-    $prnow= sub {
+    my (%kv) = @_;
+    my $prnow= sub {
+	my ($key) = @_;
+	my $value = $kv{$key};
+	return unless defined $value;
 	print ::EO "$key=$value\n" or die $!;
-	$prnow= sub { };
+	delete $kv{$key};
     };
     while (<::EI>) {
-	print ::EO or die $! unless m/^$key\b/;
-	$prnow->() if m/^#$key/;
+	if (m/^\S+\b/ && exists $kv{$&}) {
+	    $prnow->($&);
+	} else {
+	    print ::EO or die $!;
+	}
     }
     print ::EO "\n" or die $!;
-    $prnow->();
+    foreach my $key (sort keys %kv) {
+	$prnow->($key);
+    }
 };
 
 sub target_editfile_root ($$$;$$) { teditfileex('root',@_); }
-- 
2.20.1

[OSSTEST PATCH 2/7] target_editfile_kvp_replace: Add a bit of logging

[Ian Jackson]

This helps see what's going on without fishing individual
before-and-after files out of the log directory.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
---
 Osstest/TestSupport.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index d2558f31..9e85303a 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -777,6 +777,7 @@ sub teditfileex {
 sub target_editfile_kvp_replace
 {
     my (%kv) = @_;
+    logm("substituing: @_");
     my $prnow= sub {
 	my ($key) = @_;
 	my $value = $kv{$key};
-- 
2.20.1

[OSSTEST PATCH 3/7] ts-xen-install: Rename $commons_config_file

[Ian Jackson]

This is the shell script config for xencommons.  We are going to set
other things here too.

No functional change.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
---
 ts-xen-install | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ts-xen-install b/ts-xen-install
index 5d4f8b0d..feb98951 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -127,18 +127,18 @@ sub adjustconfig () {
 	print EO $extra or die $!;
     }) if toolstack($ho)->{Name} eq "xend";
 
-    my $trace_config_file;
+    my $commons_config_file;
     foreach my $try (qw(/etc/default/xencommons
                         /etc/sysconfig/xencommons
                         /etc/default/xend
                         /etc/sysconfig/xend)) {
         next unless target_file_exists($ho, $try);
-        $trace_config_file= $try;
+        $commons_config_file= $try;
         last;
     }
-    die unless defined $trace_config_file;
+    die unless defined $commons_config_file;
 
-    target_editfile_root($ho, $trace_config_file,
+    target_editfile_root($ho, $commons_config_file,
 	sub { target_editfile_kvp_replace("XENCONSOLED_TRACE", "guest") });
 
     target_editfile_root($ho, '/etc/libvirt/libvirtd.conf',
-- 
2.20.1

[OSSTEST PATCH 4/7] ts-xen-install: Break out @commons_config

[Ian Jackson]

We are going to set other things here too.  Prepare for that.

No functional change.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
---
 ts-xen-install | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ts-xen-install b/ts-xen-install
index feb98951..fc4bf423 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -138,8 +138,12 @@ sub adjustconfig () {
     }
     die unless defined $commons_config_file;
 
+    my @commons_config =
+        (
+	   "XENCONSOLED_TRACE" => "guest",
+	);
     target_editfile_root($ho, $commons_config_file,
-	sub { target_editfile_kvp_replace("XENCONSOLED_TRACE", "guest") });
+	sub { target_editfile_kvp_replace(@commons_config) });
 
     target_editfile_root($ho, '/etc/libvirt/libvirtd.conf',
 		sub { target_editfile_kvp_replace("log_level", "1") })
-- 
2.20.1

[OSSTEST PATCH 5/7] ts-xen-install: Honour xenstored target var

[Ian Jackson]

Nothing sets this yet.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
---
 Osstest/TestSupport.pm | 1 +
 ts-xen-install         | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 9e85303a..a0ca6943 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -179,6 +179,7 @@ our @accessible_runvar_pats =
       host_linux_boot_append         *_host_linux_boot_append
       host_ip                        *_host_ip
       host_power_install             *_host_power_install
+      host_xenstored                 *_host_xenstored
    );
 
 #---------- test script startup ----------
diff --git a/ts-xen-install b/ts-xen-install
index fc4bf423..47865eb6 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -142,6 +142,13 @@ sub adjustconfig () {
         (
 	   "XENCONSOLED_TRACE" => "guest",
 	);
+
+    my $xenstored = target_var($ho, 'xenstored');
+    if (defined $xenstored) {
+	$xenstored = "/usr/local/sbin/$xenstored" unless $xenstored =~ m{/};
+	push @commons_config, "XENSTORED", $xenstored;
+    }
+
     target_editfile_root($ho, $commons_config_file,
 	sub { target_editfile_kvp_replace(@commons_config) });
 
-- 
2.20.1

[OSSTEST PATCH 6/7] mfi-common: Provide stripy_rand

[Ian Jackson]

We will use this in a moment.

Signed-off-by: Ian Jackson <iwj@xenproject.org>
---
 mfi-common | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mfi-common b/mfi-common
index 34b0c116..35efd233 100644
--- a/mfi-common
+++ b/mfi-common
@@ -31,6 +31,18 @@ stripy () {
   eval "$out_vn=\"\$out_$out_val\""
 }
 
+stripy_rand () {
+  # feel free to pass not-real values for $job
+  # if desired to perturb the hash, etc.
+  local job="$1"; shift
+  local out_vn="$1"; shift
+  local hash="$( echo "$job $out_vn" | sha256sum )"
+  hash="${hash:0:7}"
+  local ix=$(( (0x$hash * $#) / 0x10000000 + 1 ))
+  out_val="${@:$ix:1}"
+  eval "$out_vn=\"\$out_val\""
+}
+
 branch_wants_migrupgrade_tests () {
   case "$branch" in
     xen-3.*-testing) return 1 ;;
-- 
2.20.1

[OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Ian Jackson]

Previously, we let the Xen build system and startup scripts choose
which xenstored to use.  Before we upgraded to Debian buster, that
gave us C xentored tests on ARM.  Since then, armhf and arm64 have
both had enough ocaml support and we haven't been testing C xenstored
at all !

Change this, by selecting between C xenstored and Ocaml xenstored
"at random".  Actually, this is based on the job name.  So the same
job in different branches will use the same xenstored - which helps
avoid confusion.

I have diffed the output of standalone-generate-dump-flight-runvars.
As expected, this addes a variable all_host_xenstored to every job.

To make sure we have enough diversity, I eyeballed the results.  In
particular:
  * The smoke tests now have 2 C and 2 Ocaml, one of each on
    ARM and x86.
  * XTF tests have 2 oxenstored and 3 C xenstored.
  * The ovmf flight has one of each
  * The seabios and libvirt flights look reasonably mixed.

Most other flights have enough jobs that I think things are diverse
enough without looking at them all in detail.

I think this lack of testing needs fixing for the Xen 4.15 release.
So after review I intend to push this to osstest pretest, and may
force push it even if shows regressions.

CC: Edwin Török <edvin.torok@citrix.com>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Andrew Cooper <Andrew.Cooper3@citrix.com>
CC: Jürgen Groß <jgross@suse.com>
CC: Wei Liu <wl@xen.org>
Signed-off-by: Ian Jackson <iwj@xenproject.org>
Release-Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 mfi-common | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/mfi-common b/mfi-common
index 35efd233..2834411f 100644
--- a/mfi-common
+++ b/mfi-common
@@ -509,6 +509,13 @@ job_create_test () {
   xenbuildjob="${bfi}build-$xenarch$xsm_suffix"
   buildjob="${bfi}build-$dom0arch$xsm_suffix"
 
+  local xenstored="$xenstored"
+  if [ "$xenstored" = "" ]; then
+    stripy_rand "$job 2" xenstored  xenstored oxenstored
+    # Without " <n>", all XTF jobs use oxenstored
+    # With " 1", All OVMF tests use xenstored
+  fi
+
   job_create_test_filter_callback \
     "$job" "$recipe" "$toolstack" "$xenarch" "$dom0arch" "$@" || return 0
 
@@ -529,7 +536,8 @@ job_create_test () {
 
   ./cs-job-create $flight $job $recipe toolstack=$toolstack       \
     $RUNVARS $TEST_RUNVARS $global_runvars $most_runvars          \
-    xenbuildjob=$xenbuildjob buildjob=$buildjob $tsbuildjob "$@"
+    xenbuildjob=$xenbuildjob buildjob=$buildjob                   \
+    all_host_xenstored=$xenstored $tsbuildjob "$@"
 }
 
 usual_debianhvm_image () {
-- 
2.20.1

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Andrew Cooper]

On 22/01/2021 15:56, Ian Jackson wrote:
> Previously, we let the Xen build system and startup scripts choose
> which xenstored to use.  Before we upgraded to Debian buster, that
> gave us C xentored tests on ARM.  Since then, armhf and arm64 have
> both had enough ocaml support and we haven't been testing C xenstored
> at all !
>
> Change this, by selecting between C xenstored and Ocaml xenstored
> "at random".  Actually, this is based on the job name.  So the same
> job in different branches will use the same xenstored - which helps
> avoid confusion.
>
> I have diffed the output of standalone-generate-dump-flight-runvars.
> As expected, this addes a variable all_host_xenstored to every job.
>
> To make sure we have enough diversity, I eyeballed the results.  In
> particular:
>   * The smoke tests now have 2 C and 2 Ocaml, one of each on
>     ARM and x86.
>   * XTF tests have 2 oxenstored and 3 C xenstored.
>   * The ovmf flight has one of each
>   * The seabios and libvirt flights look reasonably mixed.
>
> Most other flights have enough jobs that I think things are diverse
> enough without looking at them all in detail.
>
> I think this lack of testing needs fixing for the Xen 4.15 release.
> So after review I intend to push this to osstest pretest, and may
> force push it even if shows regressions.

Sounds good.

A couple of quick questions/observations.  Does this cope in a sensible
way if, for whatever reason, the chosen daemon isn't present?

How hard would it be to add the 3rd option, stub-cxenstored into this
mix?  It is just one other key in xencommons to tweak.

SUPPORT.md doesn't appear to make any statements about the disposition
of xenstoreds, but stub-cxenstored is used by at least two major
downstreams so is obviously has security support in practice, and ought
to be tested.

~Andrew

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Christian Lindig]

> Change this, by selecting between C xenstored and Ocaml xenstored
"at random"

Acked-by: Christian Lindig <christian.lindig@citrix.com>

________________________________________
From: Ian Jackson <iwj@xenproject.org>
Sent: 22 January 2021 15:56
To: xen-devel@lists.xenproject.org
Cc: Ian Jackson; Edwin Torok; Christian Lindig; Andrew Cooper; Jürgen Groß; Wei Liu; Ian Jackson
Subject: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

Previously, we let the Xen build system and startup scripts choose
which xenstored to use.  Before we upgraded to Debian buster, that
gave us C xentored tests on ARM.  Since then, armhf and arm64 have
both had enough ocaml support and we haven't been testing C xenstored
at all !

Change this, by selecting between C xenstored and Ocaml xenstored
"at random".  Actually, this is based on the job name.  So the same
job in different branches will use the same xenstored - which helps
avoid confusion.

I have diffed the output of standalone-generate-dump-flight-runvars.
As expected, this addes a variable all_host_xenstored to every job.

To make sure we have enough diversity, I eyeballed the results.  In
particular:
  * The smoke tests now have 2 C and 2 Ocaml, one of each on
    ARM and x86.
  * XTF tests have 2 oxenstored and 3 C xenstored.
  * The ovmf flight has one of each
  * The seabios and libvirt flights look reasonably mixed.

Most other flights have enough jobs that I think things are diverse
enough without looking at them all in detail.

I think this lack of testing needs fixing for the Xen 4.15 release.
So after review I intend to push this to osstest pretest, and may
force push it even if shows regressions.

CC: Edwin Török <edvin.torok@citrix.com>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Andrew Cooper <Andrew.Cooper3@citrix.com>
CC: Jürgen Groß <jgross@suse.com>
CC: Wei Liu <wl@xen.org>
Signed-off-by: Ian Jackson <iwj@xenproject.org>
Release-Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 mfi-common | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/mfi-common b/mfi-common
index 35efd233..2834411f 100644
--- a/mfi-common
+++ b/mfi-common
@@ -509,6 +509,13 @@ job_create_test () {
   xenbuildjob="${bfi}build-$xenarch$xsm_suffix"
   buildjob="${bfi}build-$dom0arch$xsm_suffix"

+  local xenstored="$xenstored"
+  if [ "$xenstored" = "" ]; then
+    stripy_rand "$job 2" xenstored  xenstored oxenstored
+    # Without " <n>", all XTF jobs use oxenstored
+    # With " 1", All OVMF tests use xenstored
+  fi
+
   job_create_test_filter_callback \
     "$job" "$recipe" "$toolstack" "$xenarch" "$dom0arch" "$@" || return 0

@@ -529,7 +536,8 @@ job_create_test () {

   ./cs-job-create $flight $job $recipe toolstack=$toolstack       \
     $RUNVARS $TEST_RUNVARS $global_runvars $most_runvars          \
-    xenbuildjob=$xenbuildjob buildjob=$buildjob $tsbuildjob "$@"
+    xenbuildjob=$xenbuildjob buildjob=$buildjob                   \
+    all_host_xenstored=$xenstored $tsbuildjob "$@"
 }

 usual_debianhvm_image () {
--
2.20.1

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Ian Jackson]

Andrew Cooper writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
> A couple of quick questions/observations.  Does this cope in a sensible
> way if, for whatever reason, the chosen daemon isn't present?

That would depend on what you mean by "sensible".  I think that given
that we now think we support both on all architectures, "sensible"
means "the tests fail if one of the xenstoreds doesn't build".  And

that's what this will do :-).

> How hard would it be to add the 3rd option, stub-cxenstored into this
> mix?  It is just one other key in xencommons to tweak.

We would presumably want to do that for a smaller set of tests, but
yes, that could be done as a future enhancement.

> SUPPORT.md doesn't appear to make any statements about the disposition
> of xenstoreds, but stub-cxenstored is used by at least two major
> downstreams so is obviously has security support in practice, and ought
> to be tested.

Looking at /etc/default/xencommons, I think that testing would be done
by setting XENSTORETYPE=domain.  Do we want to test stub C xentored or
stub ocaml xenstored or both ?  The config seems not to have a way to
specify which.  Do we build only one ?

Ian.

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1357 bytes --]

On 22.01.21 17:22, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
>> A couple of quick questions/observations.  Does this cope in a sensible
>> way if, for whatever reason, the chosen daemon isn't present?
> 
> That would depend on what you mean by "sensible".  I think that given
> that we now think we support both on all architectures, "sensible"
> means "the tests fail if one of the xenstoreds doesn't build".  And
> 
> that's what this will do :-).
> 
>> How hard would it be to add the 3rd option, stub-cxenstored into this
>> mix?  It is just one other key in xencommons to tweak.
> 
> We would presumably want to do that for a smaller set of tests, but
> yes, that could be done as a future enhancement.
> 
>> SUPPORT.md doesn't appear to make any statements about the disposition
>> of xenstoreds, but stub-cxenstored is used by at least two major
>> downstreams so is obviously has security support in practice, and ought
>> to be tested.
> 
> Looking at /etc/default/xencommons, I think that testing would be done
> by setting XENSTORETYPE=domain.  Do we want to test stub C xentored or
> stub ocaml xenstored or both ?  The config seems not to have a way to
> specify which.  Do we build only one ?

There is only stub C xenstored in our build.


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Ian Jackson]

Jürgen Groß writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
> There is only stub C xenstored in our build.

I should have looked myself, shouldn't I:

-rw-r--r-- 1 osstest osstest  233391 Jan 21 22:14 xenstorepvh-stubdom.gz
-rw-r--r-- 1 osstest osstest  232653 Jan 21 22:14 xenstore-stubdom.gz
 
Oh so many options!

Ian.

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 513 bytes --]

On 22.01.21 17:26, Ian Jackson wrote:
> Jürgen Groß writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
>> There is only stub C xenstored in our build.
> 
> I should have looked myself, shouldn't I:
> 
> -rw-r--r-- 1 osstest osstest  233391 Jan 21 22:14 xenstorepvh-stubdom.gz
> -rw-r--r-- 1 osstest osstest  232653 Jan 21 22:14 xenstore-stubdom.gz
>   
> Oh so many options!

xenstorepvh-stubdom.gz isn't functional yet, but I hope to change that
in the 4.16 timeframe.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Andrew Cooper]

On 22/01/2021 16:22, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
>> A couple of quick questions/observations.  Does this cope in a sensible
>> way if, for whatever reason, the chosen daemon isn't present?
> That would depend on what you mean by "sensible".  I think that given
> that we now think we support both on all architectures, "sensible"
> means "the tests fail if one of the xenstoreds doesn't build".  And
>
> that's what this will do :-).

Right, but nothing will actually fail the build.

So the way this error will manifest is the first non-trivial `xl $FOO`
executed in dom0 hanging until the job timeout.

Or does OSSTest have an explicit "is xenstored running" check after
boot, before any further testing occurs?

>> How hard would it be to add the 3rd option, stub-cxenstored into this
>> mix?  It is just one other key in xencommons to tweak.
> We would presumably want to do that for a smaller set of tests, but
> yes, that could be done as a future enhancement.
>
>> SUPPORT.md doesn't appear to make any statements about the disposition
>> of xenstoreds, but stub-cxenstored is used by at least two major
>> downstreams so is obviously has security support in practice, and ought
>> to be tested.
> Looking at /etc/default/xencommons, I think that testing would be done
> by setting XENSTORETYPE=domain.  Do we want to test stub C xentored or
> stub ocaml xenstored or both ?  The config seems not to have a way to
> specify which.  Do we build only one ?

There is no such thing as an ocaml stub-xenstored yet, but I have asked
the Mirage folk if they'd like to remedy this.

~Andrew

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Andrew Cooper]

On 22/01/2021 15:56, Ian Jackson wrote:
> diff --git a/mfi-common b/mfi-common
> index 35efd233..2834411f 100644
> --- a/mfi-common
> +++ b/mfi-common
> @@ -509,6 +509,13 @@ job_create_test () {
>    xenbuildjob="${bfi}build-$xenarch$xsm_suffix"
>    buildjob="${bfi}build-$dom0arch$xsm_suffix"
>  
> +  local xenstored="$xenstored"
> +  if [ "$xenstored" = "" ]; then
> +    stripy_rand "$job 2" xenstored  xenstored oxenstored
> +    # Without " <n>", all XTF jobs use oxenstored
> +    # With " 1", All OVMF tests use xenstored
> +  fi

An extra thought.  What exactly feeds into the decision?

If it includes the flight number, then the retest logic is going to get
very confused on xenstored bugs when the implementation change between
the two runs.

Also, what is the bisector going across this changeset?

~Andrew

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored

[Edwin Torok]

On Fri, 2021-01-22 at 15:56 +0000, Ian Jackson wrote:
> Previously, we let the Xen build system and startup scripts choose
> which xenstored to use.  Before we upgraded to Debian buster, that
> gave us C xentored tests on ARM.  Since then, armhf and arm64 have
> both had enough ocaml support and we haven't been testing C xenstored
> at all !
> 
> Change this, by selecting between C xenstored and Ocaml xenstored
> "at random".  Actually, this is based on the job name.  So the same
> job in different branches will use the same xenstored - which helps
> avoid confusion.
> 
> I have diffed the output of standalone-generate-dump-flight-runvars.
> As expected, this addes a variable all_host_xenstored to every job.
> 
> To make sure we have enough diversity, I eyeballed the results.  In
> particular:
>   * The smoke tests now have 2 C and 2 Ocaml, one of each on
>     ARM and x86.
>   * XTF tests have 2 oxenstored and 3 C xenstored.
>   * The ovmf flight has one of each
>   * The seabios and libvirt flights look reasonably mixed.
> 
> Most other flights have enough jobs that I think things are diverse
> enough without looking at them all in detail.
> 
> I think this lack of testing needs fixing for the Xen 4.15 release.
> So after review I intend to push this to osstest pretest, and may
> force push it even if shows regressions.

Sounds good.

In the patch series that I've recently posted to xen-devel there is
also a 'make check' target in tools/ocaml/xenstored.

There is a bit of plumbing to do before that is ready to be run
automatically (there is a Dockerfile in the patchseries, so it should
be possible to either adapt that, or vendor the 3rdparty dependencies
for the purposes of osstest, whichever is preferable).

These unit tests require OCaml 4.06+, so it'd be good to ensure that at
least one of the builders has that (the Ubuntu:focal from my patch
series should).

Best regards,
--Edwin

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages]

[Ian Jackson]

Andrew Cooper writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
> Right, but nothing will actually fail the build.

Indeed.

> So the way this error will manifest is the first non-trivial `xl $FOO`
> executed in dom0 hanging until the job timeout.

I doubt it would produce a timeout BICBW.

> Or does OSSTest have an explicit "is xenstored running" check after
> boot, before any further testing occurs?

No.

If this turns out to ever happen we can improve the pre-checking.  In
general I let the chips fall where they may, for test failures, and
improve the checking/logging later.  Otherwise adding new tests
becomes very time-consuming (and there is also the risk that added
checks do not align with actual behvaiour).

Now that it's builing, I think it's fairly unlikely that we will
accidentally stop building one of the xenstoreds.

> There is no such thing as an ocaml stub-xenstored yet, but I have asked
> the Mirage folk if they'd like to remedy this.

Cool.


Andrew Cooper writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
> An extra thought.  What exactly feeds into the decision?

Precisely, the job name.

> If it includes the flight number, then the retest logic is going to get
> very confused on xenstored bugs when the implementation change between
> the two runs.

Indeed.  But it doesn't :-).

> Also, what is the bisector going across this changeset?

The bisector always runs with the latest osstest.  So if this new C
xenstore testing discovers that it's broken, the bisector won't be
much help.  (It will fail to repro the basis pass; so in any case we
won't get false reports from it.)

On the other hand, if C xenstored still works and some future point we
break it, the bisection will DTRT.


Edwin Torok writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
> In the patch series that I've recently posted to xen-devel there is
> also a 'make check' target in tools/ocaml/xenstored.

Cool.

Thanks,
Ian.

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages]

[Andrew Cooper]

On 22/01/2021 17:37, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored"):
>> Or does OSSTest have an explicit "is xenstored running" check after
>> boot, before any further testing occurs?
> No.
>
> If this turns out to ever happen we can improve the pre-checking.  In
> general I let the chips fall where they may, for test failures, and
> improve the checking/logging later.  Otherwise adding new tests
> becomes very time-consuming (and there is also the risk that added
> checks do not align with actual behvaiour).

I'm not objecting to this going in as-is, but I want to at least ensure
we're not heading in blind.

In practice, a lot of what I'm trying to achieve with some of the
extended commentary on the autotests thread is better pre-checking of
this kind of form, although admittedly at a rather lower level than "is
xenstored running".

>
> Now that it's builing, I think it's fairly unlikely that we will
> accidentally stop building one of the xenstoreds.

Well - I ask specifically because there is a thread on xen-devel about
upping the minimum supported version of Ocaml, in order to simplify a
couple of aspects.

This would manifest as oxenstored no longer building on older distros. 
(I've got no idea if the specific suggestion would impact OSSTest this
time.)

For now - lets just fix our testing gap.

~Andrew

Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages]

[Ian Jackson]

Andrew Cooper writes ("Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages]"):
> Well - I ask specifically because there is a thread on xen-devel about
> upping the minimum supported version of Ocaml, in order to simplify a
> couple of aspects.
> 
> This would manifest as oxenstored no longer building on older distros. 
> (I've got no idea if the specific suggestion would impact OSSTest this
> time.)

I wasn't aware of that.  So, noted, thanks.

> For now - lets just fix our testing gap.

This series is in osstest production now and happily did not reveal
any previously-masked problems with C xenstored!

Ian.