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 <hre...@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.
Hanna
