Re: [PATCH] Revert "tests/migration: Reduce autoconverge initial bandwidth"

[Philippe Mathieu-Daudé]

On 6/24/20 7:04 AM, Thomas Huth wrote:
> On 23/06/2020 19.35, Philippe Mathieu-Daudé wrote:
>> On 6/23/20 7:07 PM, Thomas Huth wrote:
>>> On 23/06/2020 17.39, Philippe Mathieu-Daudé wrote:
>>>> On 6/23/20 4:56 PM, Michael S. Tsirkin wrote:
>>>>> This reverts commit 6d1da867e65f ("tests/migration: Reduce autoconverge 
>>>>> initial bandwidth")
>>>>> since that change makes unit tests much slower for all developers, while 
>>>>> it's not
>>>>> a robust way to fix migration tests. Migration tests need to find
>>>>> a more robust way to discover a reasonable bandwidth without slowing
>>>>> things down for everyone.
>>>>
>>>> Please also mention we can do this since 1de8e4c4dcf which allow
>>>> marked the s390x job as "unstable" and allow it to fail.
>>>>
>>>> But if nobody is going to look at it, instead lets disable
>>>> it until someone figure out the issue:
>>>>
>>>> -- >8 --
>>>> diff --git a/.travis.yml b/.travis.yml
>>>> index 74158f741b..364e67b14b 100644
>>>> --- a/.travis.yml
>>>> +++ b/.travis.yml
>>>> @@ -507,6 +507,7 @@ jobs:
>>>>
>>>>      - name: "[s390x] Clang (disable-tcg)"
>>>>        arch: s390x
>>>> +      if: false # Temporarily disabled due to issue testing migration
>>>> (see commit 6d1da867e65).
>>>>        dist: bionic
>>>>        compiler: clang
>>>>        addons:
>>>
>>> Sorry, but that looks wrong. First, the disable-tcg test does not run
>>> the qtests at all. So this is certainly the wrong location here.
>>
>> Indeed, this is the previous job:
>>
>> -- >8 --
>> diff --git a/.travis.yml b/.travis.yml
>> index 74158f741b..b399e20078 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -464,6 +464,7 @@ jobs:
>>          - CONFIG="--disable-containers
>> --target-list=ppc64-softmmu,ppc64le-linux-user"
>>
>>      - name: "[s390x] GCC check-tcg"
>> +      if: false # Temporarily disabled due to issue testing migration
>> (see commit 6d1da867e65).
>>        arch: s390x
>>        dist: bionic
>>        addons:
>> ---
>>
>>> Second,
>>> if just one of the qtests is failing, please only disable that single
>>> failing qtest and not the whole test pipeline.
>>
>> Last time we talked about this Dave was against that option:
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg690085.html
>>
> 
> Was he? Citing his reply to the mail from your URL:
> 
>  "Before we take the hammer to it, could you try reducing it's initial
> bandwidth"
> 
> So all I can see is that he first wanted to try something different than
> disabling the test. And now,  instead of using a small hammer to disable
> just this test, you now even use a very *big* hammer to disable *all*
> tests. That's just a very bad idea. Please don't.

You are right. I was being concerned about having CI working because
the more red it stay, the less likely the community will worry about
it, and I didn't want we loose interest in testing (or discredit its
importance). I now understand without having CI gating, it is
pointless to try to keep it green (at the cost of having all local
testing running slower, it is worst if maintainers stop their local
testing).

WRT this test I have no idea what it is doing, furthermore why it
fails on s390x containers, so I sent a simple patch to fix the CI,
but failed to foreseen its negative effect on the rest of the
developers.

Thanks Michael for fixing my mess with your patch:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Regards,

Phil.