[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers
From: |
Roman Kagan |
Subject: |
Re: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers |
Date: |
Wed, 30 May 2018 17:16:56 +0300 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
On Wed, May 30, 2018 at 03:53:52PM +0200, Max Reitz wrote:
> On 2018-05-30 15:47, Roman Kagan wrote:
> > On Wed, May 30, 2018 at 02:35:34PM +0200, Max Reitz wrote:
> >> On 2018-04-26 18:19, Roman Kagan wrote:
> >>> Some iotests assume availability of certain block drivers, and fail if
> >>> the driver is not supported by QEMU because it was disabled at configure
> >>> time.
> >>>
> >>> This series tries to address that, by making QEMU report the actual list
> >>> of supported block drivers in response to "-drive format=?", and using
> >>> this information to skip the parts of the io testsuite that can not be
> >>> run in this configuration.
> >>>
> >>> Roman Kagan (17):
> >>> block: iterate_format with account of whitelisting
> >>> iotests: iotests.py: prevent deadlock in subprocess
> >>> iotests: ask qemu for supported formats
> >>> iotest 030: skip quorum test setup/teardown too
> >>> iotest 030: require blkdebug
> >>> iotest 055: skip unsupported backup target formats
> >>> iotest 055: require blkdebug
> >>> iotest 056: skip testcases using blkdebug if disabled
> >>> iotest 071: notrun if blkdebug or blkverify is disabled
> >>> iotest 081: notrun if quorum is disabled
> >>> iotest 087: notrun if null-co is disabled
> >>> iotest 093: notrun if null-co or null-aio is disabled
> >>> iotest 099: notrun if blkdebug or blkverify is disabled
> >>> iotest 124: skip testcases using blkdebug if disabled
> >>> iotest 139: skip testcases using disabled drivers
> >>> iotest 147: notrun if nbd is disabled
> >>> iotest 184: notrun if null-co or throttle is disabled
> >>>
> >>> include/block/block.h | 2 +-
> >>> block.c | 23 ++++++++++++++++++----
> >>> blockdev.c | 4 +++-
> >>> qemu-img.c | 2 +-
> >>> tests/qemu-iotests/030 | 7 +++++++
> >>> tests/qemu-iotests/055 | 13 ++++++++++++
> >>> tests/qemu-iotests/056 | 3 +++
> >>> tests/qemu-iotests/071 | 1 +
> >>> tests/qemu-iotests/081 | 1 +
> >>> tests/qemu-iotests/087 | 1 +
> >>> tests/qemu-iotests/093 | 1 +
> >>> tests/qemu-iotests/099 | 1 +
> >>> tests/qemu-iotests/124 | 5 +++++
> >>> tests/qemu-iotests/139 | 4 ++++
> >>> tests/qemu-iotests/147 | 1 +
> >>> tests/qemu-iotests/184 | 1 +
> >>> tests/qemu-iotests/common.rc | 19 ++++++++++++++++++
> >>> tests/qemu-iotests/iotests.py | 46
> >>> ++++++++++++++++++++++++++++++++-----------
> >>> 18 files changed, 117 insertions(+), 18 deletions(-)
> >>
> >> I'll stop reviewing this series for now, because there are more iotests
> >> that use drivers outside of their format/protocol combination.
> >>
> >> For instance:
> >>
> >> $ grep -l null-co ??? | wc -l
> >> 15
> >> $ grep -l blkdebug ??? | wc -l
> >> 30
> >> $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l
> >> 22
> >>
> >> As I've written in my reply to patch 8, I'm not sure whether it's the
> >> right solution to check for the availability of these block drivers in
> >> every single test that needs them. It makes sense for quorum, because
> >> quorum needs an external library for hashing, so it may not be trivial
> >> to enable. But it does not seem too useful for other formats that do
> >> not have such a dependency (e.g. null-co, blkdebug, raw).
> >>
> >> The thing is that it's OK to whitelist everything for testing, and then
> >> disable some drivers when building a release. I don't think one needs
> >> to run the iotests with the release version if the whole difference is
> >> whether some drivers have been disabled or not.
> >
> > This patchset was created when we started to run (quick) iotests as a part
> > of the package build, which looked like a decent alternative to building
> > separate CI infrastructure.
> >
> > If there's no general interest in running iotests against QEMU built in
> > a release configuration with certain drivers blacklisted, I think we
> > should be fine handling this locally.
>
> It's not like the majority of drivers uses blkdebug, but still, it seems
> at least suboptimal to run the tests if you then disable a big chunk of
> them because some drivers were not whitelisted.
Indeed. In fact, our configuration (based on RedHat's) has those three
(null-co, blkdebug, raw) enabled so the patchset didn't result in
massive test skips.
> If you're OK with a local solution... OK, but naively I'd think that
> it'd be better to just build two versions for packaging: One that's
> actually going to be packaged, and one that's used for testing. (Though
> I don't know how well that fits into your process.)
It doesn't: we just added a couple of lines to the %check section of the
RPM .spec
> In any case, I'm not fully opposed to this series, but it would need to
> be complete and make every test require every block driver that is not
> part of its protocol/format combination.
OK this doesn't look like too much of work; I'll see if I can do it
quickly.
Thanks,
Roman.