| Message ID | 20190130235251.17911-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Filter second BLOCK_JOB_ERROR from 229 | expand |
On 1/30/19 6:52 PM, Max Reitz wrote: > Without this filter, this test sometimes fails. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > I intended to send this as part of my iotest fixes series, but it ended > up on the wrong branch... Doesn't really matter, though, as there is no > functional dependency. > --- > tests/qemu-iotests/229 | 6 +++++- > tests/qemu-iotests/229.out | 1 - > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/229 b/tests/qemu-iotests/229 > index 893d098ad2..b0d4885fa6 100755 > --- a/tests/qemu-iotests/229 > +++ b/tests/qemu-iotests/229 > @@ -81,11 +81,15 @@ echo > echo '=== Force cancel job paused in error state ===' > echo > > +# Filter out BLOCK_JOB_ERROR events because they may or may not occur. > +# Cancelling the job means resuming it for a bit before it is actually > +# aborted, and in that time it may or may not re-encounter the error. Oh, because the job is "paused" and cancelling it involves job_enter, which we then allow the job to gracefully fail through it's own pathways -- but depending on where it failed originally, it may-or-may-not wind up trying something else that fails before it finds the "exit gracefully" signal, is that right? I guess there's no real way to adjust that behavior. > success_or_failure="y" _send_qemu_cmd $QEMU_HANDLE \ > "{'execute': 'block-job-cancel', > 'arguments': { 'device': 'testdisk', > 'force': true}}" \ > - "BLOCK_JOB_CANCELLED" "Assertion" > + "BLOCK_JOB_CANCELLED" "Assertion" \ > + | grep -v '"BLOCK_JOB_ERROR"' > > # success, all done > echo "*** done" > diff --git a/tests/qemu-iotests/229.out b/tests/qemu-iotests/229.out > index 4c4112805f..a3eb33788a 100644 > --- a/tests/qemu-iotests/229.out > +++ b/tests/qemu-iotests/229.out > @@ -17,7 +17,6 @@ wrote 2097152/2097152 bytes at offset 0 > > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "testdisk"}} > {"return": {}} > -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "testdisk", "operation": "write", "action": "stop"}} > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "testdisk"}} > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "testdisk", "len": 2097152, "offset": 1048576, "speed": 0, "type": "mirror"}} > *** done > I think this is fine, if we cannot help to make this any more deterministic, so I'm fine with: Reviewed-by: John Snow <jsnow@redhat.com> but I am curious to know if this poses any theoretical problems for libvirt having to deal with possibly an extra hiccup before the cancel registers.
Am 31.01.2019 um 00:52 hat Max Reitz geschrieben: > Without this filter, this test sometimes fails. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Thanks, applied to the block branch. Kevin
On 31.01.19 03:21, John Snow wrote: > > > On 1/30/19 6:52 PM, Max Reitz wrote: >> Without this filter, this test sometimes fails. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> I intended to send this as part of my iotest fixes series, but it ended >> up on the wrong branch... Doesn't really matter, though, as there is no >> functional dependency. >> --- >> tests/qemu-iotests/229 | 6 +++++- >> tests/qemu-iotests/229.out | 1 - >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/tests/qemu-iotests/229 b/tests/qemu-iotests/229 >> index 893d098ad2..b0d4885fa6 100755 >> --- a/tests/qemu-iotests/229 >> +++ b/tests/qemu-iotests/229 >> @@ -81,11 +81,15 @@ echo >> echo '=== Force cancel job paused in error state ===' >> echo >> >> +# Filter out BLOCK_JOB_ERROR events because they may or may not occur. >> +# Cancelling the job means resuming it for a bit before it is actually >> +# aborted, and in that time it may or may not re-encounter the error. > > Oh, because the job is "paused" and cancelling it involves job_enter, > which we then allow the job to gracefully fail through it's own pathways > -- but depending on where it failed originally, it may-or-may-not wind > up trying something else that fails before it finds the "exit > gracefully" signal, is that right? That's at least how I explained it to me, yes. > I guess there's no real way to adjust that behavior. > >> success_or_failure="y" _send_qemu_cmd $QEMU_HANDLE \ >> "{'execute': 'block-job-cancel', >> 'arguments': { 'device': 'testdisk', >> 'force': true}}" \ >> - "BLOCK_JOB_CANCELLED" "Assertion" >> + "BLOCK_JOB_CANCELLED" "Assertion" \ >> + | grep -v '"BLOCK_JOB_ERROR"' >> >> # success, all done >> echo "*** done" >> diff --git a/tests/qemu-iotests/229.out b/tests/qemu-iotests/229.out >> index 4c4112805f..a3eb33788a 100644 >> --- a/tests/qemu-iotests/229.out >> +++ b/tests/qemu-iotests/229.out >> @@ -17,7 +17,6 @@ wrote 2097152/2097152 bytes at offset 0 >> >> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "testdisk"}} >> {"return": {}} >> -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "testdisk", "operation": "write", "action": "stop"}} >> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "testdisk"}} >> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "testdisk", "len": 2097152, "offset": 1048576, "speed": 0, "type": "mirror"}} >> *** done >> > > I think this is fine, if we cannot help to make this any more > deterministic, so I'm fine with: > > Reviewed-by: John Snow <jsnow@redhat.com> > > but I am curious to know if this poses any theoretical problems for > libvirt having to deal with possibly an extra hiccup before the cancel > registers. Hm... I would say the bigger issue is the error event appearing than that not being the case. Because it appears most of the time, I think libvirt can deal with it. Max
diff --git a/tests/qemu-iotests/229 b/tests/qemu-iotests/229 index 893d098ad2..b0d4885fa6 100755 --- a/tests/qemu-iotests/229 +++ b/tests/qemu-iotests/229 @@ -81,11 +81,15 @@ echo echo '=== Force cancel job paused in error state ===' echo +# Filter out BLOCK_JOB_ERROR events because they may or may not occur. +# Cancelling the job means resuming it for a bit before it is actually +# aborted, and in that time it may or may not re-encounter the error. success_or_failure="y" _send_qemu_cmd $QEMU_HANDLE \ "{'execute': 'block-job-cancel', 'arguments': { 'device': 'testdisk', 'force': true}}" \ - "BLOCK_JOB_CANCELLED" "Assertion" + "BLOCK_JOB_CANCELLED" "Assertion" \ + | grep -v '"BLOCK_JOB_ERROR"' # success, all done echo "*** done" diff --git a/tests/qemu-iotests/229.out b/tests/qemu-iotests/229.out index 4c4112805f..a3eb33788a 100644 --- a/tests/qemu-iotests/229.out +++ b/tests/qemu-iotests/229.out @@ -17,7 +17,6 @@ wrote 2097152/2097152 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "testdisk"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "testdisk", "operation": "write", "action": "stop"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "testdisk"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "testdisk", "len": 2097152, "offset": 1048576, "speed": 0, "type": "mirror"}} *** done
Without this filter, this test sometimes fails. Signed-off-by: Max Reitz <mreitz@redhat.com> --- I intended to send this as part of my iotest fixes series, but it ended up on the wrong branch... Doesn't really matter, though, as there is no functional dependency. --- tests/qemu-iotests/229 | 6 +++++- tests/qemu-iotests/229.out | 1 - 2 files changed, 5 insertions(+), 2 deletions(-)