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

[Philippe Mathieu-Daudé]

Hi Thomas,

On 6/24/20 12:21 PM, Philippe Mathieu-Daudé wrote:
> 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 asked on IRC the CI failures history:

https://travis-ci.org/github/philmd/qemu/jobs/663607963
https://travis-ci.org/github/philmd/qemu/jobs/663622229
https://travis-ci.org/github/philmd/qemu/jobs/663642522

"No output has been received in the last 10m0s, this potentially
indicates a stalled build or something wrong with the build itself."

> 
> 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.
>