[Qemu-devel] [PATCH 0/5] iotests: Make some tests less flaky

[Qemu-devel] [PATCH 0/5] iotests: Make some tests less flaky

[Max Reitz]

There are a couple of tests that fail (on my machine) from time to
time (and by that I mean that recently I've rarely ever had a test run
with both 083 and 136 working on first try).
This series should fix most (at least the issues I am aware of).

Notes:
- 083 might have another issue, but if so it occurs extremely rarely and
  so I was unable to debug it.

- 129 is flaky, too, because it tries to use block jobs with BB
  throttling -- however, block jobs ignore that these days.  Making it
  use a throttle filter node will require quite a bit of work.  See
  http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00111.html
  for more.

- 194 sometimes hangs because the source VM fails to drain its drive.
  This is probably not an issue with the test, but actually an issue in
  qemu.  See
  http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00256.html
  for more.


"All tests have passed, let's ship it!"
-- Me, 2:36 am


(Editor's note: "all" means raw/file, qcow2/file, and raw/nbd.)


Max Reitz (5):
  iotests: Make 030 less flaky
  iotests: Add missing 'blkdebug::' in 040
  iotests: Make 055 less flaky
  iotests: Make 083 less flaky
  iotests: Make 136 less flaky

 tests/qemu-iotests/030 |  8 ++++++--
 tests/qemu-iotests/040 |  2 +-
 tests/qemu-iotests/055 | 25 ++++++++++++++++---------
 tests/qemu-iotests/083 |  3 ++-
 tests/qemu-iotests/136 | 14 +++++++++++++-
 5 files changed, 38 insertions(+), 14 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/5] iotests: Make 030 less flaky

[Max Reitz]

This patch fixes two race conditions in 030:

1. The first is in TestENSPC.test_enospc().  After resuming the job,
   querying it to confirm it is no longer paused may fail because in the
   meantime it might have completed already.  The same was fixed in
   TestEIO.test_ignore() already (in commit
   2c3b44da07d341557a8203cc509ea07fe3605e11).

2. The second is in TestSetSpeed.test_set_speed_invalid(): Here, a
   stream job is started on a drive without any break points, with a
   block-job-set-speed invoked subsequently.  However, without any break
   points, the job might have completed in the meantime (on tmpfs at
   least); or it might complete before cancel_and_wait() which expects
   the job to still exist.  This can be fixed like everywhere else by
   pausing the drive (installing break points) before starting the job
   and letting cancel_and_wait() resume it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/030 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 18838948fa..457984b8e9 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -666,6 +666,7 @@ class TestENOSPC(TestErrors):
                 if event['event'] == 'BLOCK_JOB_ERROR':
                     self.assert_qmp(event, 'data/device', 'drive0')
                     self.assert_qmp(event, 'data/operation', 'read')
+                    error = True
 
                     result = self.vm.qmp('query-block-jobs')
                     self.assert_qmp(result, 'return[0]/paused', True)
@@ -676,9 +677,11 @@ class TestENOSPC(TestErrors):
                     self.assert_qmp(result, 'return', {})
 
                     result = self.vm.qmp('query-block-jobs')
+                    if result == {'return': []}:
+                        # Race; likely already finished. Check.
+                        continue
                     self.assert_qmp(result, 'return[0]/paused', False)
                     self.assert_qmp(result, 'return[0]/io-status', 'ok')
-                    error = True
                 elif event['event'] == 'BLOCK_JOB_COMPLETED':
                     self.assertTrue(error, 'job completed unexpectedly')
                     self.assert_qmp(event, 'data/type', 'stream')
@@ -792,13 +795,14 @@ class TestSetSpeed(iotests.QMPTestCase):
 
         self.assert_no_active_block_jobs()
 
+        self.vm.pause_drive('drive0')
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        self.cancel_and_wait()
+        self.cancel_and_wait(resume=True)
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
-- 
2.13.6

[Qemu-devel] [PATCH 2/5] iotests: Add missing 'blkdebug::' in 040

[Max Reitz]

040 tries to invoke pause_drive() on a drive that does not use blkdebug.
Good idea, but let's use blkdebug to make it actually work.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/040 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index c284d08796..90b5b4f2ad 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -289,7 +289,7 @@ class TestSetSpeed(ImageCommitTestCase):
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
         qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 0 512', test_img)
         qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', mid_img)
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
         self.vm.launch()
 
     def tearDown(self):
-- 
2.13.6

[Qemu-devel] [PATCH 3/5] iotests: Make 055 less flaky

[Max Reitz]

First of all, test 055 does a valiant job of invoking pause_drive()
sometimes, but that is worth nothing without blkdebug.  So the first
thing to do is to sprinkle a couple of "blkdebug::" in there -- with the
exception of the transaction tests, because the blkdebug break points
make the transaction QMP command hang (which is bad).  In that case, we
can get away with throttling the block job that it effectively is
paused.

Then, 055 usually does not pause the drive before starting a block job
that should be cancelled.  This means that the backup job might be
completed already before block-job-cancel is invoked; thus making the
test either fail (currently) or moot if cancel_and_wait() ignored this
condition.  Fix this by pausing the drive before starting the job.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/055 | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index e1206caf9b..8a5d9fd269 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,7 +48,7 @@ class TestSingleDrive(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
         self.vm.add_drive(blockdev_target_img, interface="none")
         if iotests.qemu_default_machine == 'pc':
             self.vm.add_drive(None, 'media=cdrom', 'ide')
@@ -65,10 +65,11 @@ class TestSingleDrive(iotests.QMPTestCase):
     def do_test_cancel(self, cmd, target):
         self.assert_no_active_block_jobs()
 
+        self.vm.pause_drive('drive0')
         result = self.vm.qmp(cmd, device='drive0', target=target, sync='full')
         self.assert_qmp(result, 'return', {})
 
-        event = self.cancel_and_wait()
+        event = self.cancel_and_wait(resume=True)
         self.assert_qmp(event, 'data/type', 'backup')
 
     def test_cancel_drive_backup(self):
@@ -166,7 +167,7 @@ class TestSetSpeed(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
         self.vm.add_drive(blockdev_target_img, interface="none")
         self.vm.launch()
 
@@ -246,6 +247,8 @@ class TestSetSpeed(iotests.QMPTestCase):
     def test_set_speed_invalid_blockdev_backup(self):
         self.do_test_set_speed_invalid('blockdev-backup',  'drive1')
 
+# Note: We cannot use pause_drive() here, or the transaction command
+#       would stall.  Instead, we limit the block job speed here.
 class TestSingleTransaction(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
@@ -271,7 +274,8 @@ class TestSingleTransaction(iotests.QMPTestCase):
                 'type': cmd,
                 'data': { 'device': 'drive0',
                           'target': target,
-                          'sync': 'full' },
+                          'sync': 'full',
+                          'speed': 64 * 1024 },
             }
         ])
 
@@ -289,12 +293,12 @@ class TestSingleTransaction(iotests.QMPTestCase):
     def do_test_pause(self, cmd, target, image):
         self.assert_no_active_block_jobs()
 
-        self.vm.pause_drive('drive0')
         result = self.vm.qmp('transaction', actions=[{
                 'type': cmd,
                 'data': { 'device': 'drive0',
                           'target': target,
-                          'sync': 'full' },
+                          'sync': 'full',
+                          'speed': 64 * 1024 },
             }
         ])
         self.assert_qmp(result, 'return', {})
@@ -302,7 +306,9 @@ class TestSingleTransaction(iotests.QMPTestCase):
         result = self.vm.qmp('block-job-pause', device='drive0')
         self.assert_qmp(result, 'return', {})
 
-        self.vm.resume_drive('drive0')
+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
+        self.assert_qmp(result, 'return', {})
+
         self.pause_job('drive0')
 
         result = self.vm.qmp('query-block-jobs')
@@ -461,7 +467,7 @@ class TestDriveCompression(iotests.QMPTestCase):
             pass
 
     def do_prepare_drives(self, fmt, args, attach_target):
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
 
         qemu_img('create', '-f', fmt, blockdev_target_img,
                  str(TestDriveCompression.image_len), *args)
@@ -500,10 +506,11 @@ class TestDriveCompression(iotests.QMPTestCase):
 
         self.assert_no_active_block_jobs()
 
+        self.vm.pause_drive('drive0')
         result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args)
         self.assert_qmp(result, 'return', {})
 
-        event = self.cancel_and_wait()
+        event = self.cancel_and_wait(resume=True)
         self.assert_qmp(event, 'data/type', 'backup')
 
         self.vm.shutdown()
-- 
2.13.6

[Qemu-devel] [PATCH 4/5] iotests: Make 083 less flaky

[Max Reitz]

083 has (at least) two issues:

1. By launching the nbd-fault-injector in background, it may not be
   scheduled until the first grep on its output file is executed.
   However, until then, that file may not have been created yet -- so it
   either does not exist yet (thus making the grep emit an error), or it
   does exist but contains stale data (thus making the rest of the test
   case work connect to a wrong address).
   Fix this by explicitly overwriting the output file before executing
   nbd-fault-injector.

2. The nbd-fault-injector prints things other than "Listening on...".
   It also prints a "Closing connection" message from time to time.  We
   currently invoke sed on the whole file in the hope of it only
   containing the "Listening on..." line yet.  That hope is sometimes
   shattered by the brutal reality of race conditions, so invoke grep
   before sed.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/083 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 0306f112da..2f6444eeb9 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -86,6 +86,7 @@ EOF
 
 	rm -f "$TEST_DIR/nbd.sock"
 
+        echo > "$TEST_DIR/nbd-fault-injector.out"
 	$PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 &
 
 	# Wait for server to be ready
@@ -94,7 +95,7 @@ EOF
 	done
 
 	# Extract the final address (port number has now been assigned in tcp case)
-	nbd_addr=$(sed 's/Listening on \(.*\)$/\1/' "$TEST_DIR/nbd-fault-injector.out")
+        nbd_addr=$(grep 'Listening on ' "$TEST_DIR/nbd-fault-injector.out" | sed 's/Listening on \(.*\)$/\1/')
 
 	if [ "$proto" = "tcp" ]; then
 		nbd_url="nbd+tcp://$nbd_addr/$export_name"
-- 
2.13.6

[Qemu-devel] [PATCH 5/5] iotests: Make 136 less flaky

[Max Reitz]

136 executes some AIO requests without a final aio_flush; then it
advances the virtual clock and thus expects the last access time of the
device to be less than the current time when queried (i.e. idle_time_ns
to be greater than 0).  However, without the aio_flush, some requests
may be settled after the clock_step invocation.  In that case,
idle_time_ns would be 0 and the test fails.

Fix this by adding an aio_flush if any AIO request but some other
aio_flush has been executed.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/136 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index 4b994897af..88b97ea7c6 100644
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -238,6 +238,18 @@ sector = "%d"
         for i in range(failed_wr_ops):
             ops.append("aio_write %d 512" % bad_offset)
 
+        # We need an extra aio_flush to settle all outstanding AIO
+        # operations before we can advance the virtual clock, so that
+        # the last access happens before clock_step and idle_time_ns
+        # will be greater than 0
+        extra_flush = 0
+        if rd_ops + wr_ops + invalid_rd_ops + invalid_wr_ops + \
+                failed_rd_ops + failed_wr_ops > 0:
+            extra_flush = 1
+
+        if extra_flush > 0:
+            ops.append("aio_flush")
+
         if failed_wr_ops > 0:
             highest_offset = max(highest_offset, bad_offset + 512)
 
@@ -251,7 +263,7 @@ sector = "%d"
         self.total_wr_bytes += wr_ops * wr_size
         self.total_wr_ops += wr_ops
         self.total_wr_merged += wr_merged
-        self.total_flush_ops += flush_ops
+        self.total_flush_ops += flush_ops + extra_flush
         self.invalid_rd_ops += invalid_rd_ops
         self.invalid_wr_ops += invalid_wr_ops
         self.failed_rd_ops += failed_rd_ops
-- 
2.13.6

Re: [Qemu-devel] [PATCH 1/5] iotests: Make 030 less flaky

[Eric Blake]

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

On 11/08/2017 07:38 PM, Max Reitz wrote:
> This patch fixes two race conditions in 030:
> 
> 1. The first is in TestENSPC.test_enospc().  After resuming the job,

s/ENSPC/ENOSPC/

>    querying it to confirm it is no longer paused may fail because in the
>    meantime it might have completed already.  The same was fixed in
>    TestEIO.test_ignore() already (in commit
>    2c3b44da07d341557a8203cc509ea07fe3605e11).
> 
> 2. The second is in TestSetSpeed.test_set_speed_invalid(): Here, a
>    stream job is started on a drive without any break points, with a
>    block-job-set-speed invoked subsequently.  However, without any break
>    points, the job might have completed in the meantime (on tmpfs at
>    least); or it might complete before cancel_and_wait() which expects
>    the job to still exist.  This can be fixed like everywhere else by
>    pausing the drive (installing break points) before starting the job
>    and letting cancel_and_wait() resume it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/030 | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/5] iotests: Add missing 'blkdebug::' in 040

[Eric Blake]

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

On 11/08/2017 07:38 PM, Max Reitz wrote:
> 040 tries to invoke pause_drive() on a drive that does not use blkdebug.
> Good idea, but let's use blkdebug to make it actually work.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/040 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index c284d08796..90b5b4f2ad 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -289,7 +289,7 @@ class TestSetSpeed(ImageCommitTestCase):
>          qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
>          qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 0 512', test_img)
>          qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', mid_img)
> -        self.vm = iotests.VM().add_drive(test_img)
> +        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
>          self.vm.launch()
>  
>      def tearDown(self):
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 3/5] iotests: Make 055 less flaky

[Eric Blake]

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

On 11/08/2017 07:38 PM, Max Reitz wrote:
> First of all, test 055 does a valiant job of invoking pause_drive()
> sometimes, but that is worth nothing without blkdebug.  So the first
> thing to do is to sprinkle a couple of "blkdebug::" in there -- with the
> exception of the transaction tests, because the blkdebug break points
> make the transaction QMP command hang (which is bad).  In that case, we
> can get away with throttling the block job that it effectively is
> paused.
> 
> Then, 055 usually does not pause the drive before starting a block job
> that should be cancelled.  This means that the backup job might be
> completed already before block-job-cancel is invoked; thus making the
> test either fail (currently) or moot if cancel_and_wait() ignored this
> condition.  Fix this by pausing the drive before starting the job.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/055 | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 4/5] iotests: Make 083 less flaky

[Eric Blake]

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

On 11/08/2017 07:38 PM, Max Reitz wrote:
> 083 has (at least) two issues:

I think I hit one of them intermittently yesterday; thanks for
diagnosing these (and like you say, there may be more lurking, but we'll
whack them separately if we can reproduce and identify them).

> 
> 1. By launching the nbd-fault-injector in background, it may not be
>    scheduled until the first grep on its output file is executed.
>    However, until then, that file may not have been created yet -- so it
>    either does not exist yet (thus making the grep emit an error), or it
>    does exist but contains stale data (thus making the rest of the test
>    case work connect to a wrong address).
>    Fix this by explicitly overwriting the output file before executing
>    nbd-fault-injector.
> 
> 2. The nbd-fault-injector prints things other than "Listening on...".
>    It also prints a "Closing connection" message from time to time.  We
>    currently invoke sed on the whole file in the hope of it only
>    containing the "Listening on..." line yet.  That hope is sometimes
>    shattered by the brutal reality of race conditions, so invoke grep
>    before sed.

Invoking 'grep | sed' is almost always a waste of a process; sed can do
the job alone.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/083 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
> index 0306f112da..2f6444eeb9 100755
> --- a/tests/qemu-iotests/083
> +++ b/tests/qemu-iotests/083
> @@ -86,6 +86,7 @@ EOF
>  
>  	rm -f "$TEST_DIR/nbd.sock"
>  
> +        echo > "$TEST_DIR/nbd-fault-injector.out"

This makes the file contain a blank line.  Would it be any better as a
truly empty file, as in:

: > "$TEST_DIR/nbd-fault-injector.out"

>  	$PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 &
>  
>  	# Wait for server to be ready
> @@ -94,7 +95,7 @@ EOF
>  	done
>  
>  	# Extract the final address (port number has now been assigned in tcp case)
> -	nbd_addr=$(sed 's/Listening on \(.*\)$/\1/' "$TEST_DIR/nbd-fault-injector.out")
> +        nbd_addr=$(grep 'Listening on ' "$TEST_DIR/nbd-fault-injector.out" | sed 's/Listening on \(.*\)$/\1/')

Fixing TAB damage while at it - nice.

Here's how to do it using just sed, and with less typing:

    nbd_addr=$(sed -n 's/^Listening on //p' \
               "$TEST_DIR/nbd-fault-injector.out")

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 5/5] iotests: Make 136 less flaky

[Eric Blake]

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

On 11/08/2017 07:38 PM, Max Reitz wrote:
> 136 executes some AIO requests without a final aio_flush; then it
> advances the virtual clock and thus expects the last access time of the
> device to be less than the current time when queried (i.e. idle_time_ns
> to be greater than 0).  However, without the aio_flush, some requests
> may be settled after the clock_step invocation.  In that case,
> idle_time_ns would be 0 and the test fails.
> 
> Fix this by adding an aio_flush if any AIO request but some other

s/but/other than/

> aio_flush has been executed.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/136 | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/5] iotests: Make some tests less flaky

[Stefan Hajnoczi]

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

On Thu, Nov 09, 2017 at 02:37:59AM +0100, Max Reitz wrote:
> There are a couple of tests that fail (on my machine) from time to
> time (and by that I mean that recently I've rarely ever had a test run
> with both 083 and 136 working on first try).
> This series should fix most (at least the issues I am aware of).
> 
> Notes:
> - 083 might have another issue, but if so it occurs extremely rarely and
>   so I was unable to debug it.
> 
> - 129 is flaky, too, because it tries to use block jobs with BB
>   throttling -- however, block jobs ignore that these days.  Making it
>   use a throttle filter node will require quite a bit of work.  See
>   http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00111.html
>   for more.
> 
> - 194 sometimes hangs because the source VM fails to drain its drive.
>   This is probably not an issue with the test, but actually an issue in
>   qemu.  See
>   http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00256.html
>   for more.
> 
> 
> "All tests have passed, let's ship it!"
> -- Me, 2:36 am
> 
> 
> (Editor's note: "all" means raw/file, qcow2/file, and raw/nbd.)
> 
> 
> Max Reitz (5):
>   iotests: Make 030 less flaky
>   iotests: Add missing 'blkdebug::' in 040
>   iotests: Make 055 less flaky
>   iotests: Make 083 less flaky
>   iotests: Make 136 less flaky
> 
>  tests/qemu-iotests/030 |  8 ++++++--
>  tests/qemu-iotests/040 |  2 +-
>  tests/qemu-iotests/055 | 25 ++++++++++++++++---------
>  tests/qemu-iotests/083 |  3 ++-
>  tests/qemu-iotests/136 | 14 +++++++++++++-
>  5 files changed, 38 insertions(+), 14 deletions(-)
> 
> -- 
> 2.13.6
> 
> 

Thanks for these fixes!  QEMU 2.11 material.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 4/5] iotests: Make 083 less flaky

[Max Reitz]

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

On 2017-11-09 15:11, Eric Blake wrote:
> On 11/08/2017 07:38 PM, Max Reitz wrote:
>> 083 has (at least) two issues:
> 
> I think I hit one of them intermittently yesterday; thanks for
> diagnosing these (and like you say, there may be more lurking, but we'll
> whack them separately if we can reproduce and identify them).
> 
>>
>> 1. By launching the nbd-fault-injector in background, it may not be
>>    scheduled until the first grep on its output file is executed.
>>    However, until then, that file may not have been created yet -- so it
>>    either does not exist yet (thus making the grep emit an error), or it
>>    does exist but contains stale data (thus making the rest of the test
>>    case work connect to a wrong address).
>>    Fix this by explicitly overwriting the output file before executing
>>    nbd-fault-injector.
>>
>> 2. The nbd-fault-injector prints things other than "Listening on...".
>>    It also prints a "Closing connection" message from time to time.  We
>>    currently invoke sed on the whole file in the hope of it only
>>    containing the "Listening on..." line yet.  That hope is sometimes
>>    shattered by the brutal reality of race conditions, so invoke grep
>>    before sed.
> 
> Invoking 'grep | sed' is almost always a waste of a process; sed can do
> the job alone.
> 
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/083 | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
>> index 0306f112da..2f6444eeb9 100755
>> --- a/tests/qemu-iotests/083
>> +++ b/tests/qemu-iotests/083
>> @@ -86,6 +86,7 @@ EOF
>>  
>>  	rm -f "$TEST_DIR/nbd.sock"
>>  
>> +        echo > "$TEST_DIR/nbd-fault-injector.out"
> 
> This makes the file contain a blank line.  Would it be any better as a
> truly empty file, as in:
> 
> : > "$TEST_DIR/nbd-fault-injector.out"

Although ":>" looks funny, I guess I'd rather stay with the echo...
Yes, it may look stupid to you, but I would have to look up what exactly
that will do, whereas the echo is clear.

And in the end, both work equally fine.

>>  	$PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 &
>>  
>>  	# Wait for server to be ready
>> @@ -94,7 +95,7 @@ EOF
>>  	done
>>  
>>  	# Extract the final address (port number has now been assigned in tcp case)
>> -	nbd_addr=$(sed 's/Listening on \(.*\)$/\1/' "$TEST_DIR/nbd-fault-injector.out")
>> +        nbd_addr=$(grep 'Listening on ' "$TEST_DIR/nbd-fault-injector.out" | sed 's/Listening on \(.*\)$/\1/')
> 
> Fixing TAB damage while at it - nice.
> 
> Here's how to do it using just sed, and with less typing:
> 
>     nbd_addr=$(sed -n 's/^Listening on //p' \
>                "$TEST_DIR/nbd-fault-injector.out")

Oh, nice!  Will do, thanks.

Max


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