[Qemu-devel] [PATCH 0/4] iotests: Selfish patches

[Qemu-devel] [PATCH 0/4] iotests: Selfish patches

[Max Reitz]

Hi,

These are some rather selfish iotests patches.  The first patch helps me
personally because I tend to run the tests over SSH and forget to set
$DISPLAY.  That makes test 139 skip the tests annotated with
skip_if_unsupprted(), because iotests.py can no longer determine the
list of whitelisted formats.

Patch 2 through 4 are specifically for RHEL.  We have not whitelisted
null-aio, so it would be nice if tests didn’t require it.  Sorry, I
don’t have a better reason to give.
In all seriousness, null-co is used widely in many tests, it basically
is our standard null driver.  Tests should prefer it over null-aio, just
for consistency alone.  It is not completely unreasonable to treat
null-aio as optional.  I guess.

Final note: The best thing would probably to skip the null-aio tests in
093/136 if there is no null-aio support.  However, I didn’t get anything
to work: Annotating with @iotests.skip_if_unsupported() didn’t work
because the format name is a class instance attribute; and just
iotests.skipTest() didn’t work because that would print 's' characters
instead of '.' in the output (and emit the number of skipped tests), so
the comparison against the reference output fails...  Which is why I
decided to just run the test with null-co then.  That means that some
tests run twice with null-co (if there is no null-aio support), but
that’s not too bad.  Just ugly.


Max Reitz (4):
  iotests: Add -display none to the qemu options
  iotests: Prefer null-co over null-aio
  iotests: Test driver whitelisting in 093
  iotests: Test driver whitelisting in 136

 tests/qemu-iotests/093   | 22 +++++++++++++++-------
 tests/qemu-iotests/136   | 17 +++++++++++++----
 tests/qemu-iotests/245   |  2 +-
 tests/qemu-iotests/check |  2 +-
 4 files changed, 30 insertions(+), 13 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/4] iotests: Add -display none to the qemu options

[Max Reitz]

Without this argument, qemu will print an angry message about not being
able to connect to a display server if $DISPLAY is not set.  For me,
that breaks iotests.supported_formats() because it thus only sees
["Could", "not", "connect"] as the supported formats.

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

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index f9c24b6753..d6fec4bf3c 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -130,7 +130,7 @@ export CACHEMODE="writeback"
 export QEMU_IO_OPTIONS=""
 export QEMU_IO_OPTIONS_NO_FMT=""
 export CACHEMODE_IS_DEFAULT=true
-export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
+export QEMU_OPTIONS="-nodefaults -machine accel=qtest -display none"
 export VALGRIND_QEMU=
 export IMGKEYSECRET=
 export IMGOPTSSYNTAX=false
-- 
2.21.0

[Qemu-devel] [PATCH 2/4] iotests: Prefer null-co over null-aio

[Max Reitz]

We use null-co basically everywhere in the iotests.  Unless we want to
test null-aio specifically, we should use it instead (for consistency).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/093 | 7 +++----
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index d88fbc182e..bd56c94708 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -267,13 +267,12 @@ class ThrottleTestCoroutine(ThrottleTestCase):
     test_img = "null-co://"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
-    test_img = "null-aio://"
     max_drives = 3
 
     def setUp(self):
         self.vm = iotests.VM()
         for i in range(0, self.max_drives):
-            self.vm.add_drive(self.test_img, "throttling.iops-total=100")
+            self.vm.add_drive("null-co://", "throttling.iops-total=100")
         self.vm.launch()
 
     def tearDown(self):
@@ -377,10 +376,10 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
     def test_removable_media(self):
         # Add a couple of dummy nodes named cd0 and cd1
-        result = self.vm.qmp("blockdev-add", driver="null-aio",
+        result = self.vm.qmp("blockdev-add", driver="null-co",
                              node_name="cd0")
         self.assert_qmp(result, 'return', {})
-        result = self.vm.qmp("blockdev-add", driver="null-aio",
+        result = self.vm.qmp("blockdev-add", driver="null-co",
                              node_name="cd1")
         self.assert_qmp(result, 'return', {})
 
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a04c6235c1..d888a1354c 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -598,7 +598,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         ##################
         ###### null ######
         ##################
-        opts = {'driver': 'null-aio', 'node-name': 'root', 'size': 1024}
+        opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024}
 
         result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
         self.assert_qmp(result, 'return', {})
-- 
2.21.0

[Qemu-devel] [PATCH 3/4] iotests: Test driver whitelisting in 093

[Max Reitz]

null-aio may not be whitelisted.  If it is not, fall back to null-co.
This may run tests twice in the same configuration, but this is the
simplest way to effectively skip the tests in setUp() (without changing
the output, and while having the respective driver in a class
attribute).

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

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index bd56c94708..d6f285001a 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -22,9 +22,11 @@
 import iotests
 
 nsec_per_sec = 1000000000
+supported_null_drivers = list(filter(lambda f: f.startswith('null-'),
+                                     iotests.supported_formats()))
 
 class ThrottleTestCase(iotests.QMPTestCase):
-    test_img = "null-aio://"
+    test_driver = "null-aio"
     max_drives = 3
 
     def blockstats(self, device):
@@ -36,9 +38,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
         raise Exception("Device not found for blockstats: %s" % device)
 
     def setUp(self):
+        global supported_null_drivers
+        if self.test_driver not in supported_null_drivers:
+            # Silently fall back to supported driver
+            self.test_driver = supported_null_drivers[0]
+
         self.vm = iotests.VM()
         for i in range(0, self.max_drives):
-            self.vm.add_drive(self.test_img)
+            self.vm.add_drive(self.test_driver + "://")
         self.vm.launch()
 
     def tearDown(self):
@@ -264,7 +271,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
         self.assertEqual(self.blockstats('drive1')[0], 4096)
 
 class ThrottleTestCoroutine(ThrottleTestCase):
-    test_img = "null-co://"
+    test_driver = "null-co"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
     max_drives = 3
@@ -426,4 +433,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 
 if __name__ == '__main__':
+    if 'null-co' not in supported_null_drivers:
+        iotests.notrun('null-co driver support missing')
     iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-devel] [PATCH 4/4] iotests: Test driver whitelisting in 136

[Max Reitz]

null-aio may not be whitelisted.  If it is not, fall back to null-co.
This may run tests twice in the same configuration, but this is the
simplest way to effectively skip the tests in setUp() (without changing
the output, and while having the respective driver in a class
attribute).

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

diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index af7ffa4540..44e3bc1575 100755
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -28,9 +28,11 @@ op_latency = nsec_per_sec // 1000 # See qtest_latency_ns in accounting.c
 bad_sector = 8192
 bad_offset = bad_sector * 512
 blkdebug_file = os.path.join(iotests.test_dir, 'blkdebug.conf')
+supported_null_drivers = list(filter(lambda f: f.startswith('null-'),
+                                     iotests.supported_formats()))
 
 class BlockDeviceStatsTestCase(iotests.QMPTestCase):
-    test_img = "null-aio://"
+    test_driver = "null-aio"
     total_rd_bytes = 0
     total_rd_ops = 0
     total_wr_bytes = 0
@@ -68,6 +70,11 @@ sector = "%d"
         file.close()
 
     def setUp(self):
+        global supported_null_drivers
+        if self.test_driver not in supported_null_drivers:
+            # Silently fall back to supported driver
+            self.test_driver = supported_null_drivers[0]
+
         drive_args = []
         drive_args.append("stats-intervals.0=%d" % interval_length)
         drive_args.append("stats-account-invalid=%s" %
@@ -75,8 +82,8 @@ sector = "%d"
         drive_args.append("stats-account-failed=%s" %
                           (self.account_failed and "on" or "off"))
         self.create_blkdebug_file()
-        self.vm = iotests.VM().add_drive('blkdebug:%s:%s' %
-                                         (blkdebug_file, self.test_img),
+        self.vm = iotests.VM().add_drive('blkdebug:%s:%s://' %
+                                         (blkdebug_file, self.test_driver),
                                          ','.join(drive_args))
         self.vm.launch()
         # Set an initial value for the clock
@@ -336,7 +343,9 @@ class BlockDeviceStatsTestAccountBoth(BlockDeviceStatsTestCase):
     account_failed = True
 
 class BlockDeviceStatsTestCoroutine(BlockDeviceStatsTestCase):
-    test_img = "null-co://"
+    test_driver = "null-co"
 
 if __name__ == '__main__':
+    if 'null-co' not in supported_null_drivers:
+        iotests.notrun('null-co driver support missing')
     iotests.main(supported_fmts=["raw"])
-- 
2.21.0

Re: [Qemu-devel] [PATCH 1/4] iotests: Add -display none to the qemu options

[Thomas Huth]

On 17/05/2019 11.56, Max Reitz wrote:
> Without this argument, qemu will print an angry message about not being
> able to connect to a display server if $DISPLAY is not set.  For me,
> that breaks iotests.supported_formats() because it thus only sees
> ["Could", "not", "connect"] as the supported formats.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/check | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index f9c24b6753..d6fec4bf3c 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -130,7 +130,7 @@ export CACHEMODE="writeback"
>  export QEMU_IO_OPTIONS=""
>  export QEMU_IO_OPTIONS_NO_FMT=""
>  export CACHEMODE_IS_DEFAULT=true
> -export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
> +export QEMU_OPTIONS="-nodefaults -machine accel=qtest -display none"
>  export VALGRIND_QEMU=
>  export IMGKEYSECRET=
>  export IMGOPTSSYNTAX=false
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 1/4] iotests: Add -display none to the qemu options

[Alex Bennée]

Max Reitz <mreitz@redhat.com> writes:

> Without this argument, qemu will print an angry message about not being
> able to connect to a display server if $DISPLAY is not set.  For me,
> that breaks iotests.supported_formats() because it thus only sees
> ["Could", "not", "connect"] as the supported formats.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  tests/qemu-iotests/check | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index f9c24b6753..d6fec4bf3c 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -130,7 +130,7 @@ export CACHEMODE="writeback"
>  export QEMU_IO_OPTIONS=""
>  export QEMU_IO_OPTIONS_NO_FMT=""
>  export CACHEMODE_IS_DEFAULT=true
> -export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
> +export QEMU_OPTIONS="-nodefaults -machine accel=qtest -display none"
>  export VALGRIND_QEMU=
>  export IMGKEYSECRET=
>  export IMGOPTSSYNTAX=false


--
Alex Bennée

Re: [Qemu-devel] [PATCH 3/4] iotests: Test driver whitelisting in 093

[Kevin Wolf]

Am 17.05.2019 um 11:56 hat Max Reitz geschrieben:
> null-aio may not be whitelisted.  If it is not, fall back to null-co.
> This may run tests twice in the same configuration, but this is the
> simplest way to effectively skip the tests in setUp() (without changing
> the output, and while having the respective driver in a class
> attribute).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/093 | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index bd56c94708..d6f285001a 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -22,9 +22,11 @@
>  import iotests
>  
>  nsec_per_sec = 1000000000
> +supported_null_drivers = list(filter(lambda f: f.startswith('null-'),
> +                                     iotests.supported_formats()))

Is this just a convoluted way of writing the following?

    supported_null_drivers = [ f for f in iotests.supported_formats()
                               if f.startswith('null-') ]

>  class ThrottleTestCase(iotests.QMPTestCase):
> -    test_img = "null-aio://"
> +    test_driver = "null-aio"
>      max_drives = 3
>  
>      def blockstats(self, device):
> @@ -36,9 +38,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>          raise Exception("Device not found for blockstats: %s" % device)
>  
>      def setUp(self):
> +        global supported_null_drivers
> +        if self.test_driver not in supported_null_drivers:
> +            # Silently fall back to supported driver
> +            self.test_driver = supported_null_drivers[0]

I think this is what you mentioned in the cover letter:

> Final note: The best thing would probably to skip the null-aio tests in
> 093/136 if there is no null-aio support.  However, I didn’t get anything
> to work: Annotating with @iotests.skip_if_unsupported() didn’t work
> because the format name is a class instance attribute; and just
> iotests.skipTest() didn’t work because that would print 's' characters
>  instead of '.' in the output (and emit the number of skipped tests), so
> the comparison against the reference output fails...

With a little modification to the @skip_if_unsupported() decorator it
can be done. I think I'd prefer this (hacked up on top of this series):

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f811f69135..f83d56b156 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -789,8 +789,11 @@ def skip_if_unsupported(required_formats=[], read_only=False):
        Runs the test if all the required formats are whitelisted'''
     def skip_test_decorator(func):
         def func_wrapper(*args, **kwargs):
-            usf_list = list(set(required_formats) -
-                            set(supported_formats(read_only)))
+            if callable(required_formats):
+                fmts = required_formats(args[0])
+            else:
+                fmts = required_formats
+            usf_list = list(set(fmts) - set(supported_formats(read_only)))
             if usf_list:
                 case_notrun('{}: formats {} are not whitelisted'.format(
                     args[0], usf_list))
diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index d6f285001a..e23a8189bc 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -22,8 +22,6 @@
 import iotests

 nsec_per_sec = 1000000000
-supported_null_drivers = list(filter(lambda f: f.startswith('null-'),
-                                     iotests.supported_formats()))

 class ThrottleTestCase(iotests.QMPTestCase):
     test_driver = "null-aio"
@@ -37,11 +35,12 @@ class ThrottleTestCase(iotests.QMPTestCase):
                 return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
         raise Exception("Device not found for blockstats: %s" % device)

+    def required_driver(self):
+        return self.test_driver
+
     def setUp(self):
-        global supported_null_drivers
-        if self.test_driver not in supported_null_drivers:
-            # Silently fall back to supported driver
-            self.test_driver = supported_null_drivers[0]
+        if not self.required_driver() in iotests.supported_formats():
+            return

         self.vm = iotests.VM()
         for i in range(0, self.max_drives):
@@ -49,6 +48,9 @@ class ThrottleTestCase(iotests.QMPTestCase):
         self.vm.launch()

     def tearDown(self):
+        if not self.required_driver() in iotests.supported_formats():
+            return
+
         self.vm.shutdown()

     def configure_throttle(self, ndrives, params):
@@ -145,6 +147,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
         self.vm.qtest("clock_step %d" % ns)

     # Connect N drives to a VM and test I/O in all of them
+    @iotests.skip_if_unsupported(required_driver)
     def test_all(self):
         params = {"bps": 4096,
                   "bps_rd": 4096,
@@ -163,6 +166,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
                 self.do_test_throttle(ndrives, 5, limits)

     # Connect N drives to a VM and test I/O in just one of them a time
+    @iotests.skip_if_unsupported(required_driver)
     def test_one(self):
         params = {"bps": 4096,
                   "bps_rd": 4096,
@@ -180,6 +184,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
                 self.configure_throttle(self.max_drives, limits)
                 self.do_test_throttle(1, 5, limits, drive)

+    @iotests.skip_if_unsupported(required_driver)
     def test_burst(self):
         params = {"bps": 4096,
                   "bps_rd": 4096,
@@ -218,6 +223,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
     # Test that removing a drive from a throttle group should not
     # affect the remaining members of the group.
     # https://bugzilla.redhat.com/show_bug.cgi?id=1535914
+    @iotests.skip_if_unsupported(required_driver)
     def test_remove_group_member(self):
         # Create a throttle group with two drives
         # and set a 4 KB/s read limit.
@@ -433,6 +439,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):


 if __name__ == '__main__':
-    if 'null-co' not in supported_null_drivers:
+    if 'null-co' not in iotests.supported_formats():
         iotests.notrun('null-co driver support missing')
     iotests.main(supported_fmts=["raw"])

Re: [Qemu-devel] [PATCH 3/4] iotests: Test driver whitelisting in 093

[Max Reitz]

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

On 17.05.19 13:00, Kevin Wolf wrote:
> Am 17.05.2019 um 11:56 hat Max Reitz geschrieben:
>> null-aio may not be whitelisted.  If it is not, fall back to null-co.
>> This may run tests twice in the same configuration, but this is the
>> simplest way to effectively skip the tests in setUp() (without changing
>> the output, and while having the respective driver in a class
>> attribute).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/093 | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>> index bd56c94708..d6f285001a 100755
>> --- a/tests/qemu-iotests/093
>> +++ b/tests/qemu-iotests/093
>> @@ -22,9 +22,11 @@
>>  import iotests
>>  
>>  nsec_per_sec = 1000000000
>> +supported_null_drivers = list(filter(lambda f: f.startswith('null-'),
>> +                                     iotests.supported_formats()))
> 
> Is this just a convoluted way of writing the following?
> 
>     supported_null_drivers = [ f for f in iotests.supported_formats()
>                                if f.startswith('null-') ]

Well, it’s a different way, yes.  I would call it the Ruby way, but that
would have been "iotests.supported_formats() & ['null-co', 'null-aio']".
 (And with list(set() & set()) it suddenly isn’t short anymore.)

Sorry, I’m just not suited for Python.

>>  class ThrottleTestCase(iotests.QMPTestCase):
>> -    test_img = "null-aio://"
>> +    test_driver = "null-aio"
>>      max_drives = 3
>>  
>>      def blockstats(self, device):
>> @@ -36,9 +38,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>          raise Exception("Device not found for blockstats: %s" % device)
>>  
>>      def setUp(self):
>> +        global supported_null_drivers
>> +        if self.test_driver not in supported_null_drivers:
>> +            # Silently fall back to supported driver
>> +            self.test_driver = supported_null_drivers[0]
> 
> I think this is what you mentioned in the cover letter:
> 
>> Final note: The best thing would probably to skip the null-aio tests in
>> 093/136 if there is no null-aio support.  However, I didn’t get anything
>> to work: Annotating with @iotests.skip_if_unsupported() didn’t work
>> because the format name is a class instance attribute; and just
>> iotests.skipTest() didn’t work because that would print 's' characters
>>  instead of '.' in the output (and emit the number of skipped tests), so
>> the comparison against the reference output fails...
> 
> With a little modification to the @skip_if_unsupported() decorator it
> can be done. I think I'd prefer this (hacked up on top of this series):

Hm.  I really don’t like having to put it above every single test
method, and putting it above setUp() has the problem of not actually
skipping the test.

I guess I’ll look back into filtering the test output to remove the
“skipped” stuff.

Max


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