Re: [PATCH v2 10/10] iotests/030: Unthrottle parallel jobs in reverse

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

15.11.2021 16:56, Hanna Reitz wrote:
> On 12.11.21 17:25, Vladimir Sementsov-Ogievskiy wrote:
>> 11.11.2021 15:08, Hanna Reitz wrote:
>>> See the comment for why this is necessary.
>>>
>>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>>> ---
>>>   tests/qemu-iotests/030 | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>>> index 5fb65b4bef..567bf1da67 100755
>>> --- a/tests/qemu-iotests/030
>>> +++ b/tests/qemu-iotests/030
>>> @@ -251,7 +251,16 @@ class TestParallelOps(iotests.QMPTestCase):
>>>                                    speed=1024)
>>>               self.assert_qmp(result, 'return', {})
>>>   -        for job in pending_jobs:
>>> +        # Do this in reverse: After unthrottling them, some jobs may finish
>>> +        # before we have unthrottled all of them.  This will drain their
>>> +        # subgraph, and this will make jobs above them advance (despite those
>>> +        # jobs on top being throttled).  In the worst case, all jobs below the
>>> +        # top one are finished before we can unthrottle it, and this makes it
>>> +        # advance so far that it completes before we can unthrottle it - which
>>> +        # results in an error.
>>> +        # Starting from the top (i.e. in reverse) does not have this problem:
>>> +        # When a job finishes, the ones below it are not advanced.
>>
>> Hmm, interesting why only jobs above the finished job may advance in the situation..
>>
>> Looks like something may change and this workaround will stop working.
>>
>> Isn't it better just handle the error, and don't care if job was just finished?
>>
>> Something like
>>
>> if result['return'] != {}:
>>    # Job was finished during drain caused by finish of already unthrottled job
>>    self.assert_qmp(result, 'error/class', 'DeviceNotActive')
> 
> Well.  My explanation (excuse) is that I felt like this was the hack-ish solution that I could have gone for from the start without understanding what the issue is (and in fact it was the solution I used while debugging the other problems).  I went with `reversed()`, because that really addresses the problem.
> 
> You’re right in that it only addresses the problem for now and there’s a chance it might reappear.  If we want to go with ignoring DeviceNotActive errors, then I think we should at least query all block jobs before the unthrottle loop and see that at least at one point they were all running simultaneously.
> 
> I don’t really have a strong opinion.  We can exchange this patch now (though I’d rather not hold up the rest of the series for it), or have a patch on top later, or, well, just keep it for now.  I think the least stressful option would be to just fix it up later.
> 

OK, I agree


-- 
Best regards,
Vladimir