Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver

[Max Reitz]

On 25.09.20 17:32, Vladimir Sementsov-Ogievskiy wrote:
> 25.09.2020 18:11, Vladimir Sementsov-Ogievskiy wrote:
>> 25.09.2020 12:11, Max Reitz wrote:
>>> On 25.09.20 10:49, Vladimir Sementsov-Ogievskiy wrote:
>>>> 25.09.2020 11:26, Max Reitz wrote:
>>>>> On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>>>> <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>    tests/qemu-iotests/298     | 186
>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>    tests/qemu-iotests/298.out |   5 +
>>>>>>    tests/qemu-iotests/group   |   1 +
>>>>>>    3 files changed, 192 insertions(+)
>>>>>>    create mode 100644 tests/qemu-iotests/298
>>>>>>    create mode 100644 tests/qemu-iotests/298.out
>>>
>>> [...]
>>>
>>>>>> +class TestTruncate(iotests.QMPTestCase):
>>>>>
>>>>> The same decorator could be placed here, although this class doesn’t
>>>>> start a VM, and so is unaffected by the allowlist.  Still may be
>>>>> relevant in case of block modules, I don’t know.
>>>>
>>>> Or just global test skip at file top
>>>
>>> Hm.  Like verify_quorum()?  Is there a generic function for that
>>> already?
>>>
>>> [...]
>>>
>>>>>> +        # Probably we'll want preallocate filter to keep align to
>>>>>> cluster when
>>>>>> +        # shrink preallocation, so, ignore small differece
>>>>>> +        self.assertLess(abs(stat.st_size - refstat.st_size), 64 *
>>>>>> 1024)
>>>>>> +
>>>>>> +        # Preallocate filter may leak some internal clusters (for
>>>>>> example, if
>>>>>> +        # guest write far over EOF, skipping some clusters - they
>>>>>> will remain
>>>>>> +        # fallocated, preallocate filter don't care about such
>>>>>> leaks, it drops
>>>>>> +        # only trailing preallocation.
>>>>>
>>>>> True, but that isn’t what’s happening here.  (We only write 10M at
>>>>> 0, so
>>>>> there are no gaps.)  Why do we need this 1M margin?
>>>>
>>>> We write 10M, but qcow2 also writes metadata as it wants
>>>
>>> Ah, yes, sure.  Shouldn’t result in 1M, but why not.
>>>
>>>>>> +        self.assertLess(abs(stat.st_blocks - refstat.st_blocks) *
>>>>>> 512,
>>>>>> +                        1024 * 1024)
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>>>>>> index ff59cfd2d4..15d5f9619b 100644
>>>>>> --- a/tests/qemu-iotests/group
>>>>>> +++ b/tests/qemu-iotests/group
>>>>>> @@ -307,6 +307,7 @@
>>>>>>    295 rw
>>>>>>    296 rw
>>>>>>    297 meta
>>>>>> +298 auto quick
>>>>>
>>>>> I wouldn’t mark it as quick, there is at least one preallocate=full of
>>>>> 140M, and one of 40M, plus multiple 10M data writes and falloc
>>>>> preallocations.
>>>>>
>>>>> Also, since you mark it as “auto”, have you run this test on all
>>>>> CI-relevant hosts?  (Among other things I can’t predict) I wonder how
>>>>> preallocation behaves on macOS.  Just because that one was always a
>>>>> bit
>>>>> weird about not-really-data areas.
>>>>>
>>>>
>>>> Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this..
> 
> Sorry, I see now that it sounds rude.

I found it completely understandable, because I share the same
sentiment.  It’s the reason I’m so hesitant adding tests to auto.

>>> Well, someone has to do it.  The background story is that tests are
>>> added to auto all the time (because “why not”), and then they fail on
>>> BSD or macOS.  We have BSD docker test build targets at least, so they
>>> can be easily tested.  (Well, it takes like half an hour, but you know.)
>>>
>>> (We don’t have macOS builds, as far as I can tell, but I personally
>>> don’t even know why we run the iotests on macOS at all.  (Well, I also
>>> wonder about the BSDs, but given the test build targets, I shouldn’t
>>> complain, I suppose.))
>>>
>>> (Though macOS isn’t part of the gating CI, is it?  I seem to remember
>>> macOS errors are generally only reported to me half a week after the
>>> pull request is merged, which is even worse.)
>>>
>>> Anyway.  I just ran the test for OpenBSD
>>> (EXTRA_CONFIGURE_OPTS='--target-list=x86_64-softmmu' \
>>>     make vm-build-openbsd)
>>
>> Oh, I didn't know that it's so simple.

It kind of is simple, but it still takes so long that I don’t consider
it simple.

>> What another things you are
>> running before sending a PULL?

Actually, I’m not running any of the vm-build-* targets.  (If I did, I
wouldn’t have to ask you whether you did, because I’d just run them
anyway and then tell you about any potential failures.)

I compile everything on Fedora and Arch (x86-64), -m32 and -m64, clang
and gcc, and one time with mingw.  I run make check on everything but
mingw, and then run the following iotests on Fedora gcc-m64 and Arch
clang-m32 (on tmpfs):

-qcow2 -x auto, -qcow2 -o compat=0.10, -qcow2 -o refcount_bits=1,
-qcow2 -o data_file='$TEST_IMG.ext_data_file', -nbd, -raw, -luks, -vmdk,
-vhdx, -qcow, -vdi, -vpc, -qed, -cloop, -parallels, -bochs

And on non-tmpfs: -c none -qcow2 142 199

(At some point that meant that all iotests that don’t require root are
at least run once.  I should check whether that’s still the case, I
suppose...)

>>> and got some failures:
>>>
>>> --- /home/qemu/qemu-test.PGo2ls/src/tests/qemu-iotests/298.out  Fri Sep
>>> 25 07:10:31 2020
>>> +++ /home/qemu/qemu-test.PGo2ls/build/tests/qemu-iotests/298.out.bad
>>> Fri Sep 25 08:57:56 2020
>>> @@ -1,5 +1,67 @@
>>> -.............
>>> +qemu-io: Failed to resize underlying file: Unsupported preallocation
>>> mode: falloc
> 
> [..]
> 
>>> -OK
>>> +FAILED (failures=6)
>>>
>>>> If I don't put new test in "auto", is there any chance that test would
>>>> be automatically run somewhere?
>>>
>>> I run all tests before pull requests at least.
>>>
> 
> OK, so it doesn't work on BSD at all. I don't really want to
> investigate, but seems it's because of absence of fallocate. So let's
> drop "auto" group.

I’d be OK with that.

> Another thing: maybe, add auto-linux test group for running only in
> linux-build? So, new tests will be added to it because why not, and we
> will not bother with BSD and MacOS?

We have _supported_os in bash tests and supported_platforms in Python
tests, so if you want a test in auto but run it only on Linux, you can
specify that there.

Max

signature.asc
Description: OpenPGP digital signature