Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys
@ 2021-01-22 15:55 Ian Jackson
  2021-01-22 15:55 ` [OSSTEST PATCH 2/7] target_editfile_kvp_replace: Add a bit of logging Ian Jackson
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 15:55 UTC (permalink / raw)
  To: xen-devel; +Cc: 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OSSTEST PATCH 2/7] target_editfile_kvp_replace: Add a bit of logging
  2021-01-22 15:55 [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys Ian Jackson
@ 2021-01-22 15:55 ` Ian Jackson
  2021-01-22 15:55 ` [OSSTEST PATCH 3/7] ts-xen-install: Rename $commons_config_file Ian Jackson
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 15:55 UTC (permalink / raw)
  To: xen-devel; +Cc: 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OSSTEST PATCH 3/7] ts-xen-install: Rename $commons_config_file
  2021-01-22 15:55 [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys Ian Jackson
  2021-01-22 15:55 ` [OSSTEST PATCH 2/7] target_editfile_kvp_replace: Add a bit of logging Ian Jackson
@ 2021-01-22 15:55 ` Ian Jackson
  2021-01-22 15:56 ` [OSSTEST PATCH 4/7] ts-xen-install: Break out @commons_config Ian Jackson
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 15:55 UTC (permalink / raw)
  To: xen-devel; +Cc: 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OSSTEST PATCH 4/7] ts-xen-install: Break out @commons_config
  2021-01-22 15:55 [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys Ian Jackson
  2021-01-22 15:55 ` [OSSTEST PATCH 2/7] target_editfile_kvp_replace: Add a bit of logging Ian Jackson
  2021-01-22 15:55 ` [OSSTEST PATCH 3/7] ts-xen-install: Rename $commons_config_file Ian Jackson
@ 2021-01-22 15:56 ` Ian Jackson
  2021-01-22 15:56 ` [OSSTEST PATCH 5/7] ts-xen-install: Honour xenstored target var Ian Jackson
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 15:56 UTC (permalink / raw)
  To: xen-devel; +Cc: 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OSSTEST PATCH 5/7] ts-xen-install: Honour xenstored target var
  2021-01-22 15:55 [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys Ian Jackson
                   ` (2 preceding siblings ...)
  2021-01-22 15:56 ` [OSSTEST PATCH 4/7] ts-xen-install: Break out @commons_config Ian Jackson
@ 2021-01-22 15:56 ` Ian Jackson
  2021-01-22 15:56 ` [OSSTEST PATCH 6/7] mfi-common: Provide stripy_rand Ian Jackson
  2021-01-22 15:56 ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Ian Jackson
  5 siblings, 0 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 15:56 UTC (permalink / raw)
  To: xen-devel; +Cc: 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OSSTEST PATCH 6/7] mfi-common: Provide stripy_rand
  2021-01-22 15:55 [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys Ian Jackson
                   ` (3 preceding siblings ...)
  2021-01-22 15:56 ` [OSSTEST PATCH 5/7] ts-xen-install: Honour xenstored target var Ian Jackson
@ 2021-01-22 15:56 ` Ian Jackson
  2021-01-22 15:56 ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Ian Jackson
  5 siblings, 0 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 15:56 UTC (permalink / raw)
  To: xen-devel; +Cc: 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 15:55 [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys Ian Jackson
                   ` (4 preceding siblings ...)
  2021-01-22 15:56 ` [OSSTEST PATCH 6/7] mfi-common: Provide stripy_rand Ian Jackson
@ 2021-01-22 15:56 ` Ian Jackson
  2021-01-22 16:07   ` Andrew Cooper
                     ` (3 more replies)
  5 siblings, 4 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 15:56 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Jackson, Edwin Török, Christian Lindig,
	Andrew Cooper, Jürgen Groß,
	Wei Liu, 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 15:56 ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Ian Jackson
@ 2021-01-22 16:07   ` Andrew Cooper
  2021-01-22 16:22     ` Ian Jackson
  2021-01-22 16:11   ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Christian Lindig
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Andrew Cooper @ 2021-01-22 16:07 UTC (permalink / raw)
  To: Ian Jackson, xen-devel
  Cc: Edwin Török, Christian Lindig, Jürgen Groß,
	Wei Liu, Ian Jackson

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 15:56 ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Ian Jackson
  2021-01-22 16:07   ` Andrew Cooper
@ 2021-01-22 16:11   ` Christian Lindig
  2021-01-22 16:36   ` Andrew Cooper
  2021-01-22 16:40   ` Edwin Torok
  3 siblings, 0 replies; 19+ messages in thread
From: Christian Lindig @ 2021-01-22 16:11 UTC (permalink / raw)
  To: Ian Jackson, xen-devel
  Cc: Edwin Torok, Andrew Cooper, Jürgen Groß, Wei Liu, Ian Jackson

> 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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 16:07   ` Andrew Cooper
@ 2021-01-22 16:22     ` Ian Jackson
  2021-01-22 16:24       ` Jürgen Groß
  2021-01-22 16:29       ` Andrew Cooper
  0 siblings, 2 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 16:22 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: xen-devel, Edwin Török, Christian Lindig,
	Jürgen Groß,
	Wei Liu

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 16:22     ` Ian Jackson
@ 2021-01-22 16:24       ` Jürgen Groß
  2021-01-22 16:26         ` Ian Jackson
  2021-01-22 16:29       ` Andrew Cooper
  1 sibling, 1 reply; 19+ messages in thread
From: Jürgen Groß @ 2021-01-22 16:24 UTC (permalink / raw)
  To: Ian Jackson, Andrew Cooper
  Cc: xen-devel, Edwin Török, Christian Lindig, Wei Liu

[-- 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 --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 16:24       ` Jürgen Groß
@ 2021-01-22 16:26         ` Ian Jackson
  2021-01-22 16:29           ` Jürgen Groß
  0 siblings, 1 reply; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 16:26 UTC (permalink / raw)
  To: Jürgen Groß
  Cc: Andrew Cooper, xen-devel, Edwin Török,
	Christian Lindig, Wei Liu

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 16:26         ` Ian Jackson
@ 2021-01-22 16:29           ` Jürgen Groß
  0 siblings, 0 replies; 19+ messages in thread
From: Jürgen Groß @ 2021-01-22 16:29 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Andrew Cooper, xen-devel, Edwin Török,
	Christian Lindig, Wei Liu

[-- 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 --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 16:22     ` Ian Jackson
  2021-01-22 16:24       ` Jürgen Groß
@ 2021-01-22 16:29       ` Andrew Cooper
  2021-01-22 17:37         ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages] Ian Jackson
  1 sibling, 1 reply; 19+ messages in thread
From: Andrew Cooper @ 2021-01-22 16:29 UTC (permalink / raw)
  To: Ian Jackson
  Cc: xen-devel, Edwin Török, Christian Lindig,
	Jürgen Groß,
	Wei Liu

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 15:56 ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Ian Jackson
  2021-01-22 16:07   ` Andrew Cooper
  2021-01-22 16:11   ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Christian Lindig
@ 2021-01-22 16:36   ` Andrew Cooper
  2021-01-22 16:40   ` Edwin Torok
  3 siblings, 0 replies; 19+ messages in thread
From: Andrew Cooper @ 2021-01-22 16:36 UTC (permalink / raw)
  To: Ian Jackson, xen-devel
  Cc: Edwin Török, Christian Lindig, Jürgen Groß,
	Wei Liu, Ian Jackson

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored
  2021-01-22 15:56 ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Ian Jackson
                     ` (2 preceding siblings ...)
  2021-01-22 16:36   ` Andrew Cooper
@ 2021-01-22 16:40   ` Edwin Torok
  3 siblings, 0 replies; 19+ messages in thread
From: Edwin Torok @ 2021-01-22 16:40 UTC (permalink / raw)
  To: iwj, xen-devel; +Cc: wl, Ian Jackson, jgross, Christian Lindig, Andrew Cooper

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages]
  2021-01-22 16:29       ` Andrew Cooper
@ 2021-01-22 17:37         ` Ian Jackson
  2021-01-22 17:52           ` Andrew Cooper
  0 siblings, 1 reply; 19+ messages in thread
From: Ian Jackson @ 2021-01-22 17:37 UTC (permalink / raw)
  To: Edwin Torok, Andrew Cooper
  Cc: xen-devel, Christian Lindig, Jürgen Groß, Wei Liu

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages]
  2021-01-22 17:37         ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages] Ian Jackson
@ 2021-01-22 17:52           ` Andrew Cooper
  2021-01-28 14:26             ` Ian Jackson
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Cooper @ 2021-01-22 17:52 UTC (permalink / raw)
  To: Ian Jackson, Edwin Torok
  Cc: xen-devel, Christian Lindig, Jürgen Groß, Wei Liu

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages]
  2021-01-22 17:52           ` Andrew Cooper
@ 2021-01-28 14:26             ` Ian Jackson
  0 siblings, 0 replies; 19+ messages in thread
From: Ian Jackson @ 2021-01-28 14:26 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Edwin Torok, xen-devel, Christian Lindig, Jürgen Groß, Wei Liu

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-22 15:55 [OSSTEST PATCH 1/7] target_editfile_kvp_replace: Support changing multiple keys Ian Jackson
2021-01-22 15:55 ` [OSSTEST PATCH 2/7] target_editfile_kvp_replace: Add a bit of logging Ian Jackson
2021-01-22 15:55 ` [OSSTEST PATCH 3/7] ts-xen-install: Rename $commons_config_file Ian Jackson
2021-01-22 15:56 ` [OSSTEST PATCH 4/7] ts-xen-install: Break out @commons_config Ian Jackson
2021-01-22 15:56 ` [OSSTEST PATCH 5/7] ts-xen-install: Honour xenstored target var Ian Jackson
2021-01-22 15:56 ` [OSSTEST PATCH 6/7] mfi-common: Provide stripy_rand Ian Jackson
2021-01-22 15:56 ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Ian Jackson
2021-01-22 16:07   ` Andrew Cooper
2021-01-22 16:22     ` Ian Jackson
2021-01-22 16:24       ` Jürgen Groß
2021-01-22 16:26         ` Ian Jackson
2021-01-22 16:29           ` Jürgen Groß
2021-01-22 16:29       ` Andrew Cooper
2021-01-22 17:37         ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored [and 2 more messages] Ian Jackson
2021-01-22 17:52           ` Andrew Cooper
2021-01-28 14:26             ` Ian Jackson
2021-01-22 16:11   ` [OSSTEST PATCH 7/7] make-flight: Stripy xenstored Christian Lindig
2021-01-22 16:36   ` Andrew Cooper
2021-01-22 16:40   ` Edwin Torok

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git