[PATCH 0/7] iotests/129: Fix it

Max Reitz posted 7 patches 1 week ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210113140616.150283-1-mreitz@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
tests/qemu-iotests/124        |  8 +---
tests/qemu-iotests/129        | 76 ++++++++++++++++++++++-------------
tests/qemu-iotests/iotests.py | 11 +++--
3 files changed, 55 insertions(+), 40 deletions(-)

[PATCH 0/7] iotests/129: Fix it

Posted by Max Reitz 1 week ago
Hi,

There are some problems with iotests 129 (perhaps more than these, but
these are the ones I know of):

1. It checks @busy to see whether a block job is still running; however,
   block jobs tend to unset @busy all the time (when they yield).
   [Fixed by patch 3]

2. It uses blockdev throttling, which quite some time ago has been moved
   to the BB level; since then, such throttling will no longer affect
   block jobs.  We can get throttling to work by using a throttle filter
   node.
   [Fixed by patch 4]

3. The mirror job has a large buffer size by default.  A simple drain
   may lead to it making significant process, which is kind of
   dangerous, because we don’t want the job to complete.
   To get around this, we can simply limit its buffer size.  (And we
   should make the commit job an actual commit job instead of an active
   commit (which is just mirror), because the commit interface does not
   allow setting a buffer size.)
   [Fixed by patches 5 and 6]

This series fixes those things, and now 129 seems to reliably pass for
me.


Apart from the major issues above, there are also minor flaws:

- It doesn’t remove the test images.
  [Fixed by patches 1 and 2]

- pylint and mypy complain.
  (Running mypy with the options given in 297.)
  [Patch 4 removes one pylint complaint; patch 7 the rest.]


Max Reitz (7):
  iotests: Move try_remove to iotests.py
  iotests/129: Remove test images in tearDown()
  iotests/129: Do not check @busy
  iotests/129: Use throttle node
  iotests/129: Actually test a commit job
  iotests/129: Limit mirror job's buffer size
  iotests/129: Clean up pylint and mypy complaints

 tests/qemu-iotests/124        |  8 +---
 tests/qemu-iotests/129        | 76 ++++++++++++++++++++++-------------
 tests/qemu-iotests/iotests.py | 11 +++--
 3 files changed, 55 insertions(+), 40 deletions(-)

-- 
2.29.2


Re: [PATCH 0/7] iotests/129: Fix it

Posted by Kevin Wolf 1 week ago
Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
> - pylint and mypy complain.
>   (Running mypy with the options given in 297.)
>   [Patch 4 removes one pylint complaint; patch 7 the rest.]

Should we add it to 297 then to make sure we won't regress?

At some point, I guess we'll want to cover all Python tests, but I
assume it was too much to change in the same series as for the
iotests.py cleanup.

Kevin


Re: [PATCH 0/7] iotests/129: Fix it

Posted by Max Reitz 6 days, 23 hours ago
On 13.01.21 15:38, Kevin Wolf wrote:
> Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
>> - pylint and mypy complain.
>>    (Running mypy with the options given in 297.)
>>    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> 
> Should we add it to 297 then to make sure we won't regress?

Sounds like a good precedent to set indeed.

> At some point, I guess we'll want to cover all Python tests, but I
> assume it was too much to change in the same series as for the
> iotests.py cleanup.
Originally, I hadn’t intended to write patch 7 at all; I just wanted to 
see what pylint/mypy said to my changes in this series, but then they 
made me aware of pre-existing litter.  I thought adding patch 7 would be 
the right thing to do, so I did.

(That’s to say so far I had no intentions of cleaning up all Python 
tests.  It’s just coincidence that I did so for 129.)

Max


Re: [PATCH 0/7] iotests/129: Fix it

Posted by Kevin Wolf 6 days, 23 hours ago
Am 13.01.2021 um 16:26 hat Max Reitz geschrieben:
> On 13.01.21 15:38, Kevin Wolf wrote:
> > Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
> > > - pylint and mypy complain.
> > >    (Running mypy with the options given in 297.)
> > >    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> > 
> > Should we add it to 297 then to make sure we won't regress?
> 
> Sounds like a good precedent to set indeed.
> 
> > At some point, I guess we'll want to cover all Python tests, but I
> > assume it was too much to change in the same series as for the
> > iotests.py cleanup.
> Originally, I hadn’t intended to write patch 7 at all; I just wanted to see
> what pylint/mypy said to my changes in this series, but then they made me
> aware of pre-existing litter.  I thought adding patch 7 would be the right
> thing to do, so I did.
> 
> (That’s to say so far I had no intentions of cleaning up all Python tests.
> It’s just coincidence that I did so for 129.)

Indeed, and I wasn't trying to imply that you should, but just making a
guess why we cover iotests.py and nothing else so far.

Cleaning up 129 is a nice side effect of this series. If more such side
effects accumulate and the rest becomes small enough, I might eventually
just convert the rest. Or I might not, who knows.

Kevin


Re: [PATCH 0/7] iotests/129: Fix it

Posted by Vladimir Sementsov-Ogievskiy 1 week ago
13.01.2021 17:38, Kevin Wolf wrote:
> Am 13.01.2021 um 15:06 hat Max Reitz geschrieben:
>> - pylint and mypy complain.
>>    (Running mypy with the options given in 297.)
>>    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> 
> Should we add it to 297 then to make sure we won't regress?
> 
> At some point, I guess we'll want to cover all Python tests, but I
> assume it was too much to change in the same series as for the
> iotests.py cleanup.
> 

Related:

anyone knows, what's the difference between pylint and pylint-3?

I know I can do pip3 install pylint, which brings pylint binary.. Also there is pylint fedora package which provides pylint binary and python3-pylint fedora package which provides pylint-3 binary, both package names looks so that they are... the same?

Also. Interesting, but pylint don't check PEP8 code style (or at least not everything of it). For example, pycodestyle has a lot of complains about iotests.py (most of them "E302 expected 2 blank lines, found 1").. So, what about adding pycodestyle (or may be better flake8 which is pycodestyle + some additional things) to 127 iotest?

-- 
Best regards,
Vladimir

Re: [PATCH 0/7] iotests/129: Fix it

Posted by Vladimir Sementsov-Ogievskiy 1 week ago
13.01.2021 17:06, Max Reitz wrote:
> Hi,
> 
> There are some problems with iotests 129 (perhaps more than these, but
> these are the ones I know of):
> 
> 1. It checks @busy to see whether a block job is still running; however,
>     block jobs tend to unset @busy all the time (when they yield).
>     [Fixed by patch 3]
> 
> 2. It uses blockdev throttling, which quite some time ago has been moved
>     to the BB level; since then, such throttling will no longer affect
>     block jobs.  We can get throttling to work by using a throttle filter
>     node.
>     [Fixed by patch 4]
> 
> 3. The mirror job has a large buffer size by default.  A simple drain
>     may lead to it making significant process, which is kind of
>     dangerous, because we don’t want the job to complete.

Not quite clear to me. iotest 129 wants to mirror 128M of data. Mirror by
default will have 1M chunk size and maximum of 16 parallel requests. So with
throttling (even if throttling can't correctly handle parallel requests)
we will not exceed 16M of progress.. Why we need limiting buffer size?

>     To get around this, we can simply limit its buffer size.  (And we
>     should make the commit job an actual commit job instead of an active
>     commit (which is just mirror), because the commit interface does not
>     allow setting a buffer size.)
>     [Fixed by patches 5 and 6]
> 
> This series fixes those things, and now 129 seems to reliably pass for
> me.
> 
> 
> Apart from the major issues above, there are also minor flaws:
> 
> - It doesn’t remove the test images.
>    [Fixed by patches 1 and 2]
> 
> - pylint and mypy complain.
>    (Running mypy with the options given in 297.)
>    [Patch 4 removes one pylint complaint; patch 7 the rest.]
> 
> 
> Max Reitz (7):
>    iotests: Move try_remove to iotests.py
>    iotests/129: Remove test images in tearDown()
>    iotests/129: Do not check @busy
>    iotests/129: Use throttle node
>    iotests/129: Actually test a commit job
>    iotests/129: Limit mirror job's buffer size
>    iotests/129: Clean up pylint and mypy complaints
> 
>   tests/qemu-iotests/124        |  8 +---
>   tests/qemu-iotests/129        | 76 ++++++++++++++++++++++-------------
>   tests/qemu-iotests/iotests.py | 11 +++--
>   3 files changed, 55 insertions(+), 40 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/7] iotests/129: Fix it

Posted by Max Reitz 1 week ago
On 13.01.21 15:31, Vladimir Sementsov-Ogievskiy wrote:
> 13.01.2021 17:06, Max Reitz wrote:
>> Hi,
>>
>> There are some problems with iotests 129 (perhaps more than these, but
>> these are the ones I know of):
>>
>> 1. It checks @busy to see whether a block job is still running; however,
>>     block jobs tend to unset @busy all the time (when they yield).
>>     [Fixed by patch 3]
>>
>> 2. It uses blockdev throttling, which quite some time ago has been moved
>>     to the BB level; since then, such throttling will no longer affect
>>     block jobs.  We can get throttling to work by using a throttle filter
>>     node.
>>     [Fixed by patch 4]
>>
>> 3. The mirror job has a large buffer size by default.  A simple drain
>>     may lead to it making significant process, which is kind of
>>     dangerous, because we don’t want the job to complete.
> 
> Not quite clear to me. iotest 129 wants to mirror 128M of data. Mirror by
> default will have 1M chunk size and maximum of 16 parallel requests. So 
> with
> throttling (even if throttling can't correctly handle parallel requests)
> we will not exceed 16M of progress.. Why we need limiting buffer size?

It does exceed 16M of progress; without the limit, I generally see 
something between 16M and 32M.

Now, that still is below 128M, but it’s kind of in the same magnitude. 
I don’t feel comfortable with that, especially given it’s so easy to 
limit it to much less (buf_size=64k makes the job proceed to 128k).

Also, maybe the default is increased in the future.  Increasing the 
chunk size by 4 would mean that it might be possible to reach 128M.

I find not relying on the default better.

Max


Re: [PATCH 0/7] iotests/129: Fix it

Posted by Vladimir Sementsov-Ogievskiy 6 days, 23 hours ago
13.01.2021 18:19, Max Reitz wrote:
> On 13.01.21 15:31, Vladimir Sementsov-Ogievskiy wrote:
>> 13.01.2021 17:06, Max Reitz wrote:
>>> Hi,
>>>
>>> There are some problems with iotests 129 (perhaps more than these, but
>>> these are the ones I know of):
>>>
>>> 1. It checks @busy to see whether a block job is still running; however,
>>>     block jobs tend to unset @busy all the time (when they yield).
>>>     [Fixed by patch 3]
>>>
>>> 2. It uses blockdev throttling, which quite some time ago has been moved
>>>     to the BB level; since then, such throttling will no longer affect
>>>     block jobs.  We can get throttling to work by using a throttle filter
>>>     node.
>>>     [Fixed by patch 4]
>>>
>>> 3. The mirror job has a large buffer size by default.  A simple drain
>>>     may lead to it making significant process, which is kind of
>>>     dangerous, because we don’t want the job to complete.
>>
>> Not quite clear to me. iotest 129 wants to mirror 128M of data. Mirror by
>> default will have 1M chunk size and maximum of 16 parallel requests. So with
>> throttling (even if throttling can't correctly handle parallel requests)
>> we will not exceed 16M of progress.. Why we need limiting buffer size?
> 
> It does exceed 16M of progress; without the limit, I generally see something between 16M and 32M.
> 
> Now, that still is below 128M, but it’s kind of in the same magnitude. I don’t feel comfortable with that, especially given it’s so easy to limit it to much less (buf_size=64k makes the job proceed to 128k).
> 
> Also, maybe the default is increased in the future.  Increasing the chunk size by 4 would mean that it might be possible to reach 128M.
> 
> I find not relying on the default better.

Hmm, OK, agreed


-- 
Best regards,
Vladimir