[Qemu-devel] [PATCH v5 0/5] iotests: use python logging

[Qemu-devel] [PATCH v5 0/5] iotests: use python logging

[John Snow]

This series uses python logging to enable output conditionally on
iotests.log(). We unify an initialization call (which also enables
debugging output for those tests with -d) and then make the switch
inside of iotests.

It will help alleviate the need to create logged/unlogged versions
of all the various helpers we have made.

V5:
 - Rebased again
 - Allow Python tests to run on any platform

V4:
 - Rebased on top of kwolf/block at the behest of mreitz

V3:
 - Rebased for 4.1+; now based on main branch.

V2:
 - Added all of the other python tests I missed to use script_initialize
 - Refactored the common setup as per Ehabkost's suggestion
 - Added protocol arguments to common initialization,
   but this isn't strictly required.

John Snow (5):
  iotests: remove 'linux' from default supported platforms
  iotests: add script_initialize
  iotest 258: use script_main
  iotests: specify protocol support via initialization info
  iotests: use python logging for iotests.log()

 tests/qemu-iotests/030        |   4 +-
 tests/qemu-iotests/149        |   3 +-
 tests/qemu-iotests/194        |   3 +-
 tests/qemu-iotests/202        |   3 +-
 tests/qemu-iotests/203        |   3 +-
 tests/qemu-iotests/206        |   2 +-
 tests/qemu-iotests/207        |   4 +-
 tests/qemu-iotests/208        |   2 +-
 tests/qemu-iotests/209        |   2 +-
 tests/qemu-iotests/210        |   4 +-
 tests/qemu-iotests/211        |   4 +-
 tests/qemu-iotests/212        |   4 +-
 tests/qemu-iotests/213        |   4 +-
 tests/qemu-iotests/216        |   3 +-
 tests/qemu-iotests/218        |   2 +-
 tests/qemu-iotests/219        |   2 +-
 tests/qemu-iotests/222        |   5 +-
 tests/qemu-iotests/224        |   3 +-
 tests/qemu-iotests/228        |   3 +-
 tests/qemu-iotests/234        |   3 +-
 tests/qemu-iotests/235        |   4 +-
 tests/qemu-iotests/236        |   2 +-
 tests/qemu-iotests/237        |   2 +-
 tests/qemu-iotests/238        |   2 +
 tests/qemu-iotests/242        |   2 +-
 tests/qemu-iotests/245        |   1 +
 tests/qemu-iotests/245.out    |  24 +++----
 tests/qemu-iotests/246        |   2 +-
 tests/qemu-iotests/248        |   2 +-
 tests/qemu-iotests/254        |   2 +-
 tests/qemu-iotests/255        |   2 +-
 tests/qemu-iotests/256        |   2 +-
 tests/qemu-iotests/258        |   8 +--
 tests/qemu-iotests/262        |   3 +-
 tests/qemu-iotests/iotests.py | 123 +++++++++++++++++++++-------------
 35 files changed, 133 insertions(+), 111 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[John Snow]

verify_platform will check an explicit whitelist and blacklist instead.
The default will now be assumed to be allowed to run anywhere.

For tests that do not specify their platforms explicitly, this has the effect of
enabling these tests on non-linux platforms. For tests that always specified
linux explicitly, there is no change.

For Python tests on FreeBSD at least; only seven python tests fail:
045 147 149 169 194 199 211

045 and 149 appear to be misconfigurations,
147 and 194 are the AF_UNIX path too long error,
169 and 199 are bitmap migration bugs, and
211 is a bug that shows up on Linux platforms, too.

This is at least good evidence that these tests are not Linux-only. If
they aren't suitable for other platforms, they should be disabled on a
per-platform basis as appropriate.

Therefore, let's switch these on and deal with the failures.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b26271187c..3c8c121fd5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -844,9 +844,14 @@ def verify_protocol(supported=[], unsupported=[]):
     if not_sup or (imgproto in unsupported):
         notrun('not suitable for this protocol: %s' % imgproto)
 
-def verify_platform(supported_oses=['linux']):
-    if True not in [sys.platform.startswith(x) for x in supported_oses]:
-        notrun('not suitable for this OS: %s' % sys.platform)
+def verify_platform(supported=None, unsupported=None):
+    if unsupported is not None:
+        if any((sys.platform.startswith(x) for x in unsupported)):
+            notrun('not suitable for this OS: %s' % sys.platform)
+
+    if supported is not None:
+        if not any((sys.platform.startswith(x) for x in supported)):
+            notrun('not suitable for this OS: %s' % sys.platform)
 
 def verify_cache_mode(supported_cache_modes=[]):
     if supported_cache_modes and (cachemode not in supported_cache_modes):
@@ -908,7 +913,8 @@ def execute_unittest(output, verbosity, debug):
                                     r'Ran \1 tests', output.getvalue()))
 
 def execute_test(test_function=None,
-                 supported_fmts=[], supported_oses=['linux'],
+                 supported_fmts=[],
+                 supported_platforms=None,
                  supported_cache_modes=[], unsupported_fmts=[],
                  supported_protocols=[], unsupported_protocols=[]):
     """Run either unittest or script-style tests."""
@@ -925,7 +931,7 @@ def execute_test(test_function=None,
     verbosity = 1
     verify_image_format(supported_fmts, unsupported_fmts)
     verify_protocol(supported_protocols, unsupported_protocols)
-    verify_platform(supported_oses)
+    verify_platform(supported=supported_platforms)
     verify_cache_mode(supported_cache_modes)
 
     if debug:
-- 
2.21.0

Re: [Qemu-devel] [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Vladimir Sementsov-Ogievskiy]

18.09.2019 2:45, John Snow wrote:
> verify_platform will check an explicit whitelist and blacklist instead.
> The default will now be assumed to be allowed to run anywhere.
> 
> For tests that do not specify their platforms explicitly, this has the effect of
> enabling these tests on non-linux platforms. For tests that always specified
> linux explicitly, there is no change.
> 
> For Python tests on FreeBSD at least; only seven python tests fail:
> 045 147 149 169 194 199 211
> 
> 045 and 149 appear to be misconfigurations,
> 147 and 194 are the AF_UNIX path too long error,
> 169 and 199 are bitmap migration bugs, and
> 211 is a bug that shows up on Linux platforms, too.
> 
> This is at least good evidence that these tests are not Linux-only. If
> they aren't suitable for other platforms, they should be disabled on a
> per-platform basis as appropriate.
> 
> Therefore, let's switch these on and deal with the failures.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1753 bytes --]

On 18.09.19 01:45, John Snow wrote:
> verify_platform will check an explicit whitelist and blacklist instead.
> The default will now be assumed to be allowed to run anywhere.
> 
> For tests that do not specify their platforms explicitly, this has the effect of
> enabling these tests on non-linux platforms. For tests that always specified
> linux explicitly, there is no change.
> 
> For Python tests on FreeBSD at least; only seven python tests fail:
> 045 147 149 169 194 199 211
> 
> 045 and 149 appear to be misconfigurations,
> 147 and 194 are the AF_UNIX path too long error,
> 169 and 199 are bitmap migration bugs, and
> 211 is a bug that shows up on Linux platforms, too.
> 
> This is at least good evidence that these tests are not Linux-only. If
> they aren't suitable for other platforms, they should be disabled on a
> per-platform basis as appropriate.
> 
> Therefore, let's switch these on and deal with the failures.

What exactly do you mean by “deal with the failures”?  Do you have a
reference to patches that deal with them, or are you or is someone else
working on them...?

Apart from that, I am rather hesitant to take a patch through my tree
that not only may cause test failures on platforms that I will not or
actually cannot run tests on (like MacOS or Windows), but that actually
does introduce new failures as you describe.

Well, at least it doesn’t introduce build failures because it appears
there is no Python test that’s in the auto group, so I suppose “rather
hesitant” is not an “I won’t”.

Max

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[John Snow]

On 9/23/19 9:09 AM, Max Reitz wrote:
> On 18.09.19 01:45, John Snow wrote:
>> verify_platform will check an explicit whitelist and blacklist instead.
>> The default will now be assumed to be allowed to run anywhere.
>>
>> For tests that do not specify their platforms explicitly, this has the effect of
>> enabling these tests on non-linux platforms. For tests that always specified
>> linux explicitly, there is no change.
>>
>> For Python tests on FreeBSD at least; only seven python tests fail:
>> 045 147 149 169 194 199 211
>>
>> 045 and 149 appear to be misconfigurations,
>> 147 and 194 are the AF_UNIX path too long error,
>> 169 and 199 are bitmap migration bugs, and
>> 211 is a bug that shows up on Linux platforms, too.
>>
>> This is at least good evidence that these tests are not Linux-only. If
>> they aren't suitable for other platforms, they should be disabled on a
>> per-platform basis as appropriate.
>>
>> Therefore, let's switch these on and deal with the failures.
> 
> What exactly do you mean by “deal with the failures”?  Do you have a
> reference to patches that deal with them, or are you or is someone else
> working on them...?
> 
> Apart from that, I am rather hesitant to take a patch through my tree
> that not only may cause test failures on platforms that I will not or
> actually cannot run tests on (like MacOS or Windows), but that actually
> does introduce new failures as you describe.
> 
> Well, at least it doesn’t introduce build failures because it appears
> there is no Python test that’s in the auto group, so I suppose “rather
> hesitant” is not an “I won’t”.
> 
> Max
> 

This is why I didn't want this to be part of the logging series.

There's basically no way to win and this series is egregiously beyond
the five minutes I devoted to it.

I'd rather we just merge the last version if we're not ready to enable
testing on other platforms. It's wrong, but it was wrong anyway.

--js

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[John Snow]

On 9/23/19 9:09 AM, Max Reitz wrote:
> On 18.09.19 01:45, John Snow wrote:
>> verify_platform will check an explicit whitelist and blacklist instead.
>> The default will now be assumed to be allowed to run anywhere.
>>
>> For tests that do not specify their platforms explicitly, this has the effect of
>> enabling these tests on non-linux platforms. For tests that always specified
>> linux explicitly, there is no change.
>>
>> For Python tests on FreeBSD at least; only seven python tests fail:
>> 045 147 149 169 194 199 211
>>
>> 045 and 149 appear to be misconfigurations,
>> 147 and 194 are the AF_UNIX path too long error,
>> 169 and 199 are bitmap migration bugs, and
>> 211 is a bug that shows up on Linux platforms, too.
>>
>> This is at least good evidence that these tests are not Linux-only. If
>> they aren't suitable for other platforms, they should be disabled on a
>> per-platform basis as appropriate.
>>
>> Therefore, let's switch these on and deal with the failures.
> 
> What exactly do you mean by “deal with the failures”?  Do you have a
> reference to patches that deal with them, or are you or is someone else
> working on them...?
> 
> Apart from that, I am rather hesitant to take a patch through my tree
> that not only may cause test failures on platforms that I will not or
> actually cannot run tests on (like MacOS or Windows), but that actually
> does introduce new failures as you describe.
> 
> Well, at least it doesn’t introduce build failures because it appears
> there is no Python test that’s in the auto group, so I suppose “rather
> hesitant” is not an “I won’t”.
> 

Think of it more like this: The failures were always there, but we hid
them. I'm not "introducing new failures" as such O:-)

I think that I have demonstrated sufficiently that it's not correct to
prohibit python tests from running on other platforms wholesale, so I'd
prefer we don't do that anymore.

Further, iotests on FreeBSD already weren't 100% green, so I'm not
causing a regression in that sense, either.

I'm going to meekly push and ask that we stage this as-is, and when
something bad happens you can remind me that I wanted this and make me
do it.

--js

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4116 bytes --]

On 28.09.19 01:35, John Snow wrote:
> 
> 
> On 9/23/19 9:09 AM, Max Reitz wrote:
>> On 18.09.19 01:45, John Snow wrote:
>>> verify_platform will check an explicit whitelist and blacklist instead.
>>> The default will now be assumed to be allowed to run anywhere.
>>>
>>> For tests that do not specify their platforms explicitly, this has the effect of
>>> enabling these tests on non-linux platforms. For tests that always specified
>>> linux explicitly, there is no change.
>>>
>>> For Python tests on FreeBSD at least; only seven python tests fail:
>>> 045 147 149 169 194 199 211
>>>
>>> 045 and 149 appear to be misconfigurations,
>>> 147 and 194 are the AF_UNIX path too long error,
>>> 169 and 199 are bitmap migration bugs, and
>>> 211 is a bug that shows up on Linux platforms, too.
>>>
>>> This is at least good evidence that these tests are not Linux-only. If
>>> they aren't suitable for other platforms, they should be disabled on a
>>> per-platform basis as appropriate.
>>>
>>> Therefore, let's switch these on and deal with the failures.
>>
>> What exactly do you mean by “deal with the failures”?  Do you have a
>> reference to patches that deal with them, or are you or is someone else
>> working on them...?
>>
>> Apart from that, I am rather hesitant to take a patch through my tree
>> that not only may cause test failures on platforms that I will not or
>> actually cannot run tests on (like MacOS or Windows), but that actually
>> does introduce new failures as you describe.
>>
>> Well, at least it doesn’t introduce build failures because it appears
>> there is no Python test that’s in the auto group, so I suppose “rather
>> hesitant” is not an “I won’t”.
>>
> 
> Think of it more like this: The failures were always there, but we hid
> them. I'm not "introducing new failures" as such O:-)

That is incorrect.

As I have said, the conceptual problem is that the iotests now run as
part of make check.  As such, allowing auto tests to run on non-Linux
platforms may introduce build failures that I cannot do anything about.

And those are very much new failures.

> I think that I have demonstrated sufficiently that it's not correct to
> prohibit python tests from running on other platforms wholesale, so I'd
> prefer we don't do that anymore.

You have not.

The actual argument to convince me is “This does not affect any tests in
the auto group, so it will not introduce build failures at this time”.

> Further, iotests on FreeBSD already weren't 100% green, so I'm not
> causing a regression in that sense, either.

My problem is twofold:

(1) You claim “Sure, there are failures, but let’s just deal with them”
and then you do not deal with them.  Seems wrong to me.

I’m fine with the argument “Sorry, royal ‘we’.  But it just doesn’t help
anyone to hide the errors.  If someone’s on BSD and wants to run the
iotests, let them.”

That sounds good to me.

(2) Maybe someone adds a Python test in the future that is in auto and
that does not specify Linux as the only supported platform.  Then I send
a pull request and it breaks on macOS.  Now what?  Remove it from auto?
 Blindly put "macOS" in unsupported platforms?

In any case, it’ll be a problem for no good reason.

More on that in the next chunk.

> I'm going to meekly push and ask that we stage this as-is, and when
> something bad happens you can remind me that I wanted this and make me
> do it.

Make you do what?  Deal with the fact that a pull request is rejected
because a test fails on macOS?

This is precisely the kind of problem I already had with adding the
iotests to make check, and I’m already very much not happy about it.
(In that case it was $DISPLAY not being set on Peter’s test system.)


I’ll let you make the deduction of “The problem isn’t allowing the
iotests to run on non-Linux platforms, but the fact that they run in
make check” yourself, so that I no longer feel like I’m the only one who
considers that having been a mistake.

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

On 01/10/2019 20.44, Max Reitz wrote:
[...]
> As I have said, the conceptual problem is that the iotests now run as
> part of make check.  As such, allowing auto tests to run on non-Linux
> platforms may introduce build failures that I cannot do anything about.

Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ...
if something fails there, it likely should not be in the auto group.

> And those are very much new failures.
> 
>> I think that I have demonstrated sufficiently that it's not correct to
>> prohibit python tests from running on other platforms wholesale, so I'd
>> prefer we don't do that anymore.
> 
> You have not.
> 
> The actual argument to convince me is “This does not affect any tests in
> the auto group, so it will not introduce build failures at this time”.

I've applied the patch here and it works fine with FreeBSD and macOS:

 https://cirrus-ci.com/build/5169384718336000
 https://travis-ci.com/huth/qemu/builds/129968676

It also works fine with "make vm-build-openbsd BUILD_TARGET=check-block"
... so I think you don't have to worry here.

>> Further, iotests on FreeBSD already weren't 100% green, so I'm not
>> causing a regression in that sense, either.
> 
> My problem is twofold:
> 
> (1) You claim “Sure, there are failures, but let’s just deal with them”
> and then you do not deal with them.  Seems wrong to me.
> 
> I’m fine with the argument “Sorry, royal ‘we’.  But it just doesn’t help
> anyone to hide the errors.  If someone’s on BSD and wants to run the
> iotests, let them.”
> 
> That sounds good to me.
> 
> (2) Maybe someone adds a Python test in the future that is in auto and
> that does not specify Linux as the only supported platform.  Then I send
> a pull request and it breaks on macOS.  Now what?  Remove it from auto?
>  Blindly put "macOS" in unsupported platforms?

I think both solutions are good. If a test does not work on all systems,
it either should not be in the "auto" group, or it needs to be modified
to skip testing when running in an unsupported environment.

> In any case, it’ll be a problem for no good reason.

Really? Is "more test coverage" not a good reason?

> More on that in the next chunk.
> 
>> I'm going to meekly push and ask that we stage this as-is, and when
>> something bad happens you can remind me that I wanted this and make me
>> do it.
> 
> Make you do what?  Deal with the fact that a pull request is rejected
> because a test fails on macOS?
> 
> This is precisely the kind of problem I already had with adding the
> iotests to make check, and I’m already very much not happy about it.
> (In that case it was $DISPLAY not being set on Peter’s test system.)
> 
> I’ll let you make the deduction of “The problem isn’t allowing the
> iotests to run on non-Linux platforms, but the fact that they run in
> make check” yourself, so that I no longer feel like I’m the only one who
> considers that having been a mistake.

Max, I can understand that you are a little bit annoyed that this "make
check with iotests" caused some extra hurdles for you. But honestly,
removing that again would be quite egoistic by you. Try to put yourself
into the position of a "normal", non-blocklayer-maintainer QEMU
developer. For me, iotests were a *constant* source of frustration.
Often, when I ran them on top of my latest and greatest patches, to
check whether I caused a regression, the iotests simply failed. Then I
had to start debugging - did my patches cause the break, or is "master"
broken, too? In almost all cases, there was an issue in the master
branch already, either because they were failing on s390x, or because I
was using ext4 instead of xfs, or because I was using an older distro
than you, etc... . So while the iotests likely worked fine for the
limited set of systems that you, Kevin and the other hard-core block
layer developers are using, it's constantly broken for everybody else
who is not using the very same setup as you. The only way of *not*
getting upset about the iotests was to simply completely *ignore* them.
Is it that what you want?

Or maybe let me phrase it differently: Do you consider the iotests as
something that is more or less "private" to the hard-core block layer
developers, and it's ok if others completely ignore them and break them
by accident (and you also don't expect the normal developers to fix the
iotests afterwards)? Then sure, please go ahead and remove the iotests
from "make check" again. Maybe I just understood the idea of having
common tests in the repository wrong (or maybe the iotests should be
moved to a separate repository instead, so that the normal QEMU
developers do not get in touch with them anymore?) ... Otherwise, I
think it was the right step to add them "make check" so that everybody
now *has* to run at least a basic set of the iotests now before they can
their patches merged.

 Thomas


PS: Sorry, if my mail sounded a little bit harsh... but I really had
quite some frustration with the iotests in the past already.

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 10881 bytes --]

On 02.10.19 06:46, Thomas Huth wrote:
> On 01/10/2019 20.44, Max Reitz wrote:
> [...]
>> As I have said, the conceptual problem is that the iotests now run as
>> part of make check.  As such, allowing auto tests to run on non-Linux
>> platforms may introduce build failures that I cannot do anything about.
> 
> Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ...
> if something fails there, it likely should not be in the auto group.

Then we come to Windows and macOS.

What I’ve proposed to John on IRC was to simply skip the iotests in make
check for non-Linux systems.

>> And those are very much new failures.
>>
>>> I think that I have demonstrated sufficiently that it's not correct to
>>> prohibit python tests from running on other platforms wholesale, so I'd
>>> prefer we don't do that anymore.
>>
>> You have not.
>>
>> The actual argument to convince me is “This does not affect any tests in
>> the auto group, so it will not introduce build failures at this time”.
> 
> I've applied the patch here and it works fine with FreeBSD and macOS:
> 
>  https://cirrus-ci.com/build/5169384718336000
>  https://travis-ci.com/huth/qemu/builds/129968676
> 
> It also works fine with "make vm-build-openbsd BUILD_TARGET=check-block"
> ... so I think you don't have to worry here.

Obviously, because as I’ve said it doesn’t affect any tests in the auto
group.  Yet.

>>> Further, iotests on FreeBSD already weren't 100% green, so I'm not
>>> causing a regression in that sense, either.
>>
>> My problem is twofold:
>>
>> (1) You claim “Sure, there are failures, but let’s just deal with them”
>> and then you do not deal with them.  Seems wrong to me.
>>
>> I’m fine with the argument “Sorry, royal ‘we’.  But it just doesn’t help
>> anyone to hide the errors.  If someone’s on BSD and wants to run the
>> iotests, let them.”
>>
>> That sounds good to me.
>>
>> (2) Maybe someone adds a Python test in the future that is in auto and
>> that does not specify Linux as the only supported platform.  Then I send
>> a pull request and it breaks on macOS.  Now what?  Remove it from auto?
>>  Blindly put "macOS" in unsupported platforms?
> 
> I think both solutions are good. If a test does not work on all systems,
> it either should not be in the "auto" group, or it needs to be modified
> to skip testing when running in an unsupported environment.
> 
>> In any case, it’ll be a problem for no good reason.
> 
> Really? Is "more test coverage" not a good reason?

It isn’t when the solution is to just reduce the coverage again.

Furthermore, the problem is that we get this additional coverage on
systems that I will not support.

>> More on that in the next chunk.
>>
>>> I'm going to meekly push and ask that we stage this as-is, and when
>>> something bad happens you can remind me that I wanted this and make me
>>> do it.
>>
>> Make you do what?  Deal with the fact that a pull request is rejected
>> because a test fails on macOS?
>>
>> This is precisely the kind of problem I already had with adding the
>> iotests to make check, and I’m already very much not happy about it.
>> (In that case it was $DISPLAY not being set on Peter’s test system.)
>>
>> I’ll let you make the deduction of “The problem isn’t allowing the
>> iotests to run on non-Linux platforms, but the fact that they run in
>> make check” yourself, so that I no longer feel like I’m the only one who
>> considers that having been a mistake.
> 
> Max, I can understand that you are a little bit annoyed that this "make
> check with iotests" caused some extra hurdles for you. But honestly,
> removing that again would be quite egoistic by you. Try to put yourself
> into the position of a "normal", non-blocklayer-maintainer QEMU
> developer. For me, iotests were a *constant* source of frustration.
> Often, when I ran them on top of my latest and greatest patches, to
> check whether I caused a regression, the iotests simply failed. Then I
> had to start debugging - did my patches cause the break, or is "master"
> broken, too? In almost all cases, there was an issue in the master
> branch already, either because they were failing on s390x, or because I
> was using ext4 instead of xfs, or because I was using an older distro
> than you, etc... . So while the iotests likely worked fine for the
> limited set of systems that you, Kevin and the other hard-core block
> layer developers are using, it's constantly broken for everybody else
> who is not using the very same setup as you. The only way of *not*
> getting upset about the iotests was to simply completely *ignore* them.
> Is it that what you want?

It usually worked fine for me because it’s rather rare that non-block
patches broke the iotests.

I have to admit I actually didn’t think of other people wanting to run
the iotests; but to be honest, your mail doesn’t sound like you want to
run the iotests either.  It rather sounds like you have to because
otherwise I might complain.

(The reason I didn’t think of it is because non-block patches rarely
break them, so I wouldn’t run the iotests if I were a non-block
maintainer.  Sorry.)


Part of my problem is precisely with the fact that different systems are
different and that the iotests are not as deterministic as we’d want
them to be.  Realistically, I don’t think they’ll ever be.  I know they
haven’t been for six years even though it’s been kind of a goal, but it
hasn’t worked out so far.  (I don’t think it’s possible to write iotests
in a way that they are provably deterministic.)

So now you run the tests on your machine, they pass, you send a pull
request.  On Peter’s test machines, they pass, too, so the request is
merged.  But then on someone else’s machine, they don’t, so they get a
make check failure, which is just one step below build failure.  (Or
maybe it just turns up later on the test machines, because it’s flakey.
 Like in the case of 130 a week ago, which I CC’d you on.)

And then it’s my problem even though there’s most likely no real problem
there.  (And I can’t reproduce it, because, well, I have a different setup.)


Maybe my main problem is that I feel like now I have to deal with all of
the fallout, even though adding the iotests to make check wasn’t my idea
and neither me nor Kevin ever consented.  (I don’t know whether Kevin
consented implicitly, but I don’t see his name in bdd95e47844f2d8b.)

You can’t just give me more responsibility without my consent and then
call me egoistic for not liking it.

> Or maybe let me phrase it differently: Do you consider the iotests as
> something that is more or less "private" to the hard-core block layer
> developers, and it's ok if others completely ignore them and break them
> by accident (and you also don't expect the normal developers to fix the
> iotests afterwards)?

Well, that’s how it’s always worked, and that didn’t frustrate me.

Also, to give you a bit more perspective on why the situation has just
worsed for me, it’s because I run many more tests than what we have in
auto for qcow2.  I’ve rarely had a problem with that very limited set of
tests, but with others.  Mostly NBD, actually.

(I run qcow2, qcow2 -o compat=0.10, qcow2 -o refcount_bits=1, nbd, raw,
luks, qcow, qed, cloop, parallels, bochs, vdi, vhdx, vmdk, vpc; only
x64, but at least both -m64 and -m32.)

First, a disclaimer.  I’m aware that I may be very wrong about the
following, because I will not see make check failures on other people’s
systems that they fix themselves.  But the thing is, you haven’t told me
about any such failures on your system so far, so I’m just going to give
you my perspective here.

Honestly, it looks to me like you don’t even want to run the iotests.  I
interpret most of what you’ve written as:

- I don’t want to not run the iotests, because then people will hit me
  for making them fail.

- But they fail all the time, so I always need a baseline for what is
  expected to sometimes fail and what isn’t.  That’s very annoying.
  Let’s introduce a baseline in the form of auto/qcow2, and then let
  everyone verify that it works.

So to me it looks like we’ve just added all tests that never fail to
auto.  But if they never fail, then it’s like we haven’t run the tests
at all.  (As I’ve said, you need to tell me whether they do fail,
because I don’t see them fail[1].)

So honestly (I know this is unfair, but it’s my honest POV) it looks to
me like the current situation is just an excuse for everyone to be able
to claim they run the iotests.  When actually, they don’t, because they
run only a small well-defined set that doesn’t catch much anyway.  (What
they do catch is stuff that doesn’t help.)

I know you’ll say that we just need to ensure we can add more tests,
then.  But for one thing, the most important tests are the ones that
take the longest, like 041.  And the other of course is that adding any
more tests to make check just brings me more pain, so I won’t do it.

[1] There is the recent case of Kevin’s pull request having been
rejected because his test failed on anything but x64.  I’m torn here,
because I would have seen that failure on my -m32 build.  So it isn’t
like it would have evaded our view for long.

But OTOH, yes, this is a failure that could have annoyed quite some
people for a week; and now it has indeed been caught by make check.

> Then sure, please go ahead and remove the iotests
> from "make check" again. Maybe I just understood the idea of having
> common tests in the repository wrong (or maybe the iotests should be
> moved to a separate repository instead, so that the normal QEMU
> developers do not get in touch with them anymore?) ... Otherwise, I
> think it was the right step to add them "make check" so that everybody
> now *has* to run at least a basic set of the iotests now before they can
> their patches merged.

From what I read you yourself argued that that doesn’t mean much,
though, because everyone has different setups on their machine.

> PS: Sorry, if my mail sounded a little bit harsh... but I really had
> quite some frustration with the iotests in the past already.

Me too, that’s the point.


Overall, I think my main problem is that I feel like you’re leaving me
alone here.  It’s unfair to just add the iotests to make check without
my consent, then let me deal with the problems, and then call me
egoistic when I complain.

You have to decide: Either let me deal with the problems, but then I
have every right to be egoistic about it – or you help me deal with them.

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

On 02/10/2019 13.57, Max Reitz wrote:
> On 02.10.19 06:46, Thomas Huth wrote:
>> [...]
>> Max, I can understand that you are a little bit annoyed that this "make
>> check with iotests" caused some extra hurdles for you. But honestly,
>> removing that again would be quite egoistic by you. Try to put yourself
>> into the position of a "normal", non-blocklayer-maintainer QEMU
>> developer. For me, iotests were a *constant* source of frustration.
>> Often, when I ran them on top of my latest and greatest patches, to
>> check whether I caused a regression, the iotests simply failed. Then I
>> had to start debugging - did my patches cause the break, or is "master"
>> broken, too? In almost all cases, there was an issue in the master
>> branch already, either because they were failing on s390x, or because I
>> was using ext4 instead of xfs, or because I was using an older distro
>> than you, etc... . So while the iotests likely worked fine for the
>> limited set of systems that you, Kevin and the other hard-core block
>> layer developers are using, it's constantly broken for everybody else
>> who is not using the very same setup as you. The only way of *not*
>> getting upset about the iotests was to simply completely *ignore* them.
>> Is it that what you want?
> 
> It usually worked fine for me because it’s rather rare that non-block
> patches broke the iotests.
> 
> I have to admit I actually didn’t think of other people wanting to run
> the iotests; but to be honest, your mail doesn’t sound like you want to
> run the iotests either.

I *want* to run them. Occasionally - when I have new patches that might
affect anything related to the block layer. But then I don't want to be
in the situation where I first have to debug multiple other problems
with the iotests first that are not related to my new patches.

> (The reason I didn’t think of it is because non-block patches rarely
> break them, so I wouldn’t run the iotests if I were a non-block
> maintainer.  Sorry.)

Well, "rarely" is relative. They've been broken *completely* two times
in the 4.1 development time frame, and IIRC at least once in the 4.0
time frame.

[...]
> Maybe my main problem is that I feel like now I have to deal with all of
> the fallout, even though adding the iotests to make check wasn’t my idea
> and neither me nor Kevin ever consented.  (I don’t know whether Kevin
> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.)
> 
> You can’t just give me more responsibility without my consent and then
> call me egoistic for not liking it.

Ok, sorry for that. I guess one part of my frustration was also that the
patches to enable the iotests during "make check" have been on the list
for weeks - or rather months - and I never ever got much feedback from
you or Kevin on them. If you told me right in the beginning about your
concerns, we would not be at this point now. Also partly my bad, I
guess, since I could have reached out to you on IRC to discuss it, but
at that point in time, I rather thought that you just don't really care.
Thus it's good to have some conversation now, helps a lot to understand
the different expectations. Maybe we can also have a discussion about
this at KVM forum, I think it's easier to clarify some points of view
verbally there instead of using mails.

>> Or maybe let me phrase it differently: Do you consider the iotests as
>> something that is more or less "private" to the hard-core block layer
>> developers, and it's ok if others completely ignore them and break them
>> by accident (and you also don't expect the normal developers to fix the
>> iotests afterwards)?
> 
> Well, that’s how it’s always worked, and that didn’t frustrate me.

Ok ... you're the maintainer, so if that's really the way you prefer, I
can send a patch to remove the iotests from "make check" again.

> Honestly, it looks to me like you don’t even want to run the iotests.  I
> interpret most of what you’ve written as:
> 
> - I don’t want to not run the iotests, because then people will hit me
>   for making them fail.
> 
> - But they fail all the time, so I always need a baseline for what is
>   expected to sometimes fail and what isn’t.  That’s very annoying.
>   Let’s introduce a baseline in the form of auto/qcow2, and then let
>   everyone verify that it works.
> 
> So to me it looks like we’ve just added all tests that never fail to
> auto.  But if they never fail, then it’s like we haven’t run the tests
> at all.

I disagree. First, the complete iotest failures that have been merged
during the 4.1 development time frame to the master branch would have
been prevented, I think, so it's certainly not that "they never fail".

Second, yes, the basic idea was to start with a small set of tests in
the auto group which was already working, and then to increase that set
over time, once other tests run more stable, too. But you know the
iotests better than me, if you think that most of them can hardly
brought into the right shape, then this was likely just wishful thinking
by me.

> You have to decide: Either let me deal with the problems, but then I
> have every right to be egoistic about it – or you help me deal with them.

Since I'm not assigned to the block layer, I could only help
occasionally, so that's likely not enough for solving your frustration.
Thus I'll send a patch to remove the iotests from "make check" again.

 Thomas

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7230 bytes --]

On 02.10.19 15:11, Thomas Huth wrote:
> On 02/10/2019 13.57, Max Reitz wrote:
>> On 02.10.19 06:46, Thomas Huth wrote:
>>> [...]
>>> Max, I can understand that you are a little bit annoyed that this "make
>>> check with iotests" caused some extra hurdles for you. But honestly,
>>> removing that again would be quite egoistic by you. Try to put yourself
>>> into the position of a "normal", non-blocklayer-maintainer QEMU
>>> developer. For me, iotests were a *constant* source of frustration.
>>> Often, when I ran them on top of my latest and greatest patches, to
>>> check whether I caused a regression, the iotests simply failed. Then I
>>> had to start debugging - did my patches cause the break, or is "master"
>>> broken, too? In almost all cases, there was an issue in the master
>>> branch already, either because they were failing on s390x, or because I
>>> was using ext4 instead of xfs, or because I was using an older distro
>>> than you, etc... . So while the iotests likely worked fine for the
>>> limited set of systems that you, Kevin and the other hard-core block
>>> layer developers are using, it's constantly broken for everybody else
>>> who is not using the very same setup as you. The only way of *not*
>>> getting upset about the iotests was to simply completely *ignore* them.
>>> Is it that what you want?
>>
>> It usually worked fine for me because it’s rather rare that non-block
>> patches broke the iotests.
>>
>> I have to admit I actually didn’t think of other people wanting to run
>> the iotests; but to be honest, your mail doesn’t sound like you want to
>> run the iotests either.
> 
> I *want* to run them. Occasionally - when I have new patches that might
> affect anything related to the block layer. But then I don't want to be
> in the situation where I first have to debug multiple other problems
> with the iotests first that are not related to my new patches.

OK.

>> (The reason I didn’t think of it is because non-block patches rarely
>> break them, so I wouldn’t run the iotests if I were a non-block
>> maintainer.  Sorry.)
> 
> Well, "rarely" is relative. They've been broken *completely* two times
> in the 4.1 development time frame, and IIRC at least once in the 4.0
> time frame.

Hm, OK.  So my memory is just shot.

> [...]
>> Maybe my main problem is that I feel like now I have to deal with all of
>> the fallout, even though adding the iotests to make check wasn’t my idea
>> and neither me nor Kevin ever consented.  (I don’t know whether Kevin
>> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.)
>>
>> You can’t just give me more responsibility without my consent and then
>> call me egoistic for not liking it.
> 
> Ok, sorry for that. I guess one part of my frustration was also that the
> patches to enable the iotests during "make check" have been on the list
> for weeks - or rather months - and I never ever got much feedback from
> you or Kevin on them.
I think that’s for two reasons:

(1) The time line in the block layer is unfortunately apparently
different from what you’re used to.  Or, rather, fortunately for you.
It isn’t too rare for me to have patches up for a year or more.

(2) We’ve had discussions on this before and the result was always the
same.  Yes, we’d like to run the iotests as part of make check, but we
can’t imagine it would work right now (where “right now” is whenever we
have the discussion).

So as for me, I felt bad about plainly saying “no”.  But I cannot say
“yes” either.  You’re right that I should’ve said something.  There are
more words than “yes” and “no”.  Sorry again.

I can imagine I had hoped Kevin would speak up (and thus take
responsibility) or you’d become so annoyed with our silence that you’d
angrily reach out to us and I’d have to do something.  (Which is a
horrible attitude, I realize that.)
I do remember that I didn’t expect that you’d just find someone else to
merge the patch.  That took me by surprise.

> If you told me right in the beginning about your
> concerns, we would not be at this point now. Also partly my bad, I
> guess, since I could have reached out to you on IRC to discuss it, but
> at that point in time, I rather thought that you just don't really care.
> Thus it's good to have some conversation now, helps a lot to understand
> the different expectations. Maybe we can also have a discussion about
> this at KVM forum, I think it's easier to clarify some points of view
> verbally there instead of using mails.

Well, I won’t be at KVM Forum, unfortunately.

>>> Or maybe let me phrase it differently: Do you consider the iotests as
>>> something that is more or less "private" to the hard-core block layer
>>> developers, and it's ok if others completely ignore them and break them
>>> by accident (and you also don't expect the normal developers to fix the
>>> iotests afterwards)?
>>
>> Well, that’s how it’s always worked, and that didn’t frustrate me.
> 
> Ok ... you're the maintainer, so if that's really the way you prefer, I
> can send a patch to remove the iotests from "make check" again.
> 
>> Honestly, it looks to me like you don’t even want to run the iotests.  I
>> interpret most of what you’ve written as:
>>
>> - I don’t want to not run the iotests, because then people will hit me
>>   for making them fail.
>>
>> - But they fail all the time, so I always need a baseline for what is
>>   expected to sometimes fail and what isn’t.  That’s very annoying.
>>   Let’s introduce a baseline in the form of auto/qcow2, and then let
>>   everyone verify that it works.
>>
>> So to me it looks like we’ve just added all tests that never fail to
>> auto.  But if they never fail, then it’s like we haven’t run the tests
>> at all.
> 
> I disagree. First, the complete iotest failures that have been merged
> during the 4.1 development time frame to the master branch would have
> been prevented, I think, so it's certainly not that "they never fail".
> 
> Second, yes, the basic idea was to start with a small set of tests in
> the auto group which was already working, and then to increase that set
> over time, once other tests run more stable, too. But you know the
> iotests better than me, if you think that most of them can hardly
> brought into the right shape, then this was likely just wishful thinking
> by me.

I know that one never knows which test turns out to be unstable next.

>> You have to decide: Either let me deal with the problems, but then I
>> have every right to be egoistic about it – or you help me deal with them.
> 
> Since I'm not assigned to the block layer, I could only help
> occasionally, so that's likely not enough for solving your frustration.
> Thus I'll send a patch to remove the iotests from "make check" again.

OK.  Maybe someone else will step up once they see the patch.

Or can we have some middle ground?  Something that runs on some CI
systems (and notifies me and others) but won’t result in pull requests
being rejected or cause make check failures?

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 572 bytes --]

On 02/10/2019 15.36, Max Reitz wrote:
[...]
> Or can we have some middle ground?  Something that runs on some CI
> systems (and notifies me and others) but won’t result in pull requests
> being rejected or cause make check failures?

Yes, I think that might be an option... Since many developers are using
Travis, I think I'd make sense to add a "make check-block" to
.travis.yml so that at least most people still get automatic test
coverage, without blocking Peter's "make check" pull request criteria.

I'll post a patch for doing that, too.

 Thomas


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 4661 bytes --]

Not sure where in this thread to reply, but since my name is mentioned
in this mail, it might at least be not the worst one.

Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
> On 02.10.19 06:46, Thomas Huth wrote:
> > On 01/10/2019 20.44, Max Reitz wrote:
> > [...]
> >> As I have said, the conceptual problem is that the iotests now run as
> >> part of make check.  As such, allowing auto tests to run on non-Linux
> >> platforms may introduce build failures that I cannot do anything about.
> > 
> > Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ...
> > if something fails there, it likely should not be in the auto group.
> 
> Then we come to Windows and macOS.
> 
> What I’ve proposed to John on IRC was to simply skip the iotests in make
> check for non-Linux systems.

I think this really makes sense. Or at least have a blacklist of OSes
that we can't support iotests on, which would contain at least Windows
and OS X. Windows because I'm pretty sure that it doesn't work anyway,
and OS X because if something fails there, we have no way to reproduce.
The occasional build failures on OS X that must be fixed by blindly
guessing what could work without any way to test the fix are already bad
enough.

> > Max, I can understand that you are a little bit annoyed that this "make
> > check with iotests" caused some extra hurdles for you. But honestly,
> > removing that again would be quite egoistic by you. Try to put yourself
> > into the position of a "normal", non-blocklayer-maintainer QEMU
> > developer. For me, iotests were a *constant* source of frustration.
> > Often, when I ran them on top of my latest and greatest patches, to
> > check whether I caused a regression, the iotests simply failed. Then I
> > had to start debugging - did my patches cause the break, or is "master"
> > broken, too? In almost all cases, there was an issue in the master
> > branch already, either because they were failing on s390x, or because I
> > was using ext4 instead of xfs, or because I was using an older distro
> > than you, etc... . So while the iotests likely worked fine for the
> > limited set of systems that you, Kevin and the other hard-core block
> > layer developers are using, it's constantly broken for everybody else
> > who is not using the very same setup as you. The only way of *not*
> > getting upset about the iotests was to simply completely *ignore* them.
> > Is it that what you want?
> 
> It usually worked fine for me because it’s rather rare that non-block
> patches broke the iotests.

I disagree. It happened all the time that someone else broke the iotests
in master and we needed to fix it up.

> Maybe my main problem is that I feel like now I have to deal with all of
> the fallout, even though adding the iotests to make check wasn’t my idea
> and neither me nor Kevin ever consented.  (I don’t know whether Kevin
> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.)
> 
> You can’t just give me more responsibility without my consent and then
> call me egoistic for not liking it.

I think I may have implicitly consented by helping Alex with the changes
to 'check' that made its output fit better in 'make check'.

Basically, my stance is that I share your dislike for the effects on me
personally (also, I can't run 'make check' any more before a pull
request without testing half of the iotests twice because I still need a
full iotests run), but I also think that having iotests in 'make check'
is really the right thing for the project despite the inconvenience it
means for me.

> I know you’ll say that we just need to ensure we can add more tests,
> then.  But for one thing, the most important tests are the ones that
> take the longest, like 041.  And the other of course is that adding any
> more tests to make check just brings me more pain, so I won’t do it.

That's something that can be fixed by tweaking the auto group.

Can we run tests in 'auto' that require the system emulator? If so,
let's add 030 040 041 to the default set. They take long, but they are
among the most valuable tests we have. If this makes the tests take too
much time, let's remove some less important ones instead.

> [1] There is the recent case of Kevin’s pull request having been
> rejected because his test failed on anything but x64.  I’m torn here,
> because I would have seen that failure on my -m32 build.  So it isn’t
> like it would have evaded our view for long.

I messed up, so this pull request was rightly stopped. I consider this
one a feature, not a bug.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5218 bytes --]

On 02.10.19 18:44, Kevin Wolf wrote:
> Not sure where in this thread to reply, but since my name is mentioned
> in this mail, it might at least be not the worst one.
> 
> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>> On 02.10.19 06:46, Thomas Huth wrote:
>>> On 01/10/2019 20.44, Max Reitz wrote:
>>> [...]
>>>> As I have said, the conceptual problem is that the iotests now run as
>>>> part of make check.  As such, allowing auto tests to run on non-Linux
>>>> platforms may introduce build failures that I cannot do anything about.
>>>
>>> Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ...
>>> if something fails there, it likely should not be in the auto group.
>>
>> Then we come to Windows and macOS.
>>
>> What I’ve proposed to John on IRC was to simply skip the iotests in make
>> check for non-Linux systems.
> 
> I think this really makes sense. Or at least have a blacklist of OSes
> that we can't support iotests on, which would contain at least Windows
> and OS X. Windows because I'm pretty sure that it doesn't work anyway,
> and OS X because if something fails there, we have no way to reproduce.
> The occasional build failures on OS X that must be fixed by blindly
> guessing what could work without any way to test the fix are already bad
> enough.
> 
>>> Max, I can understand that you are a little bit annoyed that this "make
>>> check with iotests" caused some extra hurdles for you. But honestly,
>>> removing that again would be quite egoistic by you. Try to put yourself
>>> into the position of a "normal", non-blocklayer-maintainer QEMU
>>> developer. For me, iotests were a *constant* source of frustration.
>>> Often, when I ran them on top of my latest and greatest patches, to
>>> check whether I caused a regression, the iotests simply failed. Then I
>>> had to start debugging - did my patches cause the break, or is "master"
>>> broken, too? In almost all cases, there was an issue in the master
>>> branch already, either because they were failing on s390x, or because I
>>> was using ext4 instead of xfs, or because I was using an older distro
>>> than you, etc... . So while the iotests likely worked fine for the
>>> limited set of systems that you, Kevin and the other hard-core block
>>> layer developers are using, it's constantly broken for everybody else
>>> who is not using the very same setup as you. The only way of *not*
>>> getting upset about the iotests was to simply completely *ignore* them.
>>> Is it that what you want?
>>
>> It usually worked fine for me because it’s rather rare that non-block
>> patches broke the iotests.
> 
> I disagree. It happened all the time that someone else broke the iotests
> in master and we needed to fix it up.

OK.

(In my experience, it’s still mostly block patches, only that they tend
to go through non-block trees.)

>> Maybe my main problem is that I feel like now I have to deal with all of
>> the fallout, even though adding the iotests to make check wasn’t my idea
>> and neither me nor Kevin ever consented.  (I don’t know whether Kevin
>> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.)
>>
>> You can’t just give me more responsibility without my consent and then
>> call me egoistic for not liking it.
> 
> I think I may have implicitly consented by helping Alex with the changes
> to 'check' that made its output fit better in 'make check'.
> 
> Basically, my stance is that I share your dislike for the effects on me
> personally (also, I can't run 'make check' any more before a pull
> request without testing half of the iotests twice because I still need a
> full iotests run), but I also think that having iotests in 'make check'
> is really the right thing for the project despite the inconvenience it
> means for me.

Then I expect you to NACK Thomas’s patch, and I take it to mean that I
can rely on you to handle basically all make check iotests failures that
are not caused by my own pull requests.

Works for me.

>> I know you’ll say that we just need to ensure we can add more tests,
>> then.  But for one thing, the most important tests are the ones that
>> take the longest, like 041.  And the other of course is that adding any
>> more tests to make check just brings me more pain, so I won’t do it.
> 
> That's something that can be fixed by tweaking the auto group.
> 
> Can we run tests in 'auto' that require the system emulator? If so,
> let's add 030 040 041 to the default set. They take long, but they are
> among the most valuable tests we have. If this makes the tests take too
> much time, let's remove some less important ones instead.
> 
>> [1] There is the recent case of Kevin’s pull request having been
>> rejected because his test failed on anything but x64.  I’m torn here,
>> because I would have seen that failure on my -m32 build.  So it isn’t
>> like it would have evaded our view for long.
> 
> I messed up, so this pull request was rightly stopped. I consider this
> one a feature, not a bug.

Sure, that was a good thing.  I just wonder whether that instance is
enough to justify the pain.

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
> On 02.10.19 18:44, Kevin Wolf wrote:
> > Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
> >> It usually worked fine for me because it’s rather rare that non-block
> >> patches broke the iotests.
> > 
> > I disagree. It happened all the time that someone else broke the iotests
> > in master and we needed to fix it up.
> 
> OK.
> 
> (In my experience, it’s still mostly block patches, only that they tend
> to go through non-block trees.)

Fair enough, it's usually code that touches block code. I assumed "block
patches" to mean patches that go through one of the block trees and for
which iotests are run before sending a pull request.

In the end, I don't care what code these patches touched. I do an
innocent git pull, and when I finally see that it's master that breaks
iotests and not my patches on top of it, I'm annoyed.

> >> Maybe my main problem is that I feel like now I have to deal with all of
> >> the fallout, even though adding the iotests to make check wasn’t my idea
> >> and neither me nor Kevin ever consented.  (I don’t know whether Kevin
> >> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.)
> >>
> >> You can’t just give me more responsibility without my consent and then
> >> call me egoistic for not liking it.
> > 
> > I think I may have implicitly consented by helping Alex with the changes
> > to 'check' that made its output fit better in 'make check'.
> > 
> > Basically, my stance is that I share your dislike for the effects on me
> > personally (also, I can't run 'make check' any more before a pull
> > request without testing half of the iotests twice because I still need a
> > full iotests run), but I also think that having iotests in 'make check'
> > is really the right thing for the project despite the inconvenience it
> > means for me.
> 
> Then I expect you to NACK Thomas’s patch, and I take it to mean that I
> can rely on you to handle basically all make check iotests failures that
> are not caused by my own pull requests.
> 
> Works for me.

Ah, we can also play this game the other way round.

So if you merge that revert, when iotests break in master, I take it I
can rely on you to fix it up before it impacts my working branch?

> >> I know you’ll say that we just need to ensure we can add more tests,
> >> then.  But for one thing, the most important tests are the ones that
> >> take the longest, like 041.  And the other of course is that adding any
> >> more tests to make check just brings me more pain, so I won’t do it.
> > 
> > That's something that can be fixed by tweaking the auto group.
> > 
> > Can we run tests in 'auto' that require the system emulator? If so,
> > let's add 030 040 041 to the default set. They take long, but they are
> > among the most valuable tests we have. If this makes the tests take too
> > much time, let's remove some less important ones instead.
> > 
> >> [1] There is the recent case of Kevin’s pull request having been
> >> rejected because his test failed on anything but x64.  I’m torn here,
> >> because I would have seen that failure on my -m32 build.  So it isn’t
> >> like it would have evaded our view for long.
> > 
> > I messed up, so this pull request was rightly stopped. I consider this
> > one a feature, not a bug.
> 
> Sure, that was a good thing.  I just wonder whether that instance is
> enough to justify the pain.

Yes, making iotests stable on CI probably involves some pain, especially
initially. However, if we don't fix them upstream, they'll end up on our
desk again downstream because people run tests neither on your nor on my
laptop.

I think we might actually save time by fixing them once and for all,
even if they are problems in the test cases and not in QEMU, and making
new test cases portable from the start, instead of getting individual
reports one at a time and having to look at the same test cases again
and again.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 8319 bytes --]

On 04.10.19 12:19, Kevin Wolf wrote:
> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
>> On 02.10.19 18:44, Kevin Wolf wrote:
>>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>>>> It usually worked fine for me because it’s rather rare that non-block
>>>> patches broke the iotests.
>>>
>>> I disagree. It happened all the time that someone else broke the iotests
>>> in master and we needed to fix it up.
>>
>> OK.
>>
>> (In my experience, it’s still mostly block patches, only that they tend
>> to go through non-block trees.)
> 
> Fair enough, it's usually code that touches block code. I assumed "block
> patches" to mean patches that go through one of the block trees and for
> which iotests are run before sending a pull request.
> 
> In the end, I don't care what code these patches touched. I do an
> innocent git pull, and when I finally see that it's master that breaks
> iotests and not my patches on top of it, I'm annoyed.

Hm.  Part of my point was that this still happens all the time.

Which is why I’d prefer more tests to run in CI than a handful of not
very useful ones in make check.

(Of course, running it in a CI won’t prevent iotest failures on your
machine, but in practice neither does the current model.)

>>>> Maybe my main problem is that I feel like now I have to deal with all of
>>>> the fallout, even though adding the iotests to make check wasn’t my idea
>>>> and neither me nor Kevin ever consented.  (I don’t know whether Kevin
>>>> consented implicitly, but I don’t see his name in bdd95e47844f2d8b.)
>>>>
>>>> You can’t just give me more responsibility without my consent and then
>>>> call me egoistic for not liking it.
>>>
>>> I think I may have implicitly consented by helping Alex with the changes
>>> to 'check' that made its output fit better in 'make check'.
>>>
>>> Basically, my stance is that I share your dislike for the effects on me
>>> personally (also, I can't run 'make check' any more before a pull
>>> request without testing half of the iotests twice because I still need a
>>> full iotests run),

-x auto for the qcow2 pass, by the way.

>>> but I also think that having iotests in 'make check'
>>> is really the right thing for the project despite the inconvenience it
>>> means for me.
>>
>> Then I expect you to NACK Thomas’s patch, and I take it to mean that I
>> can rely on you to handle basically all make check iotests failures that
>> are not caused by my own pull requests.
>>
>> Works for me.
> 
> Ah, we can also play this game the other way round.
> 
> So if you merge that revert, when iotests break in master, I take it I
> can rely on you to fix it up before it impacts my working branch?

This is not a game, I’m talking purely from my standpoint:
(I talked wrongly, but more on that below)

Whenever make check fails, it’s urgent.  Without iotests running in make
check, we had some time to sort the situation out.  That’s no longer the
case.

That means we need to take care of everything immediately.  And I purely
want help with that.  I just did not have the impression that such help
was there in the past months.
(This impression may or may not have a correlation to reality.)

I thought that was just because nobody really cared about the iotests in
make check.  Now you say you do, so that’s why I said you should NACK
the revert and then I know that I can count on your help.


Now where I was wrong: I didn’t say “your help” but “you handle it”.
That was wrong.  I’m sorry.  That would mean I reap all the benefits and
you have to pay the price.


What I’m not so sure of is why you didn’t just say that’s unfair and
instead reply with an impossible suggestion.  That makes it a bit
difficult for me to find out exactly what you plan to do.

I take your point as “Exactly, this suggestion would be impossible.
With the tests in make check, the worst that happens is CI breakage and
rejected pull requests, and dealing with those is very much possible.”[1]

As I’ve said, my POV is different, because I find CI breakage, make
check breakage, and rejected pull requests to be worse because those are
failures on other people’s machines.  That puts me personally under much
more stress than failures on my own machine.  But it seems that you feel
differently.

[1] Or maybe you wanted to say that you find a rare intermittent failure
that slips into make check less worse than 100 % failure of some test
for everyone who runs the iotests.  I don’t know.
I know that the 100 % failures are annoying but generally easily fixed;
whereas the intermittent ones are the ones that stick and hard to fix.
And those intermittent ones are unlikely to cause pull requests to be
rejected.

>>>> I know you’ll say that we just need to ensure we can add more tests,
>>>> then.  But for one thing, the most important tests are the ones that
>>>> take the longest, like 041.  And the other of course is that adding any
>>>> more tests to make check just brings me more pain, so I won’t do it.
>>>
>>> That's something that can be fixed by tweaking the auto group.
>>>
>>> Can we run tests in 'auto' that require the system emulator? If so,
>>> let's add 030 040 041 to the default set. They take long, but they are
>>> among the most valuable tests we have. If this makes the tests take too
>>> much time, let's remove some less important ones instead.
>>>
>>>> [1] There is the recent case of Kevin’s pull request having been
>>>> rejected because his test failed on anything but x64.  I’m torn here,
>>>> because I would have seen that failure on my -m32 build.  So it isn’t
>>>> like it would have evaded our view for long.
>>>
>>> I messed up, so this pull request was rightly stopped. I consider this
>>> one a feature, not a bug.
>>
>> Sure, that was a good thing.  I just wonder whether that instance is
>> enough to justify the pain.
> 
> Yes, making iotests stable on CI probably involves some pain, especially
> initially. However, if we don't fix them upstream, they'll end up on our
> desk again downstream because people run tests neither on your nor on my
> laptop.
> 
> I think we might actually save time by fixing them once and for all,
> even if they are problems in the test cases and not in QEMU, and making
> new test cases portable from the start, instead of getting individual
> reports one at a time and having to look at the same test cases again
> and again.

You should really know I’m all for that and that I’ve done my share of
work there.

But from my perspective we’ve said and tried that for six years and we
aren’t there still.  So to me “We should just fix it” of course sounds
correct, but I also don’t believe it’ll happen any time soon.  Mostly
because we generally don’t know what to fix before it breaks, but also
because that’s exactly what we’ve tried to do for at least six years.


So, hm.  I don’t quite know where we’re going.

I still think that I personally would prefer the iotests to not run as
part of make check, but just as CI.

You do have a point that dropping the iotests from make check would
benefit me at your expense.  (Well, it’d be the same result for both of
us, it just appears that we have different perceptions of pain.)

OTOH, keeping the iotests in make check means we have to act on failure
reports immediately.  For example, someone™ needs to investigate the 130
failure Peter has reported.  I’ve just replied “I don’t know”, CC’d
Thomas, and waited whether anyone else would do anything.  Nobody did,
and that can’t work.  (I also wrote that I wouldn’t act on it, precisely
because I didn’t see the point.  I assumed that if anyone disagreed with
that statement, they would have said something.)

So acting on such reports means pain, too.  I consider it greater than
the previous kind of pain, because I prefer “This sucks, I’ll have to
fix it this week or so” to “Oh crap, I’ll have to investigate now,
reproduce it, write an email as soon as possible, and fix it”.

But at least we’ll all have this pain as long as we share it.  And then
it might not be so bad.

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Peter Maydell]

On Fri, 4 Oct 2019 at 13:45, Max Reitz <mreitz@redhat.com> wrote:
> > In the end, I don't care what code these patches touched. I do an
> > innocent git pull, and when I finally see that it's master that breaks
> > iotests and not my patches on top of it, I'm annoyed.
>
> Hm.  Part of my point was that this still happens all the time.
>
> Which is why I’d prefer more tests to run in CI than a handful of not
> very useful ones in make check.
>
> (Of course, running it in a CI won’t prevent iotest failures on your
> machine, but in practice neither does the current model.)

If you want the tree to be defended against problems, then
you need to run tests in 'make check'. Nothing else gets consistently
run and has failures spotted either before code goes into the
tree or quickly afterwards. 'make check' does have the restriction
that we don't want the tests to take too long to run, but in
general the block layer should be running some reasonable subset
of tests in the project's standard "run the tests please" command.
(I have no opinion on exactly what that subset ought to be, beyond
that it would be good if that subset doesn't have known intermittent
failures in it.)

> I still think that I personally would prefer the iotests to not run as
> part of make check, but just as CI.

'make check' *is* our CI gate, to a first approximation. Most of
the various travis and other setups are simply a front-end for
"build QEMU in various configurations and on various hosts
and then run 'make check'". The travis setup at the moment is
a bit odd, because it runs tests but it's not a gate on our
merging changes. Ideally I would like to fix this so that
rather than me personally running "make check" via a bunch
of scripts we have one CI setup that we trust (gitlab seems
the current favoured contender) and that gates changes flowing
into master rather than me doing it manually. We might then turn
travis off if it's not providing anything for us that the gitlab
setup doesn't.

thanks
-- PMM

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3322 bytes --]

On 04.10.19 15:16, Peter Maydell wrote:
> On Fri, 4 Oct 2019 at 13:45, Max Reitz <mreitz@redhat.com> wrote:
>>> In the end, I don't care what code these patches touched. I do an
>>> innocent git pull, and when I finally see that it's master that breaks
>>> iotests and not my patches on top of it, I'm annoyed.
>>
>> Hm.  Part of my point was that this still happens all the time.
>>
>> Which is why I’d prefer more tests to run in CI than a handful of not
>> very useful ones in make check.
>>
>> (Of course, running it in a CI won’t prevent iotest failures on your
>> machine, but in practice neither does the current model.)
> 
> If you want the tree to be defended against problems, then
> you need to run tests in 'make check'. Nothing else gets consistently
> run and has failures spotted either before code goes into the
> tree or quickly afterwards.

Yes, but part of the problem is that what does get run as part of make
check currently does not help much.

So this doesn’t really defend the iotests from problems.

> 'make check' does have the restriction
> that we don't want the tests to take too long to run, but in
> general the block layer should be running some reasonable subset
> of tests in the project's standard "run the tests please" command.
> (I have no opinion on exactly what that subset ought to be, beyond
> that it would be good if that subset doesn't have known intermittent
> failures in it.)

Deciding the subset is difficult.  My stance was that it’s better to not
choose an arbitrary subset here but ensure that really everything gets
run as part of CI, that is asynchronously so it doesn’t block anyone and
can take as long as it wants.

If we decide to get pull requests based on that, then that’d bring even
more pain, but at least it’d be honest.  But just running half of the
qcow2 tests to me seems only like we want to pretend we ran the iotests.
 So for me, iotests still break, and we need to deal with make check
failures on top.  I’d at least prefer one or the other.


I voted for dropping make check, because running all iotests doesn’t
seem feasible to me.

>> I still think that I personally would prefer the iotests to not run as
>> part of make check, but just as CI.
> 
> 'make check' *is* our CI gate, to a first approximation. Most of
> the various travis and other setups are simply a front-end for
> "build QEMU in various configurations and on various hosts
> and then run 'make check'". The travis setup at the moment is
> a bit odd, because it runs tests but it's not a gate on our
> merging changes. Ideally I would like to fix this so that
> rather than me personally running "make check" via a bunch
> of scripts we have one CI setup that we trust (gitlab seems
> the current favoured contender) and that gates changes flowing
> into master rather than me doing it manually. We might then turn
> travis off if it's not providing anything for us that the gitlab
> setup doesn't.

Hm, but make check also serves other purposes.  I would suppose e.g.
distributions generally require make check to pass when building packages.

I assumed our CI included more things than just make check, so we could
move the iotests from out of make check but keep them as part of CI.

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Peter Maydell]

On Fri, 4 Oct 2019 at 14:50, Max Reitz <mreitz@redhat.com> wrote:
> On 04.10.19 15:16, Peter Maydell wrote:
> > 'make check' does have the restriction
> > that we don't want the tests to take too long to run, but in
> > general the block layer should be running some reasonable subset
> > of tests in the project's standard "run the tests please" command.
> > (I have no opinion on exactly what that subset ought to be, beyond
> > that it would be good if that subset doesn't have known intermittent
> > failures in it.)
>
> Deciding the subset is difficult.  My stance was that it’s better to not
> choose an arbitrary subset here but ensure that really everything gets
> run as part of CI, that is asynchronously so it doesn’t block anyone and
> can take as long as it wants.

I wonder if we have a terminology difference confusion here.
To me "CI" means "we have tests which we automatically run
before pushing commits to master, and if they fail then we
don't push". So (a) they have to run synchronously and (b) there
is a need for them to run in a reasonable period of time because
otherwise it takes too long to run the tests before pushing
to master and we get a backlog of unprocessed pullrequests
and annoying delays.

> If we decide to get pull requests based on that, then that’d bring even
> more pain, but at least it’d be honest.  But just running half of the
> qcow2 tests to me seems only like we want to pretend we ran the iotests.
>  So for me, iotests still break, and we need to deal with make check
> failures on top.  I’d at least prefer one or the other.

I think the ideal we're aiming for here is:
 (1) people doing active work in the block subsystem are probably
often going to want to run all the iotests, and certainly the
subsystem maintainers will want to do that as they put together
pull requests.
 (2) but people who *don't* do active work in the block subsystem
still sometimes touch it in passing as part of things like global
refactorings or other changes that touch big parts of the tree,
or accidentally break it with a change to some other bit of QEMU
entirely. These people won't run the full iotests, but it is
reasonable to expect them to run 'make check', and it would be good
if that caught "whoops you broke the block subsystem".

Similarly, "make check" has incredibly spotty coverage of
various arm boards and devices, but it does do some basic
checks which do catch accidental breakage.

thanks
-- PMM

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3797 bytes --]

On 04.10.19 16:05, Peter Maydell wrote:
> On Fri, 4 Oct 2019 at 14:50, Max Reitz <mreitz@redhat.com> wrote:
>> On 04.10.19 15:16, Peter Maydell wrote:
>>> 'make check' does have the restriction
>>> that we don't want the tests to take too long to run, but in
>>> general the block layer should be running some reasonable subset
>>> of tests in the project's standard "run the tests please" command.
>>> (I have no opinion on exactly what that subset ought to be, beyond
>>> that it would be good if that subset doesn't have known intermittent
>>> failures in it.)
>>
>> Deciding the subset is difficult.  My stance was that it’s better to not
>> choose an arbitrary subset here but ensure that really everything gets
>> run as part of CI, that is asynchronously so it doesn’t block anyone and
>> can take as long as it wants.
> 
> I wonder if we have a terminology difference confusion here.
> To me "CI" means "we have tests which we automatically run
> before pushing commits to master, and if they fail then we
> don't push". So (a) they have to run synchronously and (b) there
> is a need for them to run in a reasonable period of time because
> otherwise it takes too long to run the tests before pushing
> to master and we get a backlog of unprocessed pullrequests
> and annoying delays.

Hm, yes.  In that case, there’s of course no point in moving it.

I meant moving the tests to somewhere where they run asynchronously.

>> If we decide to get pull requests based on that, then that’d bring even
>> more pain, but at least it’d be honest.  But just running half of the
>> qcow2 tests to me seems only like we want to pretend we ran the iotests.
>>  So for me, iotests still break, and we need to deal with make check
>> failures on top.  I’d at least prefer one or the other.
> 
> I think the ideal we're aiming for here is:
>  (1) people doing active work in the block subsystem are probably
> often going to want to run all the iotests, and certainly the
> subsystem maintainers will want to do that as they put together
> pull requests.
>  (2) but people who *don't* do active work in the block subsystem
> still sometimes touch it in passing as part of things like global
> refactorings or other changes that touch big parts of the tree,
> or accidentally break it with a change to some other bit of QEMU
> entirely. These people won't run the full iotests, but it is
> reasonable to expect them to run 'make check', and it would be good
> if that caught "whoops you broke the block subsystem".
> 
> Similarly, "make check" has incredibly spotty coverage of
> various arm boards and devices, but it does do some basic
> checks which do catch accidental breakage.

Yes, it has its use, but at the same time it means that intermittent
failure can appear in make check because some iotest is broken.  (The
past has shown that it’s hard to predict which tests will at some point
start to exhibit such behavior.)

This then requires immediate attention, and I simply want help with that
from someone who really cares about having the test be part of make
check, as I wrote in my reply to Thomas’s patch “iotests: Do not run the
iotests during "make check" anymore”.

The issues are just half as pressing when it isn’t make check that’s
intermittently failing.  (I hope nobody takes this as “He doesn’t want
to fix intermittent failures”, because I do really try to do that.  So I
don’t consider this a good kind of pressure.)


I suppose my main problem is that I’m just lucky that I can’t remember a
case where the block subsystem was really broken and iotests in make
check would have caught it; and that I’m naïve enough to expect this to
continue into the future.

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 5649 bytes --]

On 04/10/2019 14.44, Max Reitz wrote:
> On 04.10.19 12:19, Kevin Wolf wrote:
>> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
>>> On 02.10.19 18:44, Kevin Wolf wrote:
>>>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>>>>> It usually worked fine for me because it’s rather rare that non-block
>>>>> patches broke the iotests.
>>>>
>>>> I disagree. It happened all the time that someone else broke the iotests
>>>> in master and we needed to fix it up.
>>>
>>> OK.
>>>
>>> (In my experience, it’s still mostly block patches, only that they tend
>>> to go through non-block trees.)
>>
>> Fair enough, it's usually code that touches block code. I assumed "block
>> patches" to mean patches that go through one of the block trees and for
>> which iotests are run before sending a pull request.
>>
>> In the end, I don't care what code these patches touched. I do an
>> innocent git pull, and when I finally see that it's master that breaks
>> iotests and not my patches on top of it, I'm annoyed.
> 
> Hm.  Part of my point was that this still happens all the time.
> 
> Which is why I’d prefer more tests to run in CI than a handful of not
> very useful ones in make check.

Ok, so let's try to add some more useful test to the "auto" group. Kevin
mentioned 030, 040 and 041, and I think it should be ok to enable them
(IIRC the only issue was that they run a little bit longer, but if they
are very useful, we should include them anyway).

Do you have any other tests in mind which could be very useful?

[...]
>> So if you merge that revert, when iotests break in master, I take it I
>> can rely on you to fix it up before it impacts my working branch?
> 
> This is not a game, I’m talking purely from my standpoint:
> (I talked wrongly, but more on that below)
> 
> Whenever make check fails, it’s urgent.  Without iotests running in make
> check, we had some time to sort the situation out.  That’s no longer the
> case.
>
> That means we need to take care of everything immediately.  And I purely
> want help with that.

While that sounds noble at a first glance, I think you've maybe got too
high expectations at yourself here? I mean, all other maintainers of
other subsystems with tests have the same "problem" if the tests for
their subsystem run in "make check". The "normal" behavior that I've
seen so far (and which I think is the right way to deal with it):

1) If a pull request gets rejected due to a "make check" failure, simply
drop the offending patches from the pull request, send a v2 pull req
without them, and tell the author of the offending patches to fix the
problem (unless the fix is completely trivial and could immediately be
applied to the v2 pull req). You really don't have to fix each and every
issue on your own as a maintainer and can redirect this to the patch
authors again.

2) If a test fails occasionally during "make check" (due to race
conditions or whatever), it gets disabled from "make check" if it can't
be fixed easily (e.g. in the qtests it gets moved to the "SPEED=slow"
category, or in iotests it would get removed from the "auto" group).

>> Yes, making iotests stable on CI probably involves some pain, especially
>> initially. However, if we don't fix them upstream, they'll end up on our
>> desk again downstream because people run tests neither on your nor on my
>> laptop.
>>
>> I think we might actually save time by fixing them once and for all,
>> even if they are problems in the test cases and not in QEMU, and making
>> new test cases portable from the start, instead of getting individual
>> reports one at a time and having to look at the same test cases again
>> and again.
> 
> You should really know I’m all for that and that I’ve done my share of
> work there.
> 
> But from my perspective we’ve said and tried that for six years and we
> aren’t there still.  So to me “We should just fix it” of course sounds
> correct, but I also don’t believe it’ll happen any time soon.  Mostly
> because we generally don’t know what to fix before it breaks, but also
> because that’s exactly what we’ve tried to do for at least six years.

Well, I guess we won't ever get there if we don't try. And the hurdles
will rather get higher over the years since more and more tests are
added ...

> OTOH, keeping the iotests in make check means we have to act on failure
> reports immediately.  For example, someone™ needs to investigate the 130
> failure Peter has reported.  I’ve just replied “I don’t know”, CC’d
> Thomas, and waited whether anyone else would do anything.  Nobody did,
> and that can’t work.  (I also wrote that I wouldn’t act on it, precisely
> because I didn’t see the point.  I assumed that if anyone disagreed with
> that statement, they would have said something.)
> 
> So acting on such reports means pain, too.  I consider it greater than
> the previous kind of pain, because I prefer “This sucks, I’ll have to
> fix it this week or so” to “Oh crap, I’ll have to investigate now,
> reproduce it, write an email as soon as possible, and fix it”.

I think that "I have to investigate now ..." mindset is not the right
way to handle these kind of issues. If a test is shaky and can not be
fixed easily, we should simply disable it from the "auto" group again.
So if you like, I can send a patch to remove 130 from the "auto" group
(personally, I'd rather wait to see if it fails anytime soon again, or
if this was maybe rather a one-time issue due to a non-clean test system
...)

 Thomas


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Peter Maydell]

On Mon, 7 Oct 2019 at 13:32, Thomas Huth <thuth@redhat.com> wrote:
> On 04/10/2019 14.44, Max Reitz wrote:
> > Whenever make check fails, it’s urgent.  Without iotests running in make
> > check, we had some time to sort the situation out.  That’s no longer the
> > case.
> >
> > That means we need to take care of everything immediately.  And I purely
> > want help with that.
>
> While that sounds noble at a first glance, I think you've maybe got too
> high expectations at yourself here? I mean, all other maintainers of
> other subsystems with tests have the same "problem" if the tests for
> their subsystem run in "make check". The "normal" behavior that I've
> seen so far (and which I think is the right way to deal with it):
>
> 1) If a pull request gets rejected due to a "make check" failure, simply
> drop the offending patches from the pull request, send a v2 pull req
> without them, and tell the author of the offending patches to fix the
> problem (unless the fix is completely trivial and could immediately be
> applied to the v2 pull req). You really don't have to fix each and every
> issue on your own as a maintainer and can redirect this to the patch
> authors again.
>
> 2) If a test fails occasionally during "make check" (due to race
> conditions or whatever), it gets disabled from "make check" if it can't
> be fixed easily (e.g. in the qtests it gets moved to the "SPEED=slow"
> category, or in iotests it would get removed from the "auto" group).

I would agree with this and would also suggest that things tested
in 'make check' should ideally be reducing work for the maintainer:
if something fails 'make check' then that change won't get into
the tree and the task of getting it to work is pushed back to the
original patch submitter. If something causes failures but they're
not caught by 'make check' then the patch will get into the tree and
now it's likely the subsystem maintainer's job to track down and
fix the bug after the fact.

> So if you like, I can send a patch to remove 130 from the "auto" group
> (personally, I'd rather wait to see if it fails anytime soon again, or
> if this was maybe rather a one-time issue due to a non-clean test system
> ...)

FWIW I haven't seen that iotest 130 failure again...

thanks
-- PMM

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7832 bytes --]

On 07.10.19 14:16, Thomas Huth wrote:
> On 04/10/2019 14.44, Max Reitz wrote:
>> On 04.10.19 12:19, Kevin Wolf wrote:
>>> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
>>>> On 02.10.19 18:44, Kevin Wolf wrote:
>>>>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>>>>>> It usually worked fine for me because it’s rather rare that non-block
>>>>>> patches broke the iotests.
>>>>>
>>>>> I disagree. It happened all the time that someone else broke the iotests
>>>>> in master and we needed to fix it up.
>>>>
>>>> OK.
>>>>
>>>> (In my experience, it’s still mostly block patches, only that they tend
>>>> to go through non-block trees.)
>>>
>>> Fair enough, it's usually code that touches block code. I assumed "block
>>> patches" to mean patches that go through one of the block trees and for
>>> which iotests are run before sending a pull request.
>>>
>>> In the end, I don't care what code these patches touched. I do an
>>> innocent git pull, and when I finally see that it's master that breaks
>>> iotests and not my patches on top of it, I'm annoyed.
>>
>> Hm.  Part of my point was that this still happens all the time.
>>
>> Which is why I’d prefer more tests to run in CI than a handful of not
>> very useful ones in make check.
> 
> Ok, so let's try to add some more useful test to the "auto" group. Kevin
> mentioned 030, 040 and 041, and I think it should be ok to enable them
> (IIRC the only issue was that they run a little bit longer, but if they
> are very useful, we should include them anyway).

I agree on those.  (Maybe not 040, but definitely 030 and 041.)

Maybe one of the issues was the “path too long” thing for Unix sockets?

> Do you have any other tests in mind which could be very useful?

I’d like a test for iothreads, it doesn’t look like there currently is
one in auto.  (The problem of course is that they have a higher chance
of being flaky, but I think they also have a higher probability of
breaking because of non-block stuff.)

127 and 256 look promising to me.


Also, I don’t see any migration tests in auto (156 doesn’t really count).

The ones that looks interesting here are 091, 181 (but I seem to
remember that 181 is flaky under load, I should investigate that), 183,
and 203 (which also tests iothreads).


Those are the two area that I spontaneously think of when considering
what is more likely to be broken by non-block patches.  Unfortunately,
those are also the two areas with the tests that tend to be the
flakiest, I guess...

> [...]
>>> So if you merge that revert, when iotests break in master, I take it I
>>> can rely on you to fix it up before it impacts my working branch?
>>
>> This is not a game, I’m talking purely from my standpoint:
>> (I talked wrongly, but more on that below)
>>
>> Whenever make check fails, it’s urgent.  Without iotests running in make
>> check, we had some time to sort the situation out.  That’s no longer the
>> case.
>>
>> That means we need to take care of everything immediately.  And I purely
>> want help with that.
> 
> While that sounds noble at a first glance, I think you've maybe got too
> high expectations at yourself here? I mean, all other maintainers of
> other subsystems with tests have the same "problem" if the tests for
> their subsystem run in "make check".

The difference is that the iotests generally seem much less
deterministic to me than the other tests we have.

> The "normal" behavior that I've
> seen so far (and which I think is the right way to deal with it):
> 
> 1) If a pull request gets rejected due to a "make check" failure, simply
> drop the offending patches from the pull request, send a v2 pull req
> without them, and tell the author of the offending patches to fix the
> problem (unless the fix is completely trivial and could immediately be
> applied to the v2 pull req). You really don't have to fix each and every
> issue on your own as a maintainer and can redirect this to the patch
> authors again.
> 
> 2) If a test fails occasionally during "make check" (due to race
> conditions or whatever), it gets disabled from "make check" if it can't
> be fixed easily (e.g. in the qtests it gets moved to the "SPEED=slow"
> category, or in iotests it would get removed from the "auto" group).

Well, we’ll see how it goes.  I suppose in practice it won’t be too big
of a problem to just temporarily remove tests from auto if the issue
isn’t clear immediately but the test does seem important.  (I don’t
think there’s too much danger of forgetting them.)

>>> Yes, making iotests stable on CI probably involves some pain, especially
>>> initially. However, if we don't fix them upstream, they'll end up on our
>>> desk again downstream because people run tests neither on your nor on my
>>> laptop.
>>>
>>> I think we might actually save time by fixing them once and for all,
>>> even if they are problems in the test cases and not in QEMU, and making
>>> new test cases portable from the start, instead of getting individual
>>> reports one at a time and having to look at the same test cases again
>>> and again.
>>
>> You should really know I’m all for that and that I’ve done my share of
>> work there.
>>
>> But from my perspective we’ve said and tried that for six years and we
>> aren’t there still.  So to me “We should just fix it” of course sounds
>> correct, but I also don’t believe it’ll happen any time soon.  Mostly
>> because we generally don’t know what to fix before it breaks, but also
>> because that’s exactly what we’ve tried to do for at least six years.
> 
> Well, I guess we won't ever get there if we don't try.

That was my point, that we have tried.  It isn’t like we’ve failed, it’s
just that it looks like a never-ending mission to me.

And it isn’t like I’m not working on improving the situation, even when
I’m saying that I don’t think it’ll ever be perfectly deterministic.

> And the hurdles
> will rather get higher over the years since more and more tests are
> added ...
> 
>> OTOH, keeping the iotests in make check means we have to act on failure
>> reports immediately.  For example, someone™ needs to investigate the 130
>> failure Peter has reported.  I’ve just replied “I don’t know”, CC’d
>> Thomas, and waited whether anyone else would do anything.  Nobody did,
>> and that can’t work.  (I also wrote that I wouldn’t act on it, precisely
>> because I didn’t see the point.  I assumed that if anyone disagreed with
>> that statement, they would have said something.)
>>
>> So acting on such reports means pain, too.  I consider it greater than
>> the previous kind of pain, because I prefer “This sucks, I’ll have to
>> fix it this week or so” to “Oh crap, I’ll have to investigate now,
>> reproduce it, write an email as soon as possible, and fix it”.
> 
> I think that "I have to investigate now ..." mindset is not the right
> way to handle these kind of issues. If a test is shaky and can not be
> fixed easily, we should simply disable it from the "auto" group again.
> So if you like, I can send a patch to remove 130 from the "auto" group
> (personally, I'd rather wait to see if it fails anytime soon again, or
> if this was maybe rather a one-time issue due to a non-clean test system
> ...)

Waiting for another failure sounds OK to me.  (OTOH, 130 doesn’t seem
like something that needs to be in auto, in case we want to take
something out to save time for more important tests.)

But that reminds me that iotest failures probably should be
automatically signaled to me and Kevin instead of Peter having to write
an email himself.  Would that be possible?

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 5006 bytes --]

On 07/10/2019 14.52, Max Reitz wrote:
> On 07.10.19 14:16, Thomas Huth wrote:
>> On 04/10/2019 14.44, Max Reitz wrote:
>>> On 04.10.19 12:19, Kevin Wolf wrote:
>>>> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
>>>>> On 02.10.19 18:44, Kevin Wolf wrote:
>>>>>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>>>>>>> It usually worked fine for me because it’s rather rare that non-block
>>>>>>> patches broke the iotests.
>>>>>>
>>>>>> I disagree. It happened all the time that someone else broke the iotests
>>>>>> in master and we needed to fix it up.
>>>>>
>>>>> OK.
>>>>>
>>>>> (In my experience, it’s still mostly block patches, only that they tend
>>>>> to go through non-block trees.)
>>>>
>>>> Fair enough, it's usually code that touches block code. I assumed "block
>>>> patches" to mean patches that go through one of the block trees and for
>>>> which iotests are run before sending a pull request.
>>>>
>>>> In the end, I don't care what code these patches touched. I do an
>>>> innocent git pull, and when I finally see that it's master that breaks
>>>> iotests and not my patches on top of it, I'm annoyed.
>>>
>>> Hm.  Part of my point was that this still happens all the time.
>>>
>>> Which is why I’d prefer more tests to run in CI than a handful of not
>>> very useful ones in make check.
>>
>> Ok, so let's try to add some more useful test to the "auto" group. Kevin
>> mentioned 030, 040 and 041, and I think it should be ok to enable them
>> (IIRC the only issue was that they run a little bit longer, but if they
>> are very useful, we should include them anyway).
> 
> I agree on those.  (Maybe not 040, but definitely 030 and 041.)
> 
> Maybe one of the issues was the “path too long” thing for Unix sockets?

Ah, right. I've applied John's "remove 'linux' from default" patch and
added the three iotests to the "auto" group, and indeed, they fail now
on cirrus-ci due to the "path too long" socket problem. "We" (royal we,
I guess) should likely fix that first...

>> Do you have any other tests in mind which could be very useful?
> 
> I’d like a test for iothreads, it doesn’t look like there currently is
> one in auto.  (The problem of course is that they have a higher chance
> of being flaky, but I think they also have a higher probability of
> breaking because of non-block stuff.)
> 
> 127 and 256 look promising to me.
> 
> 
> Also, I don’t see any migration tests in auto (156 doesn’t really count).
> 
> The ones that looks interesting here are 091, 181 (but I seem to
> remember that 181 is flaky under load, I should investigate that), 183,
> and 203 (which also tests iothreads).
> 
> 
> Those are the two area that I spontaneously think of when considering
> what is more likely to be broken by non-block patches.  Unfortunately,
> those are also the two areas with the tests that tend to be the
> flakiest, I guess...

Ok, thanks for the list. I'll have a look at them whether it's feasible
to get them enabled...

>>> OTOH, keeping the iotests in make check means we have to act on failure
>>> reports immediately.  For example, someone™ needs to investigate the 130
>>> failure Peter has reported.  I’ve just replied “I don’t know”, CC’d
>>> Thomas, and waited whether anyone else would do anything.  Nobody did,
>>> and that can’t work.  (I also wrote that I wouldn’t act on it, precisely
>>> because I didn’t see the point.  I assumed that if anyone disagreed with
>>> that statement, they would have said something.)
>>>
>>> So acting on such reports means pain, too.  I consider it greater than
>>> the previous kind of pain, because I prefer “This sucks, I’ll have to
>>> fix it this week or so” to “Oh crap, I’ll have to investigate now,
>>> reproduce it, write an email as soon as possible, and fix it”.
>>
>> I think that "I have to investigate now ..." mindset is not the right
>> way to handle these kind of issues. If a test is shaky and can not be
>> fixed easily, we should simply disable it from the "auto" group again.
>> So if you like, I can send a patch to remove 130 from the "auto" group
>> (personally, I'd rather wait to see if it fails anytime soon again, or
>> if this was maybe rather a one-time issue due to a non-clean test system
>> ...)
> 
> Waiting for another failure sounds OK to me.  (OTOH, 130 doesn’t seem
> like something that needs to be in auto, in case we want to take
> something out to save time for more important tests.)
> 
> But that reminds me that iotest failures probably should be
> automatically signaled to me and Kevin instead of Peter having to write
> an email himself.  Would that be possible?

Not sure whether an automatic e-mail is feasible, but you could register
a github account to test with Travis (and maybe Cirrus-CI) before
sending pull requests to Peter, I guess that will catch most of the
issues on other machines already.

 Thomas


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 2396 bytes --]

On 07/10/2019 15.11, Thomas Huth wrote:
> On 07/10/2019 14.52, Max Reitz wrote:
>> On 07.10.19 14:16, Thomas Huth wrote:
>>> On 04/10/2019 14.44, Max Reitz wrote:
>>>> On 04.10.19 12:19, Kevin Wolf wrote:
>>>>> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
>>>>>> On 02.10.19 18:44, Kevin Wolf wrote:
>>>>>>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>>>>>>>> It usually worked fine for me because it’s rather rare that non-block
>>>>>>>> patches broke the iotests.
>>>>>>>
>>>>>>> I disagree. It happened all the time that someone else broke the iotests
>>>>>>> in master and we needed to fix it up.
>>>>>>
>>>>>> OK.
>>>>>>
>>>>>> (In my experience, it’s still mostly block patches, only that they tend
>>>>>> to go through non-block trees.)
>>>>>
>>>>> Fair enough, it's usually code that touches block code. I assumed "block
>>>>> patches" to mean patches that go through one of the block trees and for
>>>>> which iotests are run before sending a pull request.
>>>>>
>>>>> In the end, I don't care what code these patches touched. I do an
>>>>> innocent git pull, and when I finally see that it's master that breaks
>>>>> iotests and not my patches on top of it, I'm annoyed.
>>>>
>>>> Hm.  Part of my point was that this still happens all the time.
>>>>
>>>> Which is why I’d prefer more tests to run in CI than a handful of not
>>>> very useful ones in make check.
>>>
>>> Ok, so let's try to add some more useful test to the "auto" group. Kevin
>>> mentioned 030, 040 and 041, and I think it should be ok to enable them
>>> (IIRC the only issue was that they run a little bit longer, but if they
>>> are very useful, we should include them anyway).
>>
>> I agree on those.  (Maybe not 040, but definitely 030 and 041.)
>>
>> Maybe one of the issues was the “path too long” thing for Unix sockets?
> 
> Ah, right. I've applied John's "remove 'linux' from default" patch and
> added the three iotests to the "auto" group, and indeed, they fail now
> on cirrus-ci due to the "path too long" socket problem. "We" (royal we,
> I guess) should likely fix that first...

FWIW, 041 also fails on macOS on Travis (which does not have the "path
too long" issue):

 https://travis-ci.com/huth/qemu/jobs/242942716#L8415

... so we might need to declare this as "linux only" again after John's
patch gets merged.

 Thomas


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2686 bytes --]

On 07.10.19 18:28, Thomas Huth wrote:
> On 07/10/2019 15.11, Thomas Huth wrote:
>> On 07/10/2019 14.52, Max Reitz wrote:
>>> On 07.10.19 14:16, Thomas Huth wrote:
>>>> On 04/10/2019 14.44, Max Reitz wrote:
>>>>> On 04.10.19 12:19, Kevin Wolf wrote:
>>>>>> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
>>>>>>> On 02.10.19 18:44, Kevin Wolf wrote:
>>>>>>>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>>>>>>>>> It usually worked fine for me because it’s rather rare that non-block
>>>>>>>>> patches broke the iotests.
>>>>>>>>
>>>>>>>> I disagree. It happened all the time that someone else broke the iotests
>>>>>>>> in master and we needed to fix it up.
>>>>>>>
>>>>>>> OK.
>>>>>>>
>>>>>>> (In my experience, it’s still mostly block patches, only that they tend
>>>>>>> to go through non-block trees.)
>>>>>>
>>>>>> Fair enough, it's usually code that touches block code. I assumed "block
>>>>>> patches" to mean patches that go through one of the block trees and for
>>>>>> which iotests are run before sending a pull request.
>>>>>>
>>>>>> In the end, I don't care what code these patches touched. I do an
>>>>>> innocent git pull, and when I finally see that it's master that breaks
>>>>>> iotests and not my patches on top of it, I'm annoyed.
>>>>>
>>>>> Hm.  Part of my point was that this still happens all the time.
>>>>>
>>>>> Which is why I’d prefer more tests to run in CI than a handful of not
>>>>> very useful ones in make check.
>>>>
>>>> Ok, so let's try to add some more useful test to the "auto" group. Kevin
>>>> mentioned 030, 040 and 041, and I think it should be ok to enable them
>>>> (IIRC the only issue was that they run a little bit longer, but if they
>>>> are very useful, we should include them anyway).
>>>
>>> I agree on those.  (Maybe not 040, but definitely 030 and 041.)
>>>
>>> Maybe one of the issues was the “path too long” thing for Unix sockets?
>>
>> Ah, right. I've applied John's "remove 'linux' from default" patch and
>> added the three iotests to the "auto" group, and indeed, they fail now
>> on cirrus-ci due to the "path too long" socket problem. "We" (royal we,
>> I guess) should likely fix that first...

I’m looking into that, by the way.  I hope a SOCK_DIR that defaults to
$(mktemp -d) will suffice...

> FWIW, 041 also fails on macOS on Travis (which does not have the "path
> too long" issue):
> 
>  https://travis-ci.com/huth/qemu/jobs/242942716#L8415
> 
> ... so we might need to declare this as "linux only" again after John's
> patch gets merged.

:/

Either that, or let make check skip the iotests generally on non-Linux
systems.

Max


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 1656 bytes --]

On 02/10/2019 18.44, Kevin Wolf wrote:
> Not sure where in this thread to reply, but since my name is mentioned
> in this mail, it might at least be not the worst one.
> 
> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>> On 02.10.19 06:46, Thomas Huth wrote:
>>> On 01/10/2019 20.44, Max Reitz wrote:
>>> [...]
>>>> As I have said, the conceptual problem is that the iotests now run as
>>>> part of make check.  As such, allowing auto tests to run on non-Linux
>>>> platforms may introduce build failures that I cannot do anything about.
>>>
>>> Well, simply run "make vm-build-openbsd", "make vm-build-freebsd", ...
>>> if something fails there, it likely should not be in the auto group.
>>
>> Then we come to Windows and macOS.
>>
>> What I’ve proposed to John on IRC was to simply skip the iotests in make
>> check for non-Linux systems.
> 
> I think this really makes sense. Or at least have a blacklist of OSes
> that we can't support iotests on, which would contain at least Windows
> and OS X. Windows because I'm pretty sure that it doesn't work anyway,
> and OS X because if something fails there, we have no way to reproduce.

Both, .travis.yml and .cirrus-ci.yml have a macOS test, so you can
reproduce bugs there. It's just a PITA that you cannot do any
interactive debugging there.

[...]
> Can we run tests in 'auto' that require the system emulator?

Yes. In fact, tests/check-block.sh explicitly checks for the
availability of a system emulator before running the iotests.

> If so, let's add 030 040 041 to the default set.
Sure, but let's check whether they work in Travis first.

 Thomas


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

Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[John Snow]

On 10/1/19 2:44 PM, Max Reitz wrote:
> On 28.09.19 01:35, John Snow wrote:
>>
>>
>> On 9/23/19 9:09 AM, Max Reitz wrote:
>>> On 18.09.19 01:45, John Snow wrote:
>>>> verify_platform will check an explicit whitelist and blacklist instead.
>>>> The default will now be assumed to be allowed to run anywhere.
>>>>
>>>> For tests that do not specify their platforms explicitly, this has the effect of
>>>> enabling these tests on non-linux platforms. For tests that always specified
>>>> linux explicitly, there is no change.
>>>>
>>>> For Python tests on FreeBSD at least; only seven python tests fail:
>>>> 045 147 149 169 194 199 211
>>>>
>>>> 045 and 149 appear to be misconfigurations,
>>>> 147 and 194 are the AF_UNIX path too long error,
>>>> 169 and 199 are bitmap migration bugs, and
>>>> 211 is a bug that shows up on Linux platforms, too.
>>>>
>>>> This is at least good evidence that these tests are not Linux-only. If
>>>> they aren't suitable for other platforms, they should be disabled on a
>>>> per-platform basis as appropriate.
>>>>
>>>> Therefore, let's switch these on and deal with the failures.
>>>
>>> What exactly do you mean by “deal with the failures”?  Do you have a
>>> reference to patches that deal with them, or are you or is someone else
>>> working on them...?
>>>
>>> Apart from that, I am rather hesitant to take a patch through my tree
>>> that not only may cause test failures on platforms that I will not or
>>> actually cannot run tests on (like MacOS or Windows), but that actually
>>> does introduce new failures as you describe.
>>>
>>> Well, at least it doesn’t introduce build failures because it appears
>>> there is no Python test that’s in the auto group, so I suppose “rather
>>> hesitant” is not an “I won’t”.
>>>
>>
>> Think of it more like this: The failures were always there, but we hid
>> them. I'm not "introducing new failures" as such O:-)
> 
> That is incorrect.
> 
> As I have said, the conceptual problem is that the iotests now run as
> part of make check.  As such, allowing auto tests to run on non-Linux
> platforms may introduce build failures that I cannot do anything about.
> 
> And those are very much new failures.
> 
>> I think that I have demonstrated sufficiently that it's not correct to
>> prohibit python tests from running on other platforms wholesale, so I'd
>> prefer we don't do that anymore.
> 
> You have not.
> 

I feel as if I have.

The tests can run on other platforms and I proved that most of them
work. There's not good evidence that any of these tests are indeed
"Linux-only", as only a handful experience problems not more easily
explained by other factors. There's nothing inherent to the framework
itself that makes it Linux-only.

I think it's more than accurate to say that "it's not correct to
prohibit python tests from running on other platforms wholesale."

Maybe you want to talk about whether or not we should gate on these
tests on those platforms, and that's fair, but it's not what got written.

> The actual argument to convince me is “This does not affect any tests in
> the auto group, so it will not introduce build failures at this time”.
> 
>> Further, iotests on FreeBSD already weren't 100% green, so I'm not
>> causing a regression in that sense, either.
> 
> My problem is twofold:
> 
> (1) You claim “Sure, there are failures, but let’s just deal with them”
> and then you do not deal with them.  Seems wrong to me.
> 
> I’m fine with the argument “Sorry, royal ‘we’.  But it just doesn’t help
> anyone to hide the errors.  If someone’s on BSD and wants to run the
> iotests, let them.”
> 
> That sounds good to me.
> 

That's more or less what I want to convey. Enable them and see where the
chips fall, but don't gate pull requests on non-Linux platforms.

In the event that an "auto" group test did fail on an esoteric platform,
I wanted to offer being the point of contact for that so I wasn't
foisting work onto you.

"Royal 'we', but with the expectation that I'd likely just re-blacklist
certain tests we don't have the capacity to support."

I felt like the burden wouldn't be too severe there, as all of the
'auto' tests appeared to pass without much of a fuss.

> (2) Maybe someone adds a Python test in the future that is in auto and
> that does not specify Linux as the only supported platform.  Then I send
> a pull request and it breaks on macOS.  Now what?  Remove it from auto?
>  Blindly put "macOS" in unsupported platforms?
> 
> In any case, it’ll be a problem for no good reason.
> 

I don't agree with the sentiment that it's "no good reason".

We claim to support these platforms. If tests fail on them, I think we
rather want to know.

The fear, I think, is that it will be mostly boring platform differences
that are unimportant to the actual functioning of QEMU: that it's just
good at finding bugs in the test instead of the binary.

I think that's a legitimate concern, but it doesn't move me enough to
agree that we shouldn't run tests on platforms we claim to support.

Not that I wanted to wage this war anyway. I just wanted to fix some
Python logging.

> More on that in the next chunk.
> 
>> I'm going to meekly push and ask that we stage this as-is, and when
>> something bad happens you can remind me that I wanted this and make me
>> do it.
> 
> Make you do what?  Deal with the fact that a pull request is rejected
> because a test fails on macOS?
> 
> This is precisely the kind of problem I already had with adding the
> iotests to make check, and I’m already very much not happy about it.
> (In that case it was $DISPLAY not being set on Peter’s test system.)
> 

Make me "deal with it", is what I meant. I am willing to own
multiplatform problems for the block layer, but I'm also constrained by
some other simple facts like "I don't have a Mac OSX machine."

I am having a difficult time advocating for the idea that we have
supported platforms on which we don't actually test. If we disable tests
for those platforms, I think it's not right to say those platforms are
supported.

I will say, though, you make an extremely convincing case for the idea
that, actually, our only supported platform is Linux.

> 
> I’ll let you make the deduction of “The problem isn’t allowing the
> iotests to run on non-Linux platforms, but the fact that they run in
> make check” yourself, so that I no longer feel like I’m the only one who
> considers that having been a mistake.
> 

I'd be OK with not running them in "make check" on non-Linux anymore,
because it matches my feelings on them not being robust enough to gate
on for that platform.

It looks like we're just going to drop the idea entirely, though. As
long as the iotests are getting run regularly I will be happy anyway,
and I suppose we're looking at Travis for that now.

With that in mind, it's probably fine to remove the restrictions here, I
hope. The alternative is working it back the other way and adding new
restrictions to existing tests to force it back to Linux-only, and I'd
prefer not to do that.

https://i.kym-cdn.com/photos/images/original/001/365/753/94c.jpg

--js

[Qemu-devel] [PATCH v5 2/5] iotests: add script_initialize

[John Snow]

Like script_main, but doesn't require a single point of entry.
Replace all existing initialization sections with this drop-in replacement.

This brings debug support to all existing script-style iotests.

Any specification for supported_oses=['linux'] was dropped as explained
in the previous commit, because there was never any reason to limit python
tests to linux-only.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/149        |  3 +-
 tests/qemu-iotests/194        |  3 +-
 tests/qemu-iotests/202        |  3 +-
 tests/qemu-iotests/203        |  3 +-
 tests/qemu-iotests/206        |  2 +-
 tests/qemu-iotests/207        |  2 +-
 tests/qemu-iotests/208        |  2 +-
 tests/qemu-iotests/209        |  2 +-
 tests/qemu-iotests/210        |  2 +-
 tests/qemu-iotests/211        |  2 +-
 tests/qemu-iotests/212        |  2 +-
 tests/qemu-iotests/213        |  2 +-
 tests/qemu-iotests/216        |  3 +-
 tests/qemu-iotests/218        |  2 +-
 tests/qemu-iotests/219        |  2 +-
 tests/qemu-iotests/222        |  5 ++-
 tests/qemu-iotests/224        |  3 +-
 tests/qemu-iotests/228        |  3 +-
 tests/qemu-iotests/234        |  3 +-
 tests/qemu-iotests/235        |  4 +--
 tests/qemu-iotests/236        |  2 +-
 tests/qemu-iotests/237        |  2 +-
 tests/qemu-iotests/238        |  2 ++
 tests/qemu-iotests/242        |  2 +-
 tests/qemu-iotests/246        |  2 +-
 tests/qemu-iotests/248        |  2 +-
 tests/qemu-iotests/254        |  2 +-
 tests/qemu-iotests/255        |  2 +-
 tests/qemu-iotests/256        |  2 +-
 tests/qemu-iotests/262        |  3 +-
 tests/qemu-iotests/iotests.py | 62 ++++++++++++++++++++++-------------
 31 files changed, 73 insertions(+), 63 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 4f363f295f..9fa97966c5 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -383,8 +383,7 @@ def test_once(config, qemu_img=False):
 
 
 # Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
 
 # We need sudo in order to run cryptsetup to create
 # dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index d746ab1e21..c8aeb6d0e4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'])
 
 with iotests.FilePath('source.img') as source_img_path, \
      iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 581ca34d79..1271ac9459 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
      iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 4874a1a0d8..c40fe231ea 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
      iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index 5bb738bf23..23ff2f624b 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create',
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index ec8c1d06f0..ab9e3b6747 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,7 +24,7 @@ import iotests
 import subprocess
 import re
 
-iotests.verify_image_format(supported_fmts=['raw'])
+iotests.script_initialize(supported_fmts=['raw'])
 iotests.verify_protocol(supported=['ssh'])
 
 def filter_hash(qmsg):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1e202388dc..dfce6f9fe4 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['generic'])
+iotests.script_initialize(supported_fmts=['generic'])
 
 with iotests.FilePath('disk.img') as disk_img_path, \
      iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209
index 259e991ec6..a77f884166 100755
--- a/tests/qemu-iotests/209
+++ b/tests/qemu-iotests/209
@@ -22,7 +22,7 @@ import iotests
 from iotests import qemu_img_create, qemu_io, qemu_img_verbose, qemu_nbd, \
                     file_path
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 disk, nbd_sock = file_path('disk', 'nbd-sock')
 nbd_uri = 'nbd+unix:///exp?socket=' + nbd_sock
diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 565e3b7b9b..5a7013cd34 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['luks'])
+iotests.script_initialize(supported_fmts=['luks'])
 iotests.verify_protocol(supported=['file'])
 
 def blockdev_create(vm, options):
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index 6afc894f76..4d6aac497f 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['vdi'])
+iotests.script_initialize(supported_fmts=['vdi'])
 iotests.verify_protocol(supported=['file'])
 
 def blockdev_create(vm, options):
diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
index 42b74f208b..ec35bceb11 100755
--- a/tests/qemu-iotests/212
+++ b/tests/qemu-iotests/212
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['parallels'])
+iotests.script_initialize(supported_fmts=['parallels'])
 iotests.verify_protocol(supported=['file'])
 
 def blockdev_create(vm, options):
diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
index 5604f3cebb..3d2c024375 100755
--- a/tests/qemu-iotests/213
+++ b/tests/qemu-iotests/213
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['vhdx'])
+iotests.script_initialize(supported_fmts=['vhdx'])
 iotests.verify_protocol(supported=['file'])
 
 def blockdev_create(vm, options):
diff --git a/tests/qemu-iotests/216 b/tests/qemu-iotests/216
index 3c0ae54b44..7574bcc09f 100755
--- a/tests/qemu-iotests/216
+++ b/tests/qemu-iotests/216
@@ -23,8 +23,7 @@ import iotests
 from iotests import log, qemu_img, qemu_io_silent
 
 # Need backing file support
-iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'])
 
 log('')
 log('=== Copy-on-read across nodes ===')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 2554d84581..e18e31076b 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -29,7 +29,7 @@
 import iotests
 from iotests import log, qemu_img, qemu_io_silent
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'raw'])
+iotests.script_initialize(supported_fmts=['qcow2', 'raw'])
 
 
 # Launches the VM, adds two null-co nodes (source and target), and
diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
index e0c51662c0..9ae27cb04e 100755
--- a/tests/qemu-iotests/219
+++ b/tests/qemu-iotests/219
@@ -21,7 +21,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 img_size = 4 * 1024 * 1024
 
diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 0ead56d574..6788979ed3 100644
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -24,9 +24,8 @@
 import iotests
 from iotests import log, qemu_img, qemu_io, qemu_io_silent
 
-iotests.verify_platform(['linux'])
-iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk',
-                                            'vhdx', 'raw'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk',
+                                          'vhdx', 'raw'])
 
 patterns = [("0x5d", "0",         "64k"),
             ("0xd5", "1M",        "64k"),
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
index b4dfaa639f..d0d0c44104 100755
--- a/tests/qemu-iotests/224
+++ b/tests/qemu-iotests/224
@@ -26,8 +26,7 @@ from iotests import log, qemu_img, qemu_io_silent, filter_qmp_testfiles, \
 import json
 
 # Need backing file support (for arbitrary backing formats)
-iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed'])
 
 
 # There are two variations of this test:
diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228
index 9a50afd205..9785868ab3 100755
--- a/tests/qemu-iotests/228
+++ b/tests/qemu-iotests/228
@@ -25,8 +25,7 @@ from iotests import log, qemu_img, filter_testfiles, filter_imgfmt, \
         filter_qmp_testfiles, filter_qmp_imgfmt
 
 # Need backing file and change-backing-file support
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed'])
 
 
 def log_node_info(node):
diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
index 34c818c485..3de6ab2341 100755
--- a/tests/qemu-iotests/234
+++ b/tests/qemu-iotests/234
@@ -23,8 +23,7 @@
 import iotests
 import os
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('img') as img_path, \
      iotests.FilePath('backing') as backing_path, \
diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index fedd111fd4..9e88c65b93 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -27,6 +27,8 @@ sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 
 from qemu.machine import QEMUMachine
 
+iotests.script_initialize(supported_fmts=['qcow2'])
+
 # Note:
 # This test was added to check that mirror dead-lock was fixed (see previous
 # commit before this test addition).
@@ -40,8 +42,6 @@ from qemu.machine import QEMUMachine
 
 size = 1 * 1024 * 1024 * 1024
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-
 disk = file_path('disk')
 
 # prepare source image
diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
index 79a6381f8e..b88779eb0b 100755
--- a/tests/qemu-iotests/236
+++ b/tests/qemu-iotests/236
@@ -22,7 +22,7 @@
 import iotests
 from iotests import log
 
-iotests.verify_image_format(supported_fmts=['generic'])
+iotests.script_initialize(supported_fmts=['generic'])
 size = 64 * 1024 * 1024
 granularity = 64 * 1024
 
diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
index 06897f8c87..3758ace0bc 100755
--- a/tests/qemu-iotests/237
+++ b/tests/qemu-iotests/237
@@ -24,7 +24,7 @@ import math
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['vmdk'])
+iotests.script_initialize(supported_fmts=['vmdk'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/238 b/tests/qemu-iotests/238
index e5ac2b2ff8..6e27fb40c2 100755
--- a/tests/qemu-iotests/238
+++ b/tests/qemu-iotests/238
@@ -23,6 +23,8 @@ import os
 import iotests
 from iotests import log
 
+iotests.script_initialize()
+
 virtio_scsi_device = iotests.get_virtio_scsi_device()
 
 vm = iotests.VM()
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index c176e92da6..7c2685b4cc 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -24,7 +24,7 @@ import struct
 from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
     file_path, img_info_log, log, filter_qemu_io
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 disk = file_path('disk')
 chunk = 256 * 1024
diff --git a/tests/qemu-iotests/246 b/tests/qemu-iotests/246
index b0997a392f..1d7747d62d 100755
--- a/tests/qemu-iotests/246
+++ b/tests/qemu-iotests/246
@@ -22,7 +22,7 @@
 import iotests
 from iotests import log
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 size = 64 * 1024 * 1024 * 1024
 gran_small = 32 * 1024
 gran_large = 128 * 1024
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index f26b4bb2aa..781b21b227 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -21,7 +21,7 @@
 import iotests
 from iotests import qemu_img_create, qemu_io, file_path, filter_qmp_testfiles
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 source, target = file_path('source', 'target')
 size = 5 * 1024 * 1024
diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
index 09584f3f7d..43b40f4f71 100755
--- a/tests/qemu-iotests/254
+++ b/tests/qemu-iotests/254
@@ -21,7 +21,7 @@
 import iotests
 from iotests import qemu_img_create, file_path, log
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 disk, top = file_path('disk', 'top')
 size = 1024 * 1024
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 3632d507d0..ff16402268 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create',
diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
index c594a43205..d2f9212e5a 100755
--- a/tests/qemu-iotests/256
+++ b/tests/qemu-iotests/256
@@ -23,7 +23,7 @@ import os
 import iotests
 from iotests import log
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 size = 64 * 1024 * 1024
 
 with iotests.FilePath('img0') as img0_path, \
diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index 398f63587e..f0e9d0f8ac 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -23,8 +23,7 @@
 import iotests
 import os
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('img') as img_path, \
      iotests.FilePath('mig_fifo') as fifo, \
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3c8c121fd5..e28d75e018 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -900,7 +900,20 @@ def skip_if_unsupported(required_formats=[], read_only=False):
         return func_wrapper
     return skip_test_decorator
 
-def execute_unittest(output, verbosity, debug):
+def execute_unittest(debug=False):
+    """Executes unittests within the calling module."""
+
+    verbosity = 2 if debug else 1
+
+    if debug:
+        output = sys.stdout
+    elif sys.version_info.major >= 3:
+        output = io.StringIO()
+    else:
+        # io.StringIO is for unicode strings, which is not what
+        # 2.x's test runner emits.
+        output = io.BytesIO()
+
     runner = unittest.TextTestRunner(stream=output, descriptions=True,
                                      verbosity=verbosity)
     try:
@@ -908,16 +921,21 @@ def execute_unittest(output, verbosity, debug):
         # exception
         unittest.main(testRunner=runner)
     finally:
+        # We need to filter out the time taken from the output so that
+        # qemu-iotest can reliably diff the results against master output.
         if not debug:
             sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s',
                                     r'Ran \1 tests', output.getvalue()))
 
-def execute_test(test_function=None,
-                 supported_fmts=[],
-                 supported_platforms=None,
-                 supported_cache_modes=[], unsupported_fmts=[],
-                 supported_protocols=[], unsupported_protocols=[]):
-    """Run either unittest or script-style tests."""
+def execute_setup_common(supported_fmts=[],
+                         supported_platforms=None,
+                         supported_cache_modes=[],
+                         unsupported_fmts=[],
+                         supported_protocols=[],
+                         unsupported_protocols=[]):
+    """
+    Perform necessary setup for either script-style or unittest-style tests.
+    """
 
     # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
     # indicate that we're not being run via "check". There may be
@@ -927,38 +945,38 @@ def execute_test(test_function=None,
         sys.stderr.write('Please run this test via the "check" script\n')
         sys.exit(os.EX_USAGE)
 
-    debug = '-d' in sys.argv
-    verbosity = 1
     verify_image_format(supported_fmts, unsupported_fmts)
     verify_protocol(supported_protocols, unsupported_protocols)
     verify_platform(supported=supported_platforms)
     verify_cache_mode(supported_cache_modes)
 
+    debug = '-d' in sys.argv
     if debug:
-        output = sys.stdout
-        verbosity = 2
         sys.argv.remove('-d')
-    else:
-        # We need to filter out the time taken from the output so that
-        # qemu-iotest can reliably diff the results against master output.
-        if sys.version_info.major >= 3:
-            output = io.StringIO()
-        else:
-            # io.StringIO is for unicode strings, which is not what
-            # 2.x's test runner emits.
-            output = io.BytesIO()
-
     logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
 
+    return debug
+
+def execute_test(test_function=None, *args, **kwargs):
+    """Run either unittest or script-style tests."""
+
+    debug = execute_setup_common(*args, **kwargs)
     if not test_function:
-        execute_unittest(output, verbosity, debug)
+        execute_unittest(debug)
     else:
         test_function()
 
+# This is called from script-style iotests without a single point of entry
+def script_initialize(*args, **kwargs):
+    """Initialize script-style tests without running any tests."""
+    execute_setup_common(*args, **kwargs)
+
+# This is called from script-style iotests with a single point of entry
 def script_main(test_function, *args, **kwargs):
     """Run script-style tests outside of the unittest framework"""
     execute_test(test_function, *args, **kwargs)
 
+# This is called from unittest style iotests
 def main(*args, **kwargs):
     """Run tests using the unittest framework"""
     execute_test(None, *args, **kwargs)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v5 2/5] iotests: add script_initialize

[Vladimir Sementsov-Ogievskiy]

18.09.2019 2:45, John Snow wrote:
> Like script_main, but doesn't require a single point of entry.
> Replace all existing initialization sections with this drop-in replacement.
> 
> This brings debug support to all existing script-style iotests.
> 
> Any specification for supported_oses=['linux'] was dropped as explained
> in the previous commit, because there was never any reason to limit python
> tests to linux-only.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v5 2/5] iotests: add script_initialize

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1933 bytes --]

On 18.09.19 01:45, John Snow wrote:
> Like script_main, but doesn't require a single point of entry.
> Replace all existing initialization sections with this drop-in replacement.
> 
> This brings debug support to all existing script-style iotests.
> 
> Any specification for supported_oses=['linux'] was dropped as explained
> in the previous commit, because there was never any reason to limit python
> tests to linux-only.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/149        |  3 +-
>  tests/qemu-iotests/194        |  3 +-
>  tests/qemu-iotests/202        |  3 +-
>  tests/qemu-iotests/203        |  3 +-
>  tests/qemu-iotests/206        |  2 +-
>  tests/qemu-iotests/207        |  2 +-
>  tests/qemu-iotests/208        |  2 +-
>  tests/qemu-iotests/209        |  2 +-
>  tests/qemu-iotests/210        |  2 +-
>  tests/qemu-iotests/211        |  2 +-
>  tests/qemu-iotests/212        |  2 +-
>  tests/qemu-iotests/213        |  2 +-
>  tests/qemu-iotests/216        |  3 +-
>  tests/qemu-iotests/218        |  2 +-
>  tests/qemu-iotests/219        |  2 +-
>  tests/qemu-iotests/222        |  5 ++-
>  tests/qemu-iotests/224        |  3 +-
>  tests/qemu-iotests/228        |  3 +-
>  tests/qemu-iotests/234        |  3 +-
>  tests/qemu-iotests/235        |  4 +--
>  tests/qemu-iotests/236        |  2 +-
>  tests/qemu-iotests/237        |  2 +-
>  tests/qemu-iotests/238        |  2 ++
>  tests/qemu-iotests/242        |  2 +-
>  tests/qemu-iotests/246        |  2 +-
>  tests/qemu-iotests/248        |  2 +-
>  tests/qemu-iotests/254        |  2 +-
>  tests/qemu-iotests/255        |  2 +-
>  tests/qemu-iotests/256        |  2 +-
>  tests/qemu-iotests/262        |  3 +-
>  tests/qemu-iotests/iotests.py | 62 ++++++++++++++++++++++-------------
>  31 files changed, 73 insertions(+), 63 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [PATCH v5 2/5] iotests: add script_initialize

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2141 bytes --]

On 23.09.19 15:30, Max Reitz wrote:
> On 18.09.19 01:45, John Snow wrote:
>> Like script_main, but doesn't require a single point of entry.
>> Replace all existing initialization sections with this drop-in replacement.
>>
>> This brings debug support to all existing script-style iotests.
>>
>> Any specification for supported_oses=['linux'] was dropped as explained
>> in the previous commit, because there was never any reason to limit python
>> tests to linux-only.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/149        |  3 +-
>>  tests/qemu-iotests/194        |  3 +-
>>  tests/qemu-iotests/202        |  3 +-
>>  tests/qemu-iotests/203        |  3 +-
>>  tests/qemu-iotests/206        |  2 +-
>>  tests/qemu-iotests/207        |  2 +-
>>  tests/qemu-iotests/208        |  2 +-
>>  tests/qemu-iotests/209        |  2 +-
>>  tests/qemu-iotests/210        |  2 +-
>>  tests/qemu-iotests/211        |  2 +-
>>  tests/qemu-iotests/212        |  2 +-
>>  tests/qemu-iotests/213        |  2 +-
>>  tests/qemu-iotests/216        |  3 +-
>>  tests/qemu-iotests/218        |  2 +-
>>  tests/qemu-iotests/219        |  2 +-
>>  tests/qemu-iotests/222        |  5 ++-
>>  tests/qemu-iotests/224        |  3 +-
>>  tests/qemu-iotests/228        |  3 +-
>>  tests/qemu-iotests/234        |  3 +-
>>  tests/qemu-iotests/235        |  4 +--
>>  tests/qemu-iotests/236        |  2 +-
>>  tests/qemu-iotests/237        |  2 +-
>>  tests/qemu-iotests/238        |  2 ++
>>  tests/qemu-iotests/242        |  2 +-
>>  tests/qemu-iotests/246        |  2 +-
>>  tests/qemu-iotests/248        |  2 +-
>>  tests/qemu-iotests/254        |  2 +-
>>  tests/qemu-iotests/255        |  2 +-
>>  tests/qemu-iotests/256        |  2 +-
>>  tests/qemu-iotests/262        |  3 +-
>>  tests/qemu-iotests/iotests.py | 62 ++++++++++++++++++++++-------------
>>  31 files changed, 73 insertions(+), 63 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

(Forgot to add: Needs a bit of rebase on Kevin’s patch to drop Python 2
support, but the changes look obvious enough.)


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

[Qemu-devel] [PATCH v5 3/5] iotest 258: use script_main

[John Snow]

Since this one is nicely factored to use a single entry point,
use script_main to run the tests.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/258 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index b84cf02254..1372522c7a 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -23,11 +23,6 @@ import iotests
 from iotests import log, qemu_img, qemu_io_silent, \
         filter_qmp_testfiles, filter_qmp_imgfmt
 
-# Need backing file and change-backing-file support
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
-iotests.verify_platform(['linux'])
-
-
 # Returns a node for blockdev-add
 def node(node_name, path, backing=None, fmt=None, throttle=None):
     if fmt is None:
@@ -160,4 +155,5 @@ def main():
     test_concurrent_finish(False)
 
 if __name__ == '__main__':
-    main()
+    # Need backing file and change-backing-file support
+    iotests.script_main(main, supported_fmts=['qcow2', 'qed'])
-- 
2.21.0

Re: [PATCH v5 3/5] iotest 258: use script_main

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 410 bytes --]

On 18.09.19 01:45, John Snow wrote:
> Since this one is nicely factored to use a single entry point,
> use script_main to run the tests.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/258 | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 4/5] iotests: specify protocol support via initialization info

[John Snow]

Any one of the iotests.main, iotests.script_main, or
iotests.script_initialize functions can specify verify_protocols.

Remove the last users of calling the function individually.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/207 | 4 ++--
 tests/qemu-iotests/210 | 4 ++--
 tests/qemu-iotests/211 | 4 ++--
 tests/qemu-iotests/212 | 4 ++--
 tests/qemu-iotests/213 | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index ab9e3b6747..35d98f2736 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,8 @@ import iotests
 import subprocess
 import re
 
-iotests.script_initialize(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(supported_fmts=['raw'],
+                          supported_protocols=['ssh'])
 
 def filter_hash(qmsg):
     def _filter(key, value):
diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 5a7013cd34..d9fe780c07 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['luks'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['luks'],
+                          supported_protocols=['file'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index 4d6aac497f..155fac4e87 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['vdi'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['vdi'],
+                          supported_protocols=['file'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
index ec35bceb11..67e5a1dbb5 100755
--- a/tests/qemu-iotests/212
+++ b/tests/qemu-iotests/212
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['parallels'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['parallels'],
+                          supported_protocols=['file'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
index 3d2c024375..23f387ab63 100755
--- a/tests/qemu-iotests/213
+++ b/tests/qemu-iotests/213
@@ -23,8 +23,8 @@
 import iotests
 from iotests import imgfmt
 
-iotests.script_initialize(supported_fmts=['vhdx'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(supported_fmts=['vhdx'],
+                          supported_protocols=['file'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
-- 
2.21.0

Re: [PATCH v5 4/5] iotests: specify protocol support via initialization info

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 642 bytes --]

On 18.09.19 01:45, John Snow wrote:
> Any one of the iotests.main, iotests.script_main, or
> iotests.script_initialize functions can specify verify_protocols.
> 
> Remove the last users of calling the function individually.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/207 | 4 ++--
>  tests/qemu-iotests/210 | 4 ++--
>  tests/qemu-iotests/211 | 4 ++--
>  tests/qemu-iotests/212 | 4 ++--
>  tests/qemu-iotests/213 | 4 ++--
>  5 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH v5 5/5] iotests: use python logging for iotests.log()

[John Snow]

We can turn logging on/off globally instead of per-function.

Remove use_log from run_job, and use python logging to turn on
diffable output when we run through a script entry point.

iotest 245 changes output order due to buffering reasons.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/030        |  4 +--
 tests/qemu-iotests/245        |  1 +
 tests/qemu-iotests/245.out    | 24 ++++++++---------
 tests/qemu-iotests/iotests.py | 49 +++++++++++++++++++++--------------
 4 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index f3766f2a81..01aa96ed16 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
         self.assert_qmp(result, 'return', {})
 
-        self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
-        self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+        self.vm.run_job(job='drive0', auto_dismiss=True)
+        self.vm.run_job(job='node4', auto_dismiss=True)
         self.assert_no_active_block_jobs()
 
     # Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 41218d5f1d..eba2157cff 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1000,5 +1000,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         self.reopen(opts, {'backing': 'hd2'})
 
 if __name__ == '__main__':
+    iotests.activate_logging()
     iotests.main(supported_fmts=["qcow2"],
                  supported_protocols=["file"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index a19de5214d..15c3630e92 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 ..................
 ----------------------------------------------------------------------
 Ran 18 tests
 
 OK
-{"execute": "job-finalize", "arguments": {"id": "commit0"}}
-{"return": {}}
-{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e28d75e018..5a501f0529 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -35,6 +35,13 @@ from collections import OrderedDict
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
+# Use this logger for logging messages directly from the iotests module
+logger = logging.getLogger(__name__)
+logger.addHandler(logging.NullHandler())
+
+# Use this logger for messages that ought to be used for diff output.
+test_logger = logging.getLogger('.'.join((__name__, 'iotest')))
+test_logger.addHandler(logging.NullHandler())
 
 # This will not work if arguments contain spaces but is necessary if we
 # want to support the override options that ./check supports.
@@ -343,10 +350,10 @@ def log(msg, filters=[], indent=None):
         separators = (', ', ': ') if indent is None else (',', ': ')
         # Don't sort if it's already sorted
         do_sort = not isinstance(msg, OrderedDict)
-        print(json.dumps(msg, sort_keys=do_sort,
-                         indent=indent, separators=separators))
+        test_logger.info(json.dumps(msg, sort_keys=do_sort,
+                                    indent=indent, separators=separators))
     else:
-        print(msg)
+        test_logger.info(msg)
 
 class Timeout:
     def __init__(self, seconds, errmsg = "Timeout"):
@@ -559,7 +566,7 @@ class VM(qtest.QEMUQtestMachine):
 
     # Returns None on success, and an error string on failure
     def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-                pre_finalize=None, cancel=False, use_log=True, wait=60.0):
+                pre_finalize=None, cancel=False, wait=60.0):
         """
         run_job moves a job from creation through to dismissal.
 
@@ -572,7 +579,6 @@ class VM(qtest.QEMUQtestMachine):
                              invoked prior to issuing job-finalize, if any.
         :param cancel: Bool. When true, cancels the job after the pre_finalize
                        callback.
-        :param use_log: Bool. When false, does not log QMP messages.
         :param wait: Float. Timeout value specifying how long to wait for any
                      event, in seconds. Defaults to 60.0.
         """
@@ -590,8 +596,7 @@ class VM(qtest.QEMUQtestMachine):
         while True:
             ev = filter_qmp_event(self.events_wait(events))
             if ev['event'] != 'JOB_STATUS_CHANGE':
-                if use_log:
-                    log(ev)
+                log(ev)
                 continue
             status = ev['data']['status']
             if status == 'aborting':
@@ -599,24 +604,16 @@ class VM(qtest.QEMUQtestMachine):
                 for j in result['return']:
                     if j['id'] == job:
                         error = j['error']
-                        if use_log:
-                            log('Job failed: %s' % (j['error']))
+                        log('Job failed: %s' % (j['error']))
             elif status == 'pending' and not auto_finalize:
                 if pre_finalize:
                     pre_finalize()
-                if cancel and use_log:
+                if cancel:
                     self.qmp_log('job-cancel', id=job)
-                elif cancel:
-                    self.qmp('job-cancel', id=job)
-                elif use_log:
+                else:
                     self.qmp_log('job-finalize', id=job)
-                else:
-                    self.qmp('job-finalize', id=job)
             elif status == 'concluded' and not auto_dismiss:
-                if use_log:
-                    self.qmp_log('job-dismiss', id=job)
-                else:
-                    self.qmp('job-dismiss', id=job)
+                self.qmp_log('job-dismiss', id=job)
             elif status == 'null':
                 return error
 
@@ -809,7 +806,7 @@ def notrun(reason):
     seq = os.path.basename(sys.argv[0])
 
     open('%s/%s.notrun' % (output_dir, seq), 'w').write(reason + '\n')
-    print('%s not run: %s' % (seq, reason))
+    logger.warning("%s not run: %s", seq, reason)
     sys.exit(0)
 
 def case_notrun(reason):
@@ -954,6 +951,7 @@ def execute_setup_common(supported_fmts=[],
     if debug:
         sys.argv.remove('-d')
     logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
+    logger.debug("iotests debugging messages active")
 
     return debug
 
@@ -966,14 +964,25 @@ def execute_test(test_function=None, *args, **kwargs):
     else:
         test_function()
 
+def activate_logging():
+    """Activate iotests.log() output to stdout for script-style tests."""
+    handler = logging.StreamHandler(stream=sys.stdout)
+    formatter = logging.Formatter('%(message)s')
+    handler.setFormatter(formatter)
+    test_logger.addHandler(handler)
+    test_logger.setLevel(logging.INFO)
+    test_logger.propagate = False
+
 # This is called from script-style iotests without a single point of entry
 def script_initialize(*args, **kwargs):
     """Initialize script-style tests without running any tests."""
+    activate_logging()
     execute_setup_common(*args, **kwargs)
 
 # This is called from script-style iotests with a single point of entry
 def script_main(test_function, *args, **kwargs):
     """Run script-style tests outside of the unittest framework"""
+    activate_logging()
     execute_test(test_function, *args, **kwargs)
 
 # This is called from unittest style iotests
-- 
2.21.0

Re: [Qemu-devel] [PATCH v5 5/5] iotests: use python logging for iotests.log()

[Vladimir Sementsov-Ogievskiy]

18.09.2019 2:45, John Snow wrote:
> We can turn logging on/off globally instead of per-function.
> 
> Remove use_log from run_job, and use python logging to turn on
> diffable output when we run through a script entry point.
> 
> iotest 245 changes output order due to buffering reasons.

Interesting, how can that be? pre-patch logging goes to stdout of test-case
and after-patch logging goes to stdout of test-case.. What's the difference
from tests/qemu-iotest/check point of view?

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/030        |  4 +--
>   tests/qemu-iotests/245        |  1 +
>   tests/qemu-iotests/245.out    | 24 ++++++++---------
>   tests/qemu-iotests/iotests.py | 49 +++++++++++++++++++++--------------
>   4 files changed, 44 insertions(+), 34 deletions(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index f3766f2a81..01aa96ed16 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
>           result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>           self.assert_qmp(result, 'return', {})
>   
> -        self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
> -        self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
> +        self.vm.run_job(job='drive0', auto_dismiss=True)
> +        self.vm.run_job(job='node4', auto_dismiss=True)
>           self.assert_no_active_block_jobs()
>   
>       # Test a block-stream and a block-commit job in parallel
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index 41218d5f1d..eba2157cff 100644
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -1000,5 +1000,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>           self.reopen(opts, {'backing': 'hd2'})
>   
>   if __name__ == '__main__':
> +    iotests.activate_logging()
>       iotests.main(supported_fmts=["qcow2"],
>                    supported_protocols=["file"])
> diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
> index a19de5214d..15c3630e92 100644
> --- a/tests/qemu-iotests/245.out
> +++ b/tests/qemu-iotests/245.out
> @@ -1,17 +1,17 @@
> +{"execute": "job-finalize", "arguments": {"id": "commit0"}}
> +{"return": {}}
> +{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"execute": "job-finalize", "arguments": {"id": "stream0"}}
> +{"return": {}}
> +{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"execute": "job-finalize", "arguments": {"id": "stream0"}}
> +{"return": {}}
> +{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>   ..................
>   ----------------------------------------------------------------------
>   Ran 18 tests
>   
>   OK
> -{"execute": "job-finalize", "arguments": {"id": "commit0"}}
> -{"return": {}}
> -{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> -{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> -{"execute": "job-finalize", "arguments": {"id": "stream0"}}
> -{"return": {}}
> -{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> -{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> -{"execute": "job-finalize", "arguments": {"id": "stream0"}}
> -{"return": {}}
> -{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> -{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index e28d75e018..5a501f0529 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -35,6 +35,13 @@ from collections import OrderedDict
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>   from qemu import qtest
>   
> +# Use this logger for logging messages directly from the iotests module
> +logger = logging.getLogger(__name__)
> +logger.addHandler(logging.NullHandler())
> +
> +# Use this logger for messages that ought to be used for diff output.
> +test_logger = logging.getLogger('.'.join((__name__, 'iotest')))
> +test_logger.addHandler(logging.NullHandler())
>   
>   # This will not work if arguments contain spaces but is necessary if we
>   # want to support the override options that ./check supports.
> @@ -343,10 +350,10 @@ def log(msg, filters=[], indent=None):
>           separators = (', ', ': ') if indent is None else (',', ': ')
>           # Don't sort if it's already sorted
>           do_sort = not isinstance(msg, OrderedDict)
> -        print(json.dumps(msg, sort_keys=do_sort,
> -                         indent=indent, separators=separators))
> +        test_logger.info(json.dumps(msg, sort_keys=do_sort,
> +                                    indent=indent, separators=separators))
>       else:
> -        print(msg)
> +        test_logger.info(msg)
>   
>   class Timeout:
>       def __init__(self, seconds, errmsg = "Timeout"):
> @@ -559,7 +566,7 @@ class VM(qtest.QEMUQtestMachine):
>   
>       # Returns None on success, and an error string on failure
>       def run_job(self, job, auto_finalize=True, auto_dismiss=False,
> -                pre_finalize=None, cancel=False, use_log=True, wait=60.0):
> +                pre_finalize=None, cancel=False, wait=60.0):
>           """
>           run_job moves a job from creation through to dismissal.
>   
> @@ -572,7 +579,6 @@ class VM(qtest.QEMUQtestMachine):
>                                invoked prior to issuing job-finalize, if any.
>           :param cancel: Bool. When true, cancels the job after the pre_finalize
>                          callback.
> -        :param use_log: Bool. When false, does not log QMP messages.
>           :param wait: Float. Timeout value specifying how long to wait for any
>                        event, in seconds. Defaults to 60.0.
>           """
> @@ -590,8 +596,7 @@ class VM(qtest.QEMUQtestMachine):
>           while True:
>               ev = filter_qmp_event(self.events_wait(events))
>               if ev['event'] != 'JOB_STATUS_CHANGE':
> -                if use_log:
> -                    log(ev)
> +                log(ev)
>                   continue
>               status = ev['data']['status']
>               if status == 'aborting':
> @@ -599,24 +604,16 @@ class VM(qtest.QEMUQtestMachine):
>                   for j in result['return']:
>                       if j['id'] == job:
>                           error = j['error']
> -                        if use_log:
> -                            log('Job failed: %s' % (j['error']))
> +                        log('Job failed: %s' % (j['error']))
>               elif status == 'pending' and not auto_finalize:
>                   if pre_finalize:
>                       pre_finalize()
> -                if cancel and use_log:
> +                if cancel:
>                       self.qmp_log('job-cancel', id=job)
> -                elif cancel:
> -                    self.qmp('job-cancel', id=job)
> -                elif use_log:
> +                else:
>                       self.qmp_log('job-finalize', id=job)
> -                else:
> -                    self.qmp('job-finalize', id=job)
>               elif status == 'concluded' and not auto_dismiss:
> -                if use_log:
> -                    self.qmp_log('job-dismiss', id=job)
> -                else:
> -                    self.qmp('job-dismiss', id=job)
> +                self.qmp_log('job-dismiss', id=job)
>               elif status == 'null':
>                   return error
>   
> @@ -809,7 +806,7 @@ def notrun(reason):
>       seq = os.path.basename(sys.argv[0])
>   
>       open('%s/%s.notrun' % (output_dir, seq), 'w').write(reason + '\n')
> -    print('%s not run: %s' % (seq, reason))
> +    logger.warning("%s not run: %s", seq, reason)
>       sys.exit(0)
>   
>   def case_notrun(reason):
> @@ -954,6 +951,7 @@ def execute_setup_common(supported_fmts=[],
>       if debug:
>           sys.argv.remove('-d')
>       logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
> +    logger.debug("iotests debugging messages active")
>   
>       return debug
>   
> @@ -966,14 +964,25 @@ def execute_test(test_function=None, *args, **kwargs):
>       else:
>           test_function()
>   
> +def activate_logging():
> +    """Activate iotests.log() output to stdout for script-style tests."""
> +    handler = logging.StreamHandler(stream=sys.stdout)
> +    formatter = logging.Formatter('%(message)s')
> +    handler.setFormatter(formatter)

Hmm, it seems this formatter is default behavior, and it's not necessary to
create and set it..

> +    test_logger.addHandler(handler)

possibly, we want to remove old handler (null), as it's not needed anymore.

> +    test_logger.setLevel(logging.INFO)

Should it be DEBUG if -d given?

> +    test_logger.propagate = False
> +
>   # This is called from script-style iotests without a single point of entry
>   def script_initialize(*args, **kwargs):
>       """Initialize script-style tests without running any tests."""
> +    activate_logging()
>       execute_setup_common(*args, **kwargs)
>   
>   # This is called from script-style iotests with a single point of entry
>   def script_main(test_function, *args, **kwargs):
>       """Run script-style tests outside of the unittest framework"""
> +    activate_logging()
>       execute_test(test_function, *args, **kwargs)
>   
>   # This is called from unittest style iotests
> 

anyway, it seems OK for me as is:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 5/5] iotests: use python logging for iotests.log()

[John Snow]

On 9/18/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> 18.09.2019 2:45, John Snow wrote:
>> We can turn logging on/off globally instead of per-function.
>>
>> Remove use_log from run_job, and use python logging to turn on
>> diffable output when we run through a script entry point.
>>
>> iotest 245 changes output order due to buffering reasons.
> 
> Interesting, how can that be? pre-patch logging goes to stdout of test-case
> and after-patch logging goes to stdout of test-case.. What's the difference
> from tests/qemu-iotest/check point of view?
> 
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/030        |  4 +--
>>   tests/qemu-iotests/245        |  1 +
>>   tests/qemu-iotests/245.out    | 24 ++++++++---------
>>   tests/qemu-iotests/iotests.py | 49 +++++++++++++++++++++--------------
>>   4 files changed, 44 insertions(+), 34 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>> index f3766f2a81..01aa96ed16 100755
>> --- a/tests/qemu-iotests/030
>> +++ b/tests/qemu-iotests/030
>> @@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
>>           result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>           self.assert_qmp(result, 'return', {})
>>   
>> -        self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
>> -        self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
>> +        self.vm.run_job(job='drive0', auto_dismiss=True)
>> +        self.vm.run_job(job='node4', auto_dismiss=True)
>>           self.assert_no_active_block_jobs()
>>   
>>       # Test a block-stream and a block-commit job in parallel
>> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
>> index 41218d5f1d..eba2157cff 100644
>> --- a/tests/qemu-iotests/245
>> +++ b/tests/qemu-iotests/245
>> @@ -1000,5 +1000,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>>           self.reopen(opts, {'backing': 'hd2'})
>>   
>>   if __name__ == '__main__':
>> +    iotests.activate_logging()
>>       iotests.main(supported_fmts=["qcow2"],
>>                    supported_protocols=["file"])
>> diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
>> index a19de5214d..15c3630e92 100644
>> --- a/tests/qemu-iotests/245.out
>> +++ b/tests/qemu-iotests/245.out
>> @@ -1,17 +1,17 @@
>> +{"execute": "job-finalize", "arguments": {"id": "commit0"}}
>> +{"return": {}}
>> +{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"execute": "job-finalize", "arguments": {"id": "stream0"}}
>> +{"return": {}}
>> +{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"execute": "job-finalize", "arguments": {"id": "stream0"}}
>> +{"return": {}}
>> +{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>>   ..................
>>   ----------------------------------------------------------------------
>>   Ran 18 tests
>>   
>>   OK
>> -{"execute": "job-finalize", "arguments": {"id": "commit0"}}
>> -{"return": {}}
>> -{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> -{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> -{"execute": "job-finalize", "arguments": {"id": "stream0"}}
>> -{"return": {}}
>> -{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> -{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> -{"execute": "job-finalize", "arguments": {"id": "stream0"}}
>> -{"return": {}}
>> -{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> -{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index e28d75e018..5a501f0529 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -35,6 +35,13 @@ from collections import OrderedDict
>>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>>   from qemu import qtest
>>   
>> +# Use this logger for logging messages directly from the iotests module
>> +logger = logging.getLogger(__name__)
>> +logger.addHandler(logging.NullHandler())
>> +
>> +# Use this logger for messages that ought to be used for diff output.
>> +test_logger = logging.getLogger('.'.join((__name__, 'iotest')))
>> +test_logger.addHandler(logging.NullHandler())
>>   
>>   # This will not work if arguments contain spaces but is necessary if we
>>   # want to support the override options that ./check supports.
>> @@ -343,10 +350,10 @@ def log(msg, filters=[], indent=None):
>>           separators = (', ', ': ') if indent is None else (',', ': ')
>>           # Don't sort if it's already sorted
>>           do_sort = not isinstance(msg, OrderedDict)
>> -        print(json.dumps(msg, sort_keys=do_sort,
>> -                         indent=indent, separators=separators))
>> +        test_logger.info(json.dumps(msg, sort_keys=do_sort,
>> +                                    indent=indent, separators=separators))
>>       else:
>> -        print(msg)
>> +        test_logger.info(msg)
>>   
>>   class Timeout:
>>       def __init__(self, seconds, errmsg = "Timeout"):
>> @@ -559,7 +566,7 @@ class VM(qtest.QEMUQtestMachine):
>>   
>>       # Returns None on success, and an error string on failure
>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False,
>> -                pre_finalize=None, cancel=False, use_log=True, wait=60.0):
>> +                pre_finalize=None, cancel=False, wait=60.0):
>>           """
>>           run_job moves a job from creation through to dismissal.
>>   
>> @@ -572,7 +579,6 @@ class VM(qtest.QEMUQtestMachine):
>>                                invoked prior to issuing job-finalize, if any.
>>           :param cancel: Bool. When true, cancels the job after the pre_finalize
>>                          callback.
>> -        :param use_log: Bool. When false, does not log QMP messages.
>>           :param wait: Float. Timeout value specifying how long to wait for any
>>                        event, in seconds. Defaults to 60.0.
>>           """
>> @@ -590,8 +596,7 @@ class VM(qtest.QEMUQtestMachine):
>>           while True:
>>               ev = filter_qmp_event(self.events_wait(events))
>>               if ev['event'] != 'JOB_STATUS_CHANGE':
>> -                if use_log:
>> -                    log(ev)
>> +                log(ev)
>>                   continue
>>               status = ev['data']['status']
>>               if status == 'aborting':
>> @@ -599,24 +604,16 @@ class VM(qtest.QEMUQtestMachine):
>>                   for j in result['return']:
>>                       if j['id'] == job:
>>                           error = j['error']
>> -                        if use_log:
>> -                            log('Job failed: %s' % (j['error']))
>> +                        log('Job failed: %s' % (j['error']))
>>               elif status == 'pending' and not auto_finalize:
>>                   if pre_finalize:
>>                       pre_finalize()
>> -                if cancel and use_log:
>> +                if cancel:
>>                       self.qmp_log('job-cancel', id=job)
>> -                elif cancel:
>> -                    self.qmp('job-cancel', id=job)
>> -                elif use_log:
>> +                else:
>>                       self.qmp_log('job-finalize', id=job)
>> -                else:
>> -                    self.qmp('job-finalize', id=job)
>>               elif status == 'concluded' and not auto_dismiss:
>> -                if use_log:
>> -                    self.qmp_log('job-dismiss', id=job)
>> -                else:
>> -                    self.qmp('job-dismiss', id=job)
>> +                self.qmp_log('job-dismiss', id=job)
>>               elif status == 'null':
>>                   return error
>>   
>> @@ -809,7 +806,7 @@ def notrun(reason):
>>       seq = os.path.basename(sys.argv[0])
>>   
>>       open('%s/%s.notrun' % (output_dir, seq), 'w').write(reason + '\n')
>> -    print('%s not run: %s' % (seq, reason))
>> +    logger.warning("%s not run: %s", seq, reason)
>>       sys.exit(0)
>>   
>>   def case_notrun(reason):
>> @@ -954,6 +951,7 @@ def execute_setup_common(supported_fmts=[],
>>       if debug:
>>           sys.argv.remove('-d')
>>       logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>> +    logger.debug("iotests debugging messages active")
>>   
>>       return debug
>>   
>> @@ -966,14 +964,25 @@ def execute_test(test_function=None, *args, **kwargs):
>>       else:
>>           test_function()
>>   
>> +def activate_logging():
>> +    """Activate iotests.log() output to stdout for script-style tests."""
>> +    handler = logging.StreamHandler(stream=sys.stdout)
>> +    formatter = logging.Formatter('%(message)s')
>> +    handler.setFormatter(formatter)
> 
> Hmm, it seems this formatter is default behavior, and it's not necessary to
> create and set it..
> 

You might be right; I think it's OK to set it explicitly.

>> +    test_logger.addHandler(handler)
> 
> possibly, we want to remove old handler (null), as it's not needed anymore.
> 

Right, it's there as an "event sink" in the case that no caller sets up
a root logger. I'll look into this if/when I pursue an idempotent
enable/disable toggle.

At this point, if there's no major issues, I'd rather get the fiddly
bits checked in before continuing on this cleanup; I can always start
sending some "part 2" patches along this line.

>> +    test_logger.setLevel(logging.INFO)
> 
> Should it be DEBUG if -d given?
> 

There's no reason to yet, because there's no use of the test_logger that
uses debug-level statements. That is, regardless of the "debug level",
we're going to log the same exact things to the diff output.

We COULD complicate this in the future if we wanted to. That is, we can
utilize debug-level loggers and create nnn.out.debug; but there's no
reason to do that yet.

>> +    test_logger.propagate = False
>> +
>>   # This is called from script-style iotests without a single point of entry
>>   def script_initialize(*args, **kwargs):
>>       """Initialize script-style tests without running any tests."""
>> +    activate_logging()
>>       execute_setup_common(*args, **kwargs)
>>   
>>   # This is called from script-style iotests with a single point of entry
>>   def script_main(test_function, *args, **kwargs):
>>       """Run script-style tests outside of the unittest framework"""
>> +    activate_logging()
>>       execute_test(test_function, *args, **kwargs)
>>   
>>   # This is called from unittest style iotests
>>
> 
> anyway, it seems OK for me as is:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 

Oh, and I forgot to clean up the comment strings, sorry about that...
I'll send a fixup patch.

--js

Re: [PATCH v5 5/5] iotests: use python logging for iotests.log()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 674 bytes --]

On 18.09.19 01:45, John Snow wrote:
> We can turn logging on/off globally instead of per-function.
> 
> Remove use_log from run_job, and use python logging to turn on
> diffable output when we run through a script entry point.
> 
> iotest 245 changes output order due to buffering reasons.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/030        |  4 +--
>  tests/qemu-iotests/245        |  1 +
>  tests/qemu-iotests/245.out    | 24 ++++++++---------
>  tests/qemu-iotests/iotests.py | 49 +++++++++++++++++++++--------------
>  4 files changed, 44 insertions(+), 34 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [PATCH v5 0/5] iotests: use python logging

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]

On 18.09.19 01:45, John Snow wrote:
> This series uses python logging to enable output conditionally on
> iotests.log(). We unify an initialization call (which also enables
> debugging output for those tests with -d) and then make the switch
> inside of iotests.
> 
> It will help alleviate the need to create logged/unlogged versions
> of all the various helpers we have made.
> 
> V5:
>  - Rebased again
>  - Allow Python tests to run on any platform
> 
> V4:
>  - Rebased on top of kwolf/block at the behest of mreitz
> 
> V3:
>  - Rebased for 4.1+; now based on main branch.
> 
> V2:
>  - Added all of the other python tests I missed to use script_initialize
>  - Refactored the common setup as per Ehabkost's suggestion
>  - Added protocol arguments to common initialization,
>    but this isn't strictly required.

I’m OK to take the series as-is (it doesn’t affect any auto tests, so we
can decide what to do about non-Linux platforms in make check at a later
point), but there seems to be something you wanted to fix up in patch 5.

(And there’s also Kevin’s pending pull request that changes a bit of
iotests.py.)

Max


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

Re: [PATCH v5 0/5] iotests: use python logging

[John Snow]

On 10/4/19 11:39 AM, Max Reitz wrote:
> On 18.09.19 01:45, John Snow wrote:
>> This series uses python logging to enable output conditionally on
>> iotests.log(). We unify an initialization call (which also enables
>> debugging output for those tests with -d) and then make the switch
>> inside of iotests.
>>
>> It will help alleviate the need to create logged/unlogged versions
>> of all the various helpers we have made.
>>
>> V5:
>>  - Rebased again
>>  - Allow Python tests to run on any platform
>>
>> V4:
>>  - Rebased on top of kwolf/block at the behest of mreitz
>>
>> V3:
>>  - Rebased for 4.1+; now based on main branch.
>>
>> V2:
>>  - Added all of the other python tests I missed to use script_initialize
>>  - Refactored the common setup as per Ehabkost's suggestion
>>  - Added protocol arguments to common initialization,
>>    but this isn't strictly required.
> 
> I’m OK to take the series as-is (it doesn’t affect any auto tests, so we
> can decide what to do about non-Linux platforms in make check at a later
> point), but there seems to be something you wanted to fix up in patch 5.
> 
> (And there’s also Kevin’s pending pull request that changes a bit of
> iotests.py.)
> 
> Max
> 

Just caught up with the discussion.

It looks like Thomas took my 1/5; so I'll respin on top of his "[PATCH
0/5] Enable more iotests during "make check-block" series to catch those
improvements as they stand.

--js

Re: [PATCH v5 0/5] iotests: use python logging

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 297 bytes --]

On 12.10.19 01:39, John Snow wrote:
> Just caught up with the discussion.
> 
> It looks like Thomas took my 1/5; so I'll respin on top of his "[PATCH
> 0/5] Enable more iotests during "make check-block" series to catch those
> improvements as they stand.

Any updates on this? :)

Max


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

Re: [PATCH v5 0/5] iotests: use python logging

[John Snow]

On 2/24/20 6:15 AM, Max Reitz wrote:
> On 12.10.19 01:39, John Snow wrote:
>> Just caught up with the discussion.
>>
>> It looks like Thomas took my 1/5; so I'll respin on top of his "[PATCH
>> 0/5] Enable more iotests during "make check-block" series to catch those
>> improvements as they stand.
> 
> Any updates on this? :)
> 
> Max
> 

Nope.

Well, except that I was working on job_run today and remembered that I
needed to do this. I was waiting for that discussion to die down, and
then forgetting took over.

Will attempt to resuscitate.