* [PATCH v4 01/10] iotests.py: Assume a couple of variables as given
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 18:44 ` Willian Rampazzo
2021-01-15 17:43 ` [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach Max Reitz
` (8 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
There are a couple of environment variables that we fetch with
os.environ.get() without supplying a default. Clearly they are required
and expected to be set by the ./check script (as evidenced by
execute_setup_common(), which checks for test_dir and
qemu_default_machine to be set, and aborts if they are not).
Using .get() this way has the disadvantage of returning an Optional[str]
type, which mypy will complain about when tests just assume these values
to be str.
Use [] instead, which raises a KeyError for environment variables that
are not set. When this exception is raised, catch it and move the abort
code from execute_setup_common() there.
Drop the 'assert iotests.sock_dir is not None' from iotest 300, because
that sort of thing is precisely what this patch wants to prevent.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/300 | 1 -
tests/qemu-iotests/iotests.py | 26 +++++++++++++-------------
2 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 5b75121b84..b864a21d5e 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -27,7 +27,6 @@ import qemu
BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
-assert iotests.sock_dir is not None
mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dcdcd0387f..52facb8e04 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,12 +75,20 @@ qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
-test_dir = os.environ.get('TEST_DIR')
-sock_dir = os.environ.get('SOCK_DIR')
output_dir = os.environ.get('OUTPUT_DIR', '.')
-cachemode = os.environ.get('CACHEMODE')
-aiomode = os.environ.get('AIOMODE')
-qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
+
+try:
+ test_dir = os.environ['TEST_DIR']
+ sock_dir = os.environ['SOCK_DIR']
+ cachemode = os.environ['CACHEMODE']
+ aiomode = os.environ['AIOMODE']
+ qemu_default_machine = os.environ['QEMU_DEFAULT_MACHINE']
+except KeyError:
+ # We are using these variables as proxies to indicate that we're
+ # not being run via "check". There may be other things set up by
+ # "check" that individual test cases rely on.
+ sys.stderr.write('Please run this test via the "check" script\n')
+ sys.exit(os.EX_USAGE)
socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
@@ -1294,14 +1302,6 @@ def execute_setup_common(supported_fmts: Sequence[str] = (),
"""
# Note: Python 3.6 and pylint do not like 'Collection' so use 'Sequence'.
- # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
- # indicate that we're not being run via "check". There may be
- # other things set up by "check" that individual test cases rely
- # on.
- if test_dir is None or qemu_default_machine is None:
- sys.stderr.write('Please run this test via the "check" script\n')
- sys.exit(os.EX_USAGE)
-
debug = '-d' in sys.argv
if debug:
sys.argv.remove('-d')
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 01/10] iotests.py: Assume a couple of variables as given
2021-01-15 17:43 ` [PATCH v4 01/10] iotests.py: Assume a couple of variables as given Max Reitz
@ 2021-01-15 18:44 ` Willian Rampazzo
0 siblings, 0 replies; 20+ messages in thread
From: Willian Rampazzo @ 2021-01-15 18:44 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>
> There are a couple of environment variables that we fetch with
> os.environ.get() without supplying a default. Clearly they are required
> and expected to be set by the ./check script (as evidenced by
> execute_setup_common(), which checks for test_dir and
> qemu_default_machine to be set, and aborts if they are not).
>
> Using .get() this way has the disadvantage of returning an Optional[str]
> type, which mypy will complain about when tests just assume these values
> to be str.
>
> Use [] instead, which raises a KeyError for environment variables that
> are not set. When this exception is raised, catch it and move the abort
> code from execute_setup_common() there.
>
> Drop the 'assert iotests.sock_dir is not None' from iotest 300, because
> that sort of thing is precisely what this patch wants to prevent.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/300 | 1 -
> tests/qemu-iotests/iotests.py | 26 +++++++++++++-------------
> 2 files changed, 13 insertions(+), 14 deletions(-)
>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
2021-01-15 17:43 ` [PATCH v4 01/10] iotests.py: Assume a couple of variables as given Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 19:27 ` Willian Rampazzo
2021-01-15 17:43 ` [PATCH v4 03/10] iotests: Move try_remove to iotests.py Max Reitz
` (7 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
Instead of checking iotests.py only, check all Python files in the
qemu-iotests/ directory. Of course, most of them do not pass, so there
is an extensive skip list for now. (The only files that do pass are
209, 254, 283, and iotests.py.)
(Alternatively, we could have the opposite, i.e. an explicit list of
files that we do want to check, but I think it is better to check files
by default.)
Unless started in debug mode (./check -d), the output has no information
on which files are tested, so we will not have a problem e.g. with
backports, where some files may be missing when compared to upstream.
Besides the technical rewrite, some more things are changed:
- For the pylint invocation, PYTHONPATH is adjusted. This mirrors
setting MYPYPATH for mypy.
- Also, MYPYPATH is now derived from PYTHONPATH, so that we include
paths set by the environment. Maybe at some point we want to let the
check script add '../../python/' to PYTHONPATH so that iotests.py does
not need to do that.
- Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
comments. TODO is fine, we do not need 297 to complain about such
comments.
- The "Success" line from mypy's output is suppressed, because (A) it
does not add useful information, and (B) it would leak information
about the files having been tested to the reference output, which we
decidedly do not want.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/297 | 110 +++++++++++++++++++++++++++++--------
tests/qemu-iotests/297.out | 5 +-
2 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 5c5420712b..fa9e2cac78 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/usr/bin/env python3
#
# Copyright (C) 2020 Red Hat, Inc.
#
@@ -15,30 +15,96 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
-seq=$(basename $0)
-echo "QA output created by $seq"
+import os
+import re
+import shutil
+import subprocess
+import sys
-status=1 # failure is the default!
+import iotests
-# get standard environment
-. ./common.rc
-if ! type -p "pylint-3" > /dev/null; then
- _notrun "pylint-3 not found"
-fi
-if ! type -p "mypy" > /dev/null; then
- _notrun "mypy not found"
-fi
+# TODO: Empty this list!
+SKIP_FILES = (
+ '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+ '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+ '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+ '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+ '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
+ '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
+ '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
+ '299', '300', '302', '303', '304', '307',
+ 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
-pylint-3 --score=n iotests.py
-MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
- --disallow-any-generics --disallow-incomplete-defs \
- --disallow-untyped-decorators --no-implicit-optional \
- --warn-redundant-casts --warn-unused-ignores \
- --no-implicit-reexport iotests.py
+def is_python_file(filename):
+ if not os.path.isfile(filename):
+ return False
-# success, all done
-echo "*** done"
-rm -f $seq.full
-status=0
+ if filename.endswith('.py'):
+ return True
+
+ with open(filename) as f:
+ try:
+ first_line = f.readline()
+ return re.match('^#!.*python', first_line) is not None
+ except UnicodeDecodeError: # Ignore binary files
+ return False
+
+
+def run_linters():
+ files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
+ if is_python_file(filename)]
+
+ iotests.logger.debug('Files to be checked:')
+ iotests.logger.debug(', '.join(sorted(files)))
+
+ print('=== pylint ===')
+ sys.stdout.flush()
+
+ # Todo notes are fine, but fixme's or xxx's should probably just be
+ # fixed (in tests, at least)
+ env = os.environ.copy()
+ try:
+ env['PYTHONPATH'] += ':../../python/'
+ except KeyError:
+ env['PYTHONPATH'] = '../../python/'
+ subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+ env=env, check=False)
+
+ print('=== mypy ===')
+ sys.stdout.flush()
+
+ # We have to call mypy separately for each file. Otherwise, it
+ # will interpret all given files as belonging together (i.e., they
+ # may not both define the same classes, etc.; most notably, they
+ # must not both define the __main__ module).
+ env['MYPYPATH'] = env['PYTHONPATH']
+ for filename in files:
+ p = subprocess.run(('mypy',
+ '--warn-unused-configs',
+ '--disallow-subclassing-any',
+ '--disallow-any-generics',
+ '--disallow-incomplete-defs',
+ '--disallow-untyped-decorators',
+ '--no-implicit-optional',
+ '--warn-redundant-casts',
+ '--warn-unused-ignores',
+ '--no-implicit-reexport',
+ filename),
+ env=env,
+ check=False,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ universal_newlines=True)
+
+ if p.returncode != 0:
+ print(p.stdout)
+
+
+for linter in ('pylint-3', 'mypy'):
+ if shutil.which(linter) is None:
+ iotests.notrun(f'{linter} not found')
+
+iotests.script_main(run_linters)
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
index 6acc843649..f2e1314d10 100644
--- a/tests/qemu-iotests/297.out
+++ b/tests/qemu-iotests/297.out
@@ -1,3 +1,2 @@
-QA output created by 297
-Success: no issues found in 1 source file
-*** done
+=== pylint ===
+=== mypy ===
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-15 17:43 ` [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach Max Reitz
@ 2021-01-15 19:27 ` Willian Rampazzo
2021-01-18 10:09 ` Max Reitz
0 siblings, 1 reply; 20+ messages in thread
From: Willian Rampazzo @ 2021-01-15 19:27 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>
> Instead of checking iotests.py only, check all Python files in the
> qemu-iotests/ directory. Of course, most of them do not pass, so there
> is an extensive skip list for now. (The only files that do pass are
> 209, 254, 283, and iotests.py.)
>
> (Alternatively, we could have the opposite, i.e. an explicit list of
> files that we do want to check, but I think it is better to check files
> by default.)
>
> Unless started in debug mode (./check -d), the output has no information
> on which files are tested, so we will not have a problem e.g. with
> backports, where some files may be missing when compared to upstream.
>
> Besides the technical rewrite, some more things are changed:
>
> - For the pylint invocation, PYTHONPATH is adjusted. This mirrors
> setting MYPYPATH for mypy.
>
> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
> paths set by the environment. Maybe at some point we want to let the
> check script add '../../python/' to PYTHONPATH so that iotests.py does
> not need to do that.
>
> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
> comments. TODO is fine, we do not need 297 to complain about such
> comments.
>
> - The "Success" line from mypy's output is suppressed, because (A) it
> does not add useful information, and (B) it would leak information
> about the files having been tested to the reference output, which we
> decidedly do not want.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/297 | 110 +++++++++++++++++++++++++++++--------
> tests/qemu-iotests/297.out | 5 +-
> 2 files changed, 90 insertions(+), 25 deletions(-)
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 5c5420712b..fa9e2cac78 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/usr/bin/env python3
> #
> # Copyright (C) 2020 Red Hat, Inc.
> #
> @@ -15,30 +15,96 @@
> # You should have received a copy of the GNU General Public License
> # along with this program. If not, see <http://www.gnu.org/licenses/>.
>
> -seq=$(basename $0)
> -echo "QA output created by $seq"
> +import os
> +import re
> +import shutil
> +import subprocess
> +import sys
>
> -status=1 # failure is the default!
> +import iotests
>
> -# get standard environment
> -. ./common.rc
>
> -if ! type -p "pylint-3" > /dev/null; then
> - _notrun "pylint-3 not found"
> -fi
> -if ! type -p "mypy" > /dev/null; then
> - _notrun "mypy not found"
> -fi
> +# TODO: Empty this list!
> +SKIP_FILES = (
> + '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
> + '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
> + '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
> + '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
> + '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
> + '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
> + '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
> + '299', '300', '302', '303', '304', '307',
> + 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
> +)
>
> -pylint-3 --score=n iotests.py
>
> -MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
> - --disallow-any-generics --disallow-incomplete-defs \
> - --disallow-untyped-decorators --no-implicit-optional \
> - --warn-redundant-casts --warn-unused-ignores \
> - --no-implicit-reexport iotests.py
> +def is_python_file(filename):
> + if not os.path.isfile(filename):
> + return False
>
> -# success, all done
> -echo "*** done"
> -rm -f $seq.full
> -status=0
> + if filename.endswith('.py'):
> + return True
> +
> + with open(filename) as f:
> + try:
> + first_line = f.readline()
> + return re.match('^#!.*python', first_line) is not None
> + except UnicodeDecodeError: # Ignore binary files
> + return False
> +
> +
> +def run_linters():
> + files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
> + if is_python_file(filename)]
> +
> + iotests.logger.debug('Files to be checked:')
> + iotests.logger.debug(', '.join(sorted(files)))
> +
> + print('=== pylint ===')
> + sys.stdout.flush()
> +
> + # Todo notes are fine, but fixme's or xxx's should probably just be
> + # fixed (in tests, at least)
> + env = os.environ.copy()
> + try:
> + env['PYTHONPATH'] += ':../../python/'
Do you have any objection to using os.path.dirname and os.path.join
here? This would make the code more pythonic.
> + except KeyError:
> + env['PYTHONPATH'] = '../../python/'
Same here. You could do it once, before the 'try' and use it inside.
Other than that,
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-15 19:27 ` Willian Rampazzo
@ 2021-01-18 10:09 ` Max Reitz
2021-02-08 20:27 ` Willian Rampazzo
0 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-18 10:09 UTC (permalink / raw)
To: Willian Rampazzo
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On 15.01.21 20:27, Willian Rampazzo wrote:
> On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> Instead of checking iotests.py only, check all Python files in the
>> qemu-iotests/ directory. Of course, most of them do not pass, so there
>> is an extensive skip list for now. (The only files that do pass are
>> 209, 254, 283, and iotests.py.)
>>
>> (Alternatively, we could have the opposite, i.e. an explicit list of
>> files that we do want to check, but I think it is better to check files
>> by default.)
>>
>> Unless started in debug mode (./check -d), the output has no information
>> on which files are tested, so we will not have a problem e.g. with
>> backports, where some files may be missing when compared to upstream.
>>
>> Besides the technical rewrite, some more things are changed:
>>
>> - For the pylint invocation, PYTHONPATH is adjusted. This mirrors
>> setting MYPYPATH for mypy.
>>
>> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
>> paths set by the environment. Maybe at some point we want to let the
>> check script add '../../python/' to PYTHONPATH so that iotests.py does
>> not need to do that.
>>
>> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
>> comments. TODO is fine, we do not need 297 to complain about such
>> comments.
>>
>> - The "Success" line from mypy's output is suppressed, because (A) it
>> does not add useful information, and (B) it would leak information
>> about the files having been tested to the reference output, which we
>> decidedly do not want.
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> tests/qemu-iotests/297 | 110 +++++++++++++++++++++++++++++--------
>> tests/qemu-iotests/297.out | 5 +-
>> 2 files changed, 90 insertions(+), 25 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index 5c5420712b..fa9e2cac78 100755
>> --- a/tests/qemu-iotests/297
>> +++ b/tests/qemu-iotests/297
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/env bash
>> +#!/usr/bin/env python3
>> #
>> # Copyright (C) 2020 Red Hat, Inc.
>> #
>> @@ -15,30 +15,96 @@
>> # You should have received a copy of the GNU General Public License
>> # along with this program. If not, see <http://www.gnu.org/licenses/>.
>>
>> -seq=$(basename $0)
>> -echo "QA output created by $seq"
>> +import os
>> +import re
>> +import shutil
>> +import subprocess
>> +import sys
>>
>> -status=1 # failure is the default!
>> +import iotests
>>
>> -# get standard environment
>> -. ./common.rc
>>
>> -if ! type -p "pylint-3" > /dev/null; then
>> - _notrun "pylint-3 not found"
>> -fi
>> -if ! type -p "mypy" > /dev/null; then
>> - _notrun "mypy not found"
>> -fi
>> +# TODO: Empty this list!
>> +SKIP_FILES = (
>> + '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
>> + '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
>> + '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
>> + '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
>> + '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
>> + '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
>> + '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
>> + '299', '300', '302', '303', '304', '307',
>> + 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
>> +)
>>
>> -pylint-3 --score=n iotests.py
>>
>> -MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
>> - --disallow-any-generics --disallow-incomplete-defs \
>> - --disallow-untyped-decorators --no-implicit-optional \
>> - --warn-redundant-casts --warn-unused-ignores \
>> - --no-implicit-reexport iotests.py
>> +def is_python_file(filename):
>> + if not os.path.isfile(filename):
>> + return False
>>
>> -# success, all done
>> -echo "*** done"
>> -rm -f $seq.full
>> -status=0
>> + if filename.endswith('.py'):
>> + return True
>> +
>> + with open(filename) as f:
>> + try:
>> + first_line = f.readline()
>> + return re.match('^#!.*python', first_line) is not None
>> + except UnicodeDecodeError: # Ignore binary files
>> + return False
>> +
>> +
>> +def run_linters():
>> + files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
>> + if is_python_file(filename)]
>> +
>> + iotests.logger.debug('Files to be checked:')
>> + iotests.logger.debug(', '.join(sorted(files)))
>> +
>> + print('=== pylint ===')
>> + sys.stdout.flush()
>> +
>> + # Todo notes are fine, but fixme's or xxx's should probably just be
>> + # fixed (in tests, at least)
>> + env = os.environ.copy()
>> + try:
>> + env['PYTHONPATH'] += ':../../python/'
>
> Do you have any objection to using os.path.dirname and os.path.join
> here? This would make the code more pythonic.
Intuitively, I felt a bit uneasy about os.path.join here, because it
would make it look like this was platform-independent, when it is not:
The colon as a PATH separator is probably more platform-dependent than
the slashes.
So turns out there is os.pathsep, which yields a colon on e.g. Linux and
a semicolon on e.g. Windows.
I wondered if
env['PYTHONPATH'] = os.pathsep.join(sys.path)
wouldn’t be the most simple solution, but seems like mypy doesn’t like
that very much for the MYPYPATH. Too bad.
Guess the try-except block will have to remain, then.
(Just with os.pathsep instead of a colon, and with
os.path.join(os.path.dirname(__file__), ...).)
Max
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach
2021-01-18 10:09 ` Max Reitz
@ 2021-02-08 20:27 ` Willian Rampazzo
0 siblings, 0 replies; 20+ messages in thread
From: Willian Rampazzo @ 2021-02-08 20:27 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On 1/18/21 7:09 AM, Max Reitz wrote:
> On 15.01.21 20:27, Willian Rampazzo wrote:
>> On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>>>
>>> Instead of checking iotests.py only, check all Python files in the
>>> qemu-iotests/ directory. Of course, most of them do not pass, so there
>>> is an extensive skip list for now. (The only files that do pass are
>>> 209, 254, 283, and iotests.py.)
>>>
>>> (Alternatively, we could have the opposite, i.e. an explicit list of
>>> files that we do want to check, but I think it is better to check files
>>> by default.)
>>>
>>> Unless started in debug mode (./check -d), the output has no information
>>> on which files are tested, so we will not have a problem e.g. with
>>> backports, where some files may be missing when compared to upstream.
>>>
>>> Besides the technical rewrite, some more things are changed:
>>>
>>> - For the pylint invocation, PYTHONPATH is adjusted. This mirrors
>>> setting MYPYPATH for mypy.
>>>
>>> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
>>> paths set by the environment. Maybe at some point we want to let the
>>> check script add '../../python/' to PYTHONPATH so that iotests.py
>>> does
>>> not need to do that.
>>>
>>> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
>>> comments. TODO is fine, we do not need 297 to complain about such
>>> comments.
>>>
>>> - The "Success" line from mypy's output is suppressed, because (A) it
>>> does not add useful information, and (B) it would leak information
>>> about the files having been tested to the reference output, which we
>>> decidedly do not want.
>>>
>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> tests/qemu-iotests/297 | 110 +++++++++++++++++++++++++++++--------
>>> tests/qemu-iotests/297.out | 5 +-
>>> 2 files changed, 90 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>>> index 5c5420712b..fa9e2cac78 100755
>>> --- a/tests/qemu-iotests/297
>>> +++ b/tests/qemu-iotests/297
>>> @@ -1,4 +1,4 @@
>>> -#!/usr/bin/env bash
>>> +#!/usr/bin/env python3
>>> #
>>> # Copyright (C) 2020 Red Hat, Inc.
>>> #
>>> @@ -15,30 +15,96 @@
>>> # You should have received a copy of the GNU General Public License
>>> # along with this program. If not, see
>>> <http://www.gnu.org/licenses/>.
>>>
>>> -seq=$(basename $0)
>>> -echo "QA output created by $seq"
>>> +import os
>>> +import re
>>> +import shutil
>>> +import subprocess
>>> +import sys
>>>
>>> -status=1 # failure is the default!
>>> +import iotests
>>>
>>> -# get standard environment
>>> -. ./common.rc
>>>
>>> -if ! type -p "pylint-3" > /dev/null; then
>>> - _notrun "pylint-3 not found"
>>> -fi
>>> -if ! type -p "mypy" > /dev/null; then
>>> - _notrun "mypy not found"
>>> -fi
>>> +# TODO: Empty this list!
>>> +SKIP_FILES = (
>>> + '030', '040', '041', '044', '045', '055', '056', '057', '065',
>>> '093',
>>> + '096', '118', '124', '129', '132', '136', '139', '147', '148',
>>> '149',
>>> + '151', '152', '155', '163', '165', '169', '194', '196', '199',
>>> '202',
>>> + '203', '205', '206', '207', '208', '210', '211', '212', '213',
>>> '216',
>>> + '218', '219', '222', '224', '228', '234', '235', '236', '237',
>>> '238',
>>> + '240', '242', '245', '246', '248', '255', '256', '257', '258',
>>> '260',
>>> + '262', '264', '266', '274', '277', '280', '281', '295', '296',
>>> '298',
>>> + '299', '300', '302', '303', '304', '307',
>>> + 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
>>> +)
>>>
>>> -pylint-3 --score=n iotests.py
>>>
>>> -MYPYPATH=../../python/ mypy --warn-unused-configs
>>> --disallow-subclassing-any \
>>> - --disallow-any-generics --disallow-incomplete-defs \
>>> - --disallow-untyped-decorators --no-implicit-optional \
>>> - --warn-redundant-casts --warn-unused-ignores \
>>> - --no-implicit-reexport iotests.py
>>> +def is_python_file(filename):
>>> + if not os.path.isfile(filename):
>>> + return False
>>>
>>> -# success, all done
>>> -echo "*** done"
>>> -rm -f $seq.full
>>> -status=0
>>> + if filename.endswith('.py'):
>>> + return True
>>> +
>>> + with open(filename) as f:
>>> + try:
>>> + first_line = f.readline()
>>> + return re.match('^#!.*python', first_line) is not None
>>> + except UnicodeDecodeError: # Ignore binary files
>>> + return False
>>> +
>>> +
>>> +def run_linters():
>>> + files = [filename for filename in (set(os.listdir('.')) -
>>> set(SKIP_FILES))
>>> + if is_python_file(filename)]
>>> +
>>> + iotests.logger.debug('Files to be checked:')
>>> + iotests.logger.debug(', '.join(sorted(files)))
>>> +
>>> + print('=== pylint ===')
>>> + sys.stdout.flush()
>>> +
>>> + # Todo notes are fine, but fixme's or xxx's should probably just be
>>> + # fixed (in tests, at least)
>>> + env = os.environ.copy()
>>> + try:
>>> + env['PYTHONPATH'] += ':../../python/'
>>
>> Do you have any objection to using os.path.dirname and os.path.join
>> here? This would make the code more pythonic.
>
> Intuitively, I felt a bit uneasy about os.path.join here, because it
> would make it look like this was platform-independent, when it is not:
> The colon as a PATH separator is probably more platform-dependent than
> the slashes.
>
> So turns out there is os.pathsep, which yields a colon on e.g. Linux and
> a semicolon on e.g. Windows.
>
> I wondered if
>
> env['PYTHONPATH'] = os.pathsep.join(sys.path)
>
> wouldn’t be the most simple solution, but seems like mypy doesn’t like
> that very much for the MYPYPATH. Too bad.
>
> Guess the try-except block will have to remain, then.
> (Just with os.pathsep instead of a colon, and with
> os.path.join(os.path.dirname(__file__), ...).)
>
> Max
>
Sorry to take so long to answer, I was on vacation. I see v5 was already
merged. I took a look at it and the developed solution looks good,
thanks for all your effort here.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 03/10] iotests: Move try_remove to iotests.py
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
2021-01-15 17:43 ` [PATCH v4 01/10] iotests.py: Assume a couple of variables as given Max Reitz
2021-01-15 17:43 ` [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 18:30 ` Willian Rampazzo
2021-01-15 17:43 ` [PATCH v4 04/10] iotests/129: Remove test images in tearDown() Max Reitz
` (6 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/124 | 8 +-------
tests/qemu-iotests/iotests.py | 11 +++++++----
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3705cbb6b3..e40eeb50b9 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -22,6 +22,7 @@
import os
import iotests
+from iotests import try_remove
def io_write_patterns(img, patterns):
@@ -29,13 +30,6 @@ def io_write_patterns(img, patterns):
iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
-def try_remove(img):
- try:
- os.remove(img)
- except OSError:
- pass
-
-
def transaction_action(action, **kwargs):
return {
'type': action,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 52facb8e04..a69b4cdc4e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -523,12 +523,15 @@ class FilePath:
return False
+def try_remove(img):
+ try:
+ os.remove(img)
+ except OSError:
+ pass
+
def file_path_remover():
for path in reversed(file_path_remover.paths):
- try:
- os.remove(path)
- except OSError:
- pass
+ try_remove(path)
def file_path(*names, base_dir=test_dir):
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 03/10] iotests: Move try_remove to iotests.py
2021-01-15 17:43 ` [PATCH v4 03/10] iotests: Move try_remove to iotests.py Max Reitz
@ 2021-01-15 18:30 ` Willian Rampazzo
0 siblings, 0 replies; 20+ messages in thread
From: Willian Rampazzo @ 2021-01-15 18:30 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/124 | 8 +-------
> tests/qemu-iotests/iotests.py | 11 +++++++----
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 04/10] iotests/129: Remove test images in tearDown()
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (2 preceding siblings ...)
2021-01-15 17:43 ` [PATCH v4 03/10] iotests: Move try_remove to iotests.py Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 17:43 ` [PATCH v4 05/10] iotests/129: Do not check @busy Max Reitz
` (5 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
---
tests/qemu-iotests/129 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 0e13244d85..2fc65ada6a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -47,6 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
**params)
self.vm.shutdown()
+ for img in (self.test_img, self.target_img, self.base_img):
+ iotests.try_remove(img)
def do_test_stop(self, cmd, **args):
"""Test 'stop' while block job is running on a throttled drive.
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 05/10] iotests/129: Do not check @busy
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (3 preceding siblings ...)
2021-01-15 17:43 ` [PATCH v4 04/10] iotests/129: Remove test images in tearDown() Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 18:29 ` Willian Rampazzo
2021-01-15 17:43 ` [PATCH v4 06/10] iotests/129: Use throttle node Max Reitz
` (4 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
@busy is false when the job is paused, which happens all the time
because that is how jobs yield (e.g. for mirror at least since commit
565ac01f8d3).
Back when 129 was added (2015), perhaps there was no better way of
checking whether the job was still actually running. Now we have the
@status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
that information.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/129 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 2fc65ada6a..dd23bb2e5a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
result = self.vm.qmp("stop")
self.assert_qmp(result, 'return', {})
result = self.vm.qmp("query-block-jobs")
- self.assert_qmp(result, 'return[0]/busy', True)
+ self.assert_qmp(result, 'return[0]/status', 'running')
self.assert_qmp(result, 'return[0]/ready', False)
def test_drive_mirror(self):
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 05/10] iotests/129: Do not check @busy
2021-01-15 17:43 ` [PATCH v4 05/10] iotests/129: Do not check @busy Max Reitz
@ 2021-01-15 18:29 ` Willian Rampazzo
0 siblings, 0 replies; 20+ messages in thread
From: Willian Rampazzo @ 2021-01-15 18:29 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>
> @busy is false when the job is paused, which happens all the time
> because that is how jobs yield (e.g. for mirror at least since commit
> 565ac01f8d3).
>
> Back when 129 was added (2015), perhaps there was no better way of
> checking whether the job was still actually running. Now we have the
> @status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
> that information.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/129 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 06/10] iotests/129: Use throttle node
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (4 preceding siblings ...)
2021-01-15 17:43 ` [PATCH v4 05/10] iotests/129: Do not check @busy Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 18:28 ` Willian Rampazzo
2021-01-15 17:43 ` [PATCH v4 07/10] iotests/129: Actually test a commit job Max Reitz
` (3 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
Throttling on the BB has not affected block jobs in a while, so it is
possible that one of the jobs in 129 finishes before the VM is stopped.
We can fix that by running the job from a throttle node.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/129 | 37 +++++++++++++------------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index dd23bb2e5a..d40e2db24e 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
"-b", self.base_img, '-F', iotests.imgfmt)
iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
- self.vm = iotests.VM().add_drive(self.test_img)
+ self.vm = iotests.VM()
+ self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
+
+ source_drive = 'driver=throttle,' \
+ 'throttle-group=tg0,' \
+ f'file.driver={iotests.imgfmt},' \
+ f'file.file.filename={self.test_img}'
+
+ self.vm.add_drive(None, source_drive)
self.vm.launch()
def tearDown(self):
- params = {"device": "drive0",
- "bps": 0,
- "bps_rd": 0,
- "bps_wr": 0,
- "iops": 0,
- "iops_rd": 0,
- "iops_wr": 0,
- }
- result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
- **params)
self.vm.shutdown()
for img in (self.test_img, self.target_img, self.base_img):
iotests.try_remove(img)
@@ -53,33 +51,24 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
def do_test_stop(self, cmd, **args):
"""Test 'stop' while block job is running on a throttled drive.
The 'stop' command shouldn't drain the job"""
- params = {"device": "drive0",
- "bps": 1024,
- "bps_rd": 0,
- "bps_wr": 0,
- "iops": 0,
- "iops_rd": 0,
- "iops_wr": 0,
- }
- result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
- **params)
- self.assert_qmp(result, 'return', {})
result = self.vm.qmp(cmd, **args)
self.assert_qmp(result, 'return', {})
+
result = self.vm.qmp("stop")
self.assert_qmp(result, 'return', {})
result = self.vm.qmp("query-block-jobs")
+
self.assert_qmp(result, 'return[0]/status', 'running')
self.assert_qmp(result, 'return[0]/ready', False)
def test_drive_mirror(self):
self.do_test_stop("drive-mirror", device="drive0",
- target=self.target_img,
+ target=self.target_img, format=iotests.imgfmt,
sync="full")
def test_drive_backup(self):
self.do_test_stop("drive-backup", device="drive0",
- target=self.target_img,
+ target=self.target_img, format=iotests.imgfmt,
sync="full")
def test_block_commit(self):
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/10] iotests/129: Use throttle node
2021-01-15 17:43 ` [PATCH v4 06/10] iotests/129: Use throttle node Max Reitz
@ 2021-01-15 18:28 ` Willian Rampazzo
0 siblings, 0 replies; 20+ messages in thread
From: Willian Rampazzo @ 2021-01-15 18:28 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>
> Throttling on the BB has not affected block jobs in a while, so it is
> possible that one of the jobs in 129 finishes before the VM is stopped.
> We can fix that by running the job from a throttle node.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/129 | 37 +++++++++++++------------------------
> 1 file changed, 13 insertions(+), 24 deletions(-)
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 07/10] iotests/129: Actually test a commit job
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (5 preceding siblings ...)
2021-01-15 17:43 ` [PATCH v4 06/10] iotests/129: Use throttle node Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 18:26 ` Willian Rampazzo
2021-01-15 17:43 ` [PATCH v4 08/10] iotests/129: Limit mirror job's buffer size Max Reitz
` (2 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
Before this patch, test_block_commit() performs an active commit, which
under the hood is a mirror job. If we want to test various different
block jobs, we should perhaps run an actual commit job instead.
Doing so requires adding an overlay above the source node before the
commit is done (and then specifying the source node as the top node for
the commit job).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/129 | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index d40e2db24e..104be6dded 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -26,6 +26,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
test_img = os.path.join(iotests.test_dir, 'test.img')
target_img = os.path.join(iotests.test_dir, 'target.img')
base_img = os.path.join(iotests.test_dir, 'base.img')
+ overlay_img = os.path.join(iotests.test_dir, 'overlay.img')
def setUp(self):
iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
@@ -36,6 +37,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
source_drive = 'driver=throttle,' \
+ 'node-name=source,' \
'throttle-group=tg0,' \
f'file.driver={iotests.imgfmt},' \
f'file.file.filename={self.test_img}'
@@ -45,7 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
def tearDown(self):
self.vm.shutdown()
- for img in (self.test_img, self.target_img, self.base_img):
+ for img in (self.test_img, self.target_img, self.base_img,
+ self.overlay_img):
iotests.try_remove(img)
def do_test_stop(self, cmd, **args):
@@ -72,7 +75,27 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
sync="full")
def test_block_commit(self):
- self.do_test_stop("block-commit", device="drive0")
+ # Add overlay above the source node so that we actually use a
+ # commit job instead of a mirror job
+
+ iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
+ '1G')
+
+ result = self.vm.qmp('blockdev-add', **{
+ 'node-name': 'overlay',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': self.overlay_img
+ }
+ })
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm.qmp('blockdev-snapshot',
+ node='source', overlay='overlay')
+ self.assert_qmp(result, 'return', {})
+
+ self.do_test_stop('block-commit', device='drive0', top_node='source')
if __name__ == '__main__':
iotests.main(supported_fmts=["qcow2"],
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 07/10] iotests/129: Actually test a commit job
2021-01-15 17:43 ` [PATCH v4 07/10] iotests/129: Actually test a commit job Max Reitz
@ 2021-01-15 18:26 ` Willian Rampazzo
0 siblings, 0 replies; 20+ messages in thread
From: Willian Rampazzo @ 2021-01-15 18:26 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>
> Before this patch, test_block_commit() performs an active commit, which
> under the hood is a mirror job. If we want to test various different
> block jobs, we should perhaps run an actual commit job instead.
>
> Doing so requires adding an overlay above the source node before the
> commit is done (and then specifying the source node as the top node for
> the commit job).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/129 | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 08/10] iotests/129: Limit mirror job's buffer size
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (6 preceding siblings ...)
2021-01-15 17:43 ` [PATCH v4 07/10] iotests/129: Actually test a commit job Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 17:43 ` [PATCH v4 09/10] iotests/129: Clean up pylint and mypy complaints Max Reitz
2021-01-15 17:43 ` [PATCH v4 10/10] iotests/300: " Max Reitz
9 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
Issuing 'stop' on the VM drains all nodes. If the mirror job has many
large requests in flight, this may lead to significant I/O that looks a
bit like 'stop' would make the job try to complete (which is what 129
should verify not to happen).
We can limit the I/O in flight by limiting the buffer size, so mirror
will make very little progress during the 'stop' drain.
(We do not need to do anything about commit, which has a buffer size of
512 kB by default; or backup, which goes cluster by cluster. Once we
have asynchronous requests for backup, that will change, but then we can
fine-tune the backup job to only perform a single request on a very
small chunk, too.)
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
---
tests/qemu-iotests/129 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 104be6dded..80a5db521b 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -67,7 +67,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
def test_drive_mirror(self):
self.do_test_stop("drive-mirror", device="drive0",
target=self.target_img, format=iotests.imgfmt,
- sync="full")
+ sync="full", buf_size=65536)
def test_drive_backup(self):
self.do_test_stop("drive-backup", device="drive0",
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 09/10] iotests/129: Clean up pylint and mypy complaints
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (7 preceding siblings ...)
2021-01-15 17:43 ` [PATCH v4 08/10] iotests/129: Limit mirror job's buffer size Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 17:43 ` [PATCH v4 10/10] iotests/300: " Max Reitz
9 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
And consequentially drop it from 297's skip list.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/129 | 4 ++--
tests/qemu-iotests/297 | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 80a5db521b..9a56217bf8 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -20,7 +20,6 @@
import os
import iotests
-import time
class TestStopWithBlockJob(iotests.QMPTestCase):
test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
"-b", self.base_img, '-F', iotests.imgfmt)
- iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', self.test_img)
+ iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
+ self.test_img)
self.vm = iotests.VM()
self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index fa9e2cac78..d2d9292839 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -27,7 +27,7 @@ import iotests
# TODO: Empty this list!
SKIP_FILES = (
'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
- '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+ '096', '118', '124', '132', '136', '139', '147', '148', '149',
'151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 10/10] iotests/300: Clean up pylint and mypy complaints
2021-01-15 17:43 [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach Max Reitz
` (8 preceding siblings ...)
2021-01-15 17:43 ` [PATCH v4 09/10] iotests/129: Clean up pylint and mypy complaints Max Reitz
@ 2021-01-15 17:43 ` Max Reitz
2021-01-15 18:24 ` Willian Rampazzo
9 siblings, 1 reply; 20+ messages in thread
From: Max Reitz @ 2021-01-15 17:43 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz,
Willian Rampazzo
And consequentially drop it from 297's skip list.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/297 | 2 +-
tests/qemu-iotests/300 | 18 +++++++++++++++---
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index d2d9292839..ce7f85cfe0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -33,7 +33,7 @@ SKIP_FILES = (
'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
- '299', '300', '302', '303', '304', '307',
+ '299', '302', '303', '304', '307',
'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
)
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index b864a21d5e..4115f90578 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -22,7 +22,11 @@ import os
import random
import re
from typing import Dict, List, Optional, Union
+
import iotests
+
+# Import qemu after iotests.py has amended sys.path
+# pylint: disable=wrong-import-order
import qemu
BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
@@ -110,10 +114,14 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
If @msg is None, check that there has not been any error.
"""
self.vm_b.shutdown()
+
+ log = self.vm_b.get_log()
+ assert log is not None # Loaded after shutdown
+
if msg is None:
- self.assertNotIn('qemu-system-', self.vm_b.get_log())
+ self.assertNotIn('qemu-system-', log)
else:
- self.assertIn(msg, self.vm_b.get_log())
+ self.assertIn(msg, log)
@staticmethod
def mapping(node_name: str, node_alias: str,
@@ -445,9 +453,13 @@ class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
# Check for the error in the source's log
self.vm_a.shutdown()
+
+ log = self.vm_a.get_log()
+ assert log is not None # Loaded after shutdown
+
self.assertIn(f"Cannot migrate bitmap '{name}' on node "
f"'{self.src_node_name}': Name is longer than 255 bytes",
- self.vm_a.get_log())
+ log)
# Expect abnormal shutdown of the destination VM because of
# the failed migration
--
2.29.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 10/10] iotests/300: Clean up pylint and mypy complaints
2021-01-15 17:43 ` [PATCH v4 10/10] iotests/300: " Max Reitz
@ 2021-01-15 18:24 ` Willian Rampazzo
0 siblings, 0 replies; 20+ messages in thread
From: Willian Rampazzo @ 2021-01-15 18:24 UTC (permalink / raw)
To: Max Reitz
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>
> And consequentially drop it from 297's skip list.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/297 | 2 +-
> tests/qemu-iotests/300 | 18 +++++++++++++++---
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
Reviewed-by: Willian Rampazzo <willianr@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread