diff mbox series

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

Message ID 20210122155603.23402-7-iwj@xenproject.org (mailing list archive)
State New
Headers show
Series [OSSTEST,1/7] target_editfile_kvp_replace: Support changing multiple keys | expand

Commit Message

Ian Jackson Jan. 22, 2021, 3:56 p.m. UTC
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(-)

Comments

Andrew Cooper Jan. 22, 2021, 4:07 p.m. UTC | #1
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
Christian Lindig Jan. 22, 2021, 4:11 p.m. UTC | #2
> Change this, by selecting between C xenstored and Ocaml xenstored
"at random"

Acked-by: Christian Lindig <christian.lindig@citrix.com>
Ian Jackson Jan. 22, 2021, 4:22 p.m. UTC | #3
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.
Jürgen Groß Jan. 22, 2021, 4:24 p.m. UTC | #4
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
Ian Jackson Jan. 22, 2021, 4:26 p.m. UTC | #5
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.
Jürgen Groß Jan. 22, 2021, 4:29 p.m. UTC | #6
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
Andrew Cooper Jan. 22, 2021, 4:29 p.m. UTC | #7
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
Andrew Cooper Jan. 22, 2021, 4:36 p.m. UTC | #8
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
Edwin Torok Jan. 22, 2021, 4:40 p.m. UTC | #9
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
Ian Jackson Jan. 22, 2021, 5:37 p.m. UTC | #10
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.
Andrew Cooper Jan. 22, 2021, 5:52 p.m. UTC | #11
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
Ian Jackson Jan. 28, 2021, 2:26 p.m. UTC | #12
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.
diff mbox series

Patch

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 () {