| Message ID | 20210122155603.23402-7-iwj@xenproject.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [OSSTEST,1/7] target_editfile_kvp_replace: Support changing multiple keys | expand |
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
> Change this, by selecting between C xenstored and Ocaml xenstored "at random" Acked-by: Christian Lindig <christian.lindig@citrix.com>
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.
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
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.
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
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
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
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
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.
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
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 --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 () {
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(-)