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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature