[Qemu-devel] [PATCH] iotests: Filter second BLOCK_JOB_ERROR from 229

[Qemu-devel] [PATCH] iotests: Filter second BLOCK_JOB_ERROR from 229

[Max Reitz]

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

Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: Filter second BLOCK_JOB_ERROR from 229

[John Snow]

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.

Re: [Qemu-devel] [PATCH] iotests: Filter second BLOCK_JOB_ERROR from 229

[Kevin Wolf]

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

Re: [Qemu-devel] [Qemu-block] [PATCH] iotests: Filter second BLOCK_JOB_ERROR from 229

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]