From 95a556e7c7ddeb8e9e370dd074f48ab77915aa82 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:04 -0400 Subject: [PATCH 01/22] iotests/297: Move pylint config into pylintrc Move --score=n and --notes=XXX,FIXME into pylintrc. This pulls configuration out of code, which I think is probably a good thing in general. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-2-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 4 +--- tests/qemu-iotests/pylintrc | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index 91ec34d952..bc3a0ceb2a 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -65,10 +65,8 @@ def run_linters(): 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() - subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files), + subprocess.run(('pylint-3', *files), env=env, check=False) print('=== mypy ===') diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc index 8cb4e1d6a6..32ab77b8bb 100644 --- a/tests/qemu-iotests/pylintrc +++ b/tests/qemu-iotests/pylintrc @@ -31,6 +31,22 @@ disable=invalid-name, too-many-statements, consider-using-f-string, + +[REPORTS] + +# Activate the evaluation score. +score=no + + +[MISCELLANEOUS] + +# List of note tags to take in consideration, separated by a comma. +# TODO notes are fine, but FIXMEs or XXXs should probably just be +# fixed (in tests, at least). +notes=FIXME, + XXX, + + [FORMAT] # Maximum number of characters on a single line. From 8f7960fa3120806e0933795c6f34d8b5b09c622b Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:05 -0400 Subject: [PATCH 02/22] iotests/297: Split mypy configuration out into mypy.ini More separation of code and configuration. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-3-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 14 +------------- tests/qemu-iotests/mypy.ini | 12 ++++++++++++ 2 files changed, 13 insertions(+), 13 deletions(-) create mode 100644 tests/qemu-iotests/mypy.ini diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index bc3a0ceb2a..b8101e6024 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -73,19 +73,7 @@ def run_linters(): sys.stdout.flush() env['MYPYPATH'] = env['PYTHONPATH'] - 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', - '--namespace-packages', - '--scripts-are-modules', - *files), + p = subprocess.run(('mypy', *files), env=env, check=False, stdout=subprocess.PIPE, diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini new file mode 100644 index 0000000000..4c0339f558 --- /dev/null +++ b/tests/qemu-iotests/mypy.ini @@ -0,0 +1,12 @@ +[mypy] +disallow_any_generics = True +disallow_incomplete_defs = True +disallow_subclassing_any = True +disallow_untyped_decorators = True +implicit_reexport = False +namespace_packages = True +no_implicit_optional = True +scripts_are_modules = True +warn_redundant_casts = True +warn_unused_configs = True +warn_unused_ignores = True From 3c1d5012e8d46cd2e4084e9f995d4444b6c09076 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:06 -0400 Subject: [PATCH 03/22] iotests/297: Add get_files() function Split out file discovery into its own method to begin separating out configuration/setup and test execution. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-4-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index b8101e6024..15b54594c1 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -21,6 +21,7 @@ import re import shutil import subprocess import sys +from typing import List import iotests @@ -54,10 +55,14 @@ def is_python_file(filename): return False -def run_linters(): +def get_test_files() -> List[str]: named_tests = [f'tests/{entry}' for entry in os.listdir('tests')] check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES) - files = [filename for filename in check_tests if is_python_file(filename)] + return list(filter(is_python_file, check_tests)) + + +def run_linters(): + files = get_test_files() iotests.logger.debug('Files to be checked:') iotests.logger.debug(', '.join(sorted(files))) From 447aebda3f461de159280c4896a537395aac8e90 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:07 -0400 Subject: [PATCH 04/22] iotests/297: Create main() function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of running "run_linters" directly, create a main() function that will be responsible for environment setup, leaving run_linters() responsible only for execution of the linters. (That environment setup will be moved over in forthcoming commits.) Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-5-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index 15b54594c1..163ebc8ebf 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -89,8 +89,12 @@ def run_linters(): print(p.stdout) -for linter in ('pylint-3', 'mypy'): - if shutil.which(linter) is None: - iotests.notrun(f'{linter} not found') +def main() -> None: + for linter in ('pylint-3', 'mypy'): + if shutil.which(linter) is None: + iotests.notrun(f'{linter} not found') -iotests.script_main(run_linters) + run_linters() + + +iotests.script_main(main) From f1be6219c550aeba1f91ac33b8236296e5b34851 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:08 -0400 Subject: [PATCH 05/22] iotests/297: Don't rely on distro-specific linter binaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m mypy" to access these scripts instead. This style of invocation will prefer the "correct" tool when run in a virtual environment. Note that we still check for "pylint-3" before the test begins -- this check is now "overly strict", but shouldn't cause anything that was already running correctly to start failing. This is addressed by a commit later in this series; 'iotests/297: update tool availability checks'. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-6-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index 163ebc8ebf..c1bddb9ce0 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -71,14 +71,14 @@ def run_linters(): sys.stdout.flush() env = os.environ.copy() - subprocess.run(('pylint-3', *files), + subprocess.run(('python3', '-m', 'pylint', *files), env=env, check=False) print('=== mypy ===') sys.stdout.flush() env['MYPYPATH'] = env['PYTHONPATH'] - p = subprocess.run(('mypy', *files), + p = subprocess.run(('python3', '-m', 'mypy', *files), env=env, check=False, stdout=subprocess.PIPE, From 2d804f55b4f6b4f500ad99567c60631ac47fd860 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:09 -0400 Subject: [PATCH 06/22] iotests/297: Split run_linters apart into run_pylint and run_mypy Move environment setup into main(), and split the actual linter execution into run_pylint and run_mypy, respectively. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-7-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index c1bddb9ce0..189bcaf5f9 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -21,7 +21,7 @@ import re import shutil import subprocess import sys -from typing import List +from typing import List, Mapping, Optional import iotests @@ -61,23 +61,19 @@ def get_test_files() -> List[str]: return list(filter(is_python_file, check_tests)) -def run_linters(): - files = get_test_files() +def run_pylint( + files: List[str], + env: Optional[Mapping[str, str]] = None, +) -> None: - iotests.logger.debug('Files to be checked:') - iotests.logger.debug(', '.join(sorted(files))) - - print('=== pylint ===') - sys.stdout.flush() - - env = os.environ.copy() subprocess.run(('python3', '-m', 'pylint', *files), env=env, check=False) - print('=== mypy ===') - sys.stdout.flush() - env['MYPYPATH'] = env['PYTHONPATH'] +def run_mypy( + files: List[str], + env: Optional[Mapping[str, str]] = None, +) -> None: p = subprocess.run(('python3', '-m', 'mypy', *files), env=env, check=False, @@ -94,7 +90,21 @@ def main() -> None: if shutil.which(linter) is None: iotests.notrun(f'{linter} not found') - run_linters() + files = get_test_files() + + iotests.logger.debug('Files to be checked:') + iotests.logger.debug(', '.join(sorted(files))) + + env = os.environ.copy() + env['MYPYPATH'] = env['PYTHONPATH'] + + print('=== pylint ===') + sys.stdout.flush() + run_pylint(files, env=env) + + print('=== mypy ===') + sys.stdout.flush() + run_mypy(files, env=env) iotests.script_main(main) From a4bde736295bd0951005d7292b99086825f74f8a Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:10 -0400 Subject: [PATCH 07/22] iotests/297: refactor run_[mypy|pylint] as generic execution shim There's virtually nothing special here anymore; we can combine these into a single, rather generic function. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-8-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index 189bcaf5f9..d21673a292 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -61,27 +61,29 @@ def get_test_files() -> List[str]: return list(filter(is_python_file, check_tests)) -def run_pylint( - files: List[str], - env: Optional[Mapping[str, str]] = None, +def run_linter( + tool: str, + args: List[str], + env: Optional[Mapping[str, str]] = None, + suppress_output: bool = False, ) -> None: + """ + Run a python-based linting tool. - subprocess.run(('python3', '-m', 'pylint', *files), - env=env, check=False) + If suppress_output is True, capture stdout/stderr of the child + process and only print that information back to stdout if the child + process's return code was non-zero. + """ + p = subprocess.run( + ('python3', '-m', tool, *args), + env=env, + check=False, + stdout=subprocess.PIPE if suppress_output else None, + stderr=subprocess.STDOUT if suppress_output else None, + universal_newlines=True, + ) - -def run_mypy( - files: List[str], - env: Optional[Mapping[str, str]] = None, -) -> None: - p = subprocess.run(('python3', '-m', 'mypy', *files), - env=env, - check=False, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - universal_newlines=True) - - if p.returncode != 0: + if suppress_output and p.returncode != 0: print(p.stdout) @@ -100,11 +102,11 @@ def main() -> None: print('=== pylint ===') sys.stdout.flush() - run_pylint(files, env=env) + run_linter('pylint', files, env=env) print('=== mypy ===') sys.stdout.flush() - run_mypy(files, env=env) + run_linter('mypy', files, env=env, suppress_output=True) iotests.script_main(main) From 752f425d832d1d4e00f39e80835dd249fb44e698 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:11 -0400 Subject: [PATCH 08/22] iotests/297: Change run_linter() to raise an exception on failure Instead of using a process return code as the python function return value (or just not returning anything at all), allow run_linter() to raise an exception instead. The responsibility for printing output on error shifts from the function itself to the caller, who will know best how to present/format that information. (Also, "suppress_output" is now a lot more accurate of a parameter name.) Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-9-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index d21673a292..76d6a23f53 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -70,22 +70,18 @@ def run_linter( """ Run a python-based linting tool. - If suppress_output is True, capture stdout/stderr of the child - process and only print that information back to stdout if the child - process's return code was non-zero. + :param suppress_output: If True, suppress all stdout/stderr output. + :raise CalledProcessError: If the linter process exits with failure. """ - p = subprocess.run( + subprocess.run( ('python3', '-m', tool, *args), env=env, - check=False, + check=True, stdout=subprocess.PIPE if suppress_output else None, stderr=subprocess.STDOUT if suppress_output else None, universal_newlines=True, ) - if suppress_output and p.returncode != 0: - print(p.stdout) - def main() -> None: for linter in ('pylint-3', 'mypy'): @@ -102,11 +98,19 @@ def main() -> None: print('=== pylint ===') sys.stdout.flush() - run_linter('pylint', files, env=env) + try: + run_linter('pylint', files, env=env) + except subprocess.CalledProcessError: + # pylint failure will be caught by diffing the IO. + pass print('=== mypy ===') sys.stdout.flush() - run_linter('mypy', files, env=env, suppress_output=True) + try: + run_linter('mypy', files, env=env, suppress_output=True) + except subprocess.CalledProcessError as exc: + if exc.output: + print(exc.output) iotests.script_main(main) From 7a90bcc269bb6f727cfd69b50128ecf4ab92e9af Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:12 -0400 Subject: [PATCH 09/22] iotests/297: update tool availability checks As mentioned in 'iotests/297: Don't rely on distro-specific linter binaries', these checks are overly strict. Update them to be in-line with how we actually invoke the linters themselves. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-10-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index 76d6a23f53..b2ad8d1cbe 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -18,7 +18,6 @@ import os import re -import shutil import subprocess import sys from typing import List, Mapping, Optional @@ -84,9 +83,11 @@ def run_linter( def main() -> None: - for linter in ('pylint-3', 'mypy'): - if shutil.which(linter) is None: - iotests.notrun(f'{linter} not found') + for linter in ('pylint', 'mypy'): + try: + run_linter(linter, ['--version'], suppress_output=True) + except subprocess.CalledProcessError: + iotests.notrun(f"'{linter}' not found") files = get_test_files() From 85cfec53d0b43182a14f8b5c3aa685955a852f1c Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:13 -0400 Subject: [PATCH 10/22] iotests/297: split test into sub-cases Take iotest 297's main() test function and split it into two sub-cases that can be skipped individually. We can also drop custom environment setup from the pylint test as it isn't needed. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-11-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 63 ++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index b2ad8d1cbe..b7d9d6077b 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -82,36 +82,51 @@ def run_linter( ) -def main() -> None: - for linter in ('pylint', 'mypy'): - try: - run_linter(linter, ['--version'], suppress_output=True) - except subprocess.CalledProcessError: - iotests.notrun(f"'{linter}' not found") +def check_linter(linter: str) -> bool: + try: + run_linter(linter, ['--version'], suppress_output=True) + except subprocess.CalledProcessError: + iotests.case_notrun(f"'{linter}' not found") + return False + return True + +def test_pylint(files: List[str]) -> None: + print('=== pylint ===') + sys.stdout.flush() + + if not check_linter('pylint'): + return + + run_linter('pylint', files) + + +def test_mypy(files: List[str]) -> None: + print('=== mypy ===') + sys.stdout.flush() + + if not check_linter('mypy'): + return + + env = os.environ.copy() + env['MYPYPATH'] = env['PYTHONPATH'] + + run_linter('mypy', files, env=env, suppress_output=True) + + +def main() -> None: files = get_test_files() iotests.logger.debug('Files to be checked:') iotests.logger.debug(', '.join(sorted(files))) - env = os.environ.copy() - env['MYPYPATH'] = env['PYTHONPATH'] - - print('=== pylint ===') - sys.stdout.flush() - try: - run_linter('pylint', files, env=env) - except subprocess.CalledProcessError: - # pylint failure will be caught by diffing the IO. - pass - - print('=== mypy ===') - sys.stdout.flush() - try: - run_linter('mypy', files, env=env, suppress_output=True) - except subprocess.CalledProcessError as exc: - if exc.output: - print(exc.output) + for test in (test_pylint, test_mypy): + try: + test(files) + except subprocess.CalledProcessError as exc: + # Linter failure will be caught by diffing the IO. + if exc.output: + print(exc.output) iotests.script_main(main) From c293ba55c52ce07e03919e61e4a17d2cc2c944d4 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:14 -0400 Subject: [PATCH 11/22] iotests: split linters.py out from 297 Now, 297 is just the iotests-specific incantations and linters.py is as minimal as I can think to make it. The only remaining element in here that ought to be configuration and not code is the list of skip files, but they're still numerous enough that repeating them for mypy and pylint configurations both would be ... a hassle. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-12-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/297 | 72 +++++---------------------------- tests/qemu-iotests/linters.py | 76 +++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 61 deletions(-) create mode 100644 tests/qemu-iotests/linters.py diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297 index b7d9d6077b..ee78a62735 100755 --- a/tests/qemu-iotests/297 +++ b/tests/qemu-iotests/297 @@ -17,74 +17,24 @@ # along with this program. If not, see . import os -import re import subprocess import sys -from typing import List, Mapping, Optional +from typing import List import iotests +import linters -# TODO: Empty this list! -SKIP_FILES = ( - '030', '040', '041', '044', '045', '055', '056', '057', '065', '093', - '096', '118', '124', '132', '136', '139', '147', '148', '149', - '151', '152', '155', '163', '165', '194', '196', '202', - '203', '205', '206', '207', '208', '210', '211', '212', '213', '216', - '218', '219', '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', '302', '303', '304', '307', - 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py' -) - - -def is_python_file(filename): - if not os.path.isfile(filename): - return False - - if filename.endswith('.py'): - return True - - with open(filename, encoding='utf-8') as f: - try: - first_line = f.readline() - return re.match('^#!.*python', first_line) is not None - except UnicodeDecodeError: # Ignore binary files - return False - - -def get_test_files() -> List[str]: - named_tests = [f'tests/{entry}' for entry in os.listdir('tests')] - check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES) - return list(filter(is_python_file, check_tests)) - - -def run_linter( - tool: str, - args: List[str], - env: Optional[Mapping[str, str]] = None, - suppress_output: bool = False, -) -> None: - """ - Run a python-based linting tool. - - :param suppress_output: If True, suppress all stdout/stderr output. - :raise CalledProcessError: If the linter process exits with failure. - """ - subprocess.run( - ('python3', '-m', tool, *args), - env=env, - check=True, - stdout=subprocess.PIPE if suppress_output else None, - stderr=subprocess.STDOUT if suppress_output else None, - universal_newlines=True, - ) +# Looking for something? +# +# List of files to exclude from linting: linters.py +# mypy configuration: mypy.ini +# pylint configuration: pylintrc def check_linter(linter: str) -> bool: try: - run_linter(linter, ['--version'], suppress_output=True) + linters.run_linter(linter, ['--version'], suppress_output=True) except subprocess.CalledProcessError: iotests.case_notrun(f"'{linter}' not found") return False @@ -98,7 +48,7 @@ def test_pylint(files: List[str]) -> None: if not check_linter('pylint'): return - run_linter('pylint', files) + linters.run_linter('pylint', files) def test_mypy(files: List[str]) -> None: @@ -111,11 +61,11 @@ def test_mypy(files: List[str]) -> None: env = os.environ.copy() env['MYPYPATH'] = env['PYTHONPATH'] - run_linter('mypy', files, env=env, suppress_output=True) + linters.run_linter('mypy', files, env=env, suppress_output=True) def main() -> None: - files = get_test_files() + files = linters.get_test_files() iotests.logger.debug('Files to be checked:') iotests.logger.debug(', '.join(sorted(files))) diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py new file mode 100644 index 0000000000..c515c7afe3 --- /dev/null +++ b/tests/qemu-iotests/linters.py @@ -0,0 +1,76 @@ +# Copyright (C) 2020 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +import os +import re +import subprocess +from typing import List, Mapping, Optional + + +# TODO: Empty this list! +SKIP_FILES = ( + '030', '040', '041', '044', '045', '055', '056', '057', '065', '093', + '096', '118', '124', '132', '136', '139', '147', '148', '149', + '151', '152', '155', '163', '165', '194', '196', '202', + '203', '205', '206', '207', '208', '210', '211', '212', '213', '216', + '218', '219', '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', '302', '303', '304', '307', + 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py' +) + + +def is_python_file(filename): + if not os.path.isfile(filename): + return False + + if filename.endswith('.py'): + return True + + with open(filename, encoding='utf-8') as f: + try: + first_line = f.readline() + return re.match('^#!.*python', first_line) is not None + except UnicodeDecodeError: # Ignore binary files + return False + + +def get_test_files() -> List[str]: + named_tests = [f'tests/{entry}' for entry in os.listdir('tests')] + check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES) + return list(filter(is_python_file, check_tests)) + + +def run_linter( + tool: str, + args: List[str], + env: Optional[Mapping[str, str]] = None, + suppress_output: bool = False, +) -> None: + """ + Run a python-based linting tool. + + :param suppress_output: If True, suppress all stdout/stderr output. + :raise CalledProcessError: If the linter process exits with failure. + """ + subprocess.run( + ('python3', '-m', tool, *args), + env=env, + check=True, + stdout=subprocess.PIPE if suppress_output else None, + stderr=subprocess.STDOUT if suppress_output else None, + universal_newlines=True, + ) From a4294435309d8429e8ee89908b5d7cebf6a648df Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:15 -0400 Subject: [PATCH 12/22] iotests/linters: Add entry point for linting via Python CI We need at least a tiny little shim here to join test file discovery with test invocation. This logic could conceivably be hosted somewhere in python/, but I felt it was strictly the least-rude thing to keep the test logic here in iotests/, even if this small function isn't itself an iotest. Note that we don't actually even need the executable bit here, we'll be relying on the ability to run this module as a script using Python CLI arguments. No chance it gets misunderstood as an actual iotest that way. (It's named, not in tests/, doesn't have the execute bit, and doesn't have an execution shebang.) Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-13-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/linters.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py index c515c7afe3..46c28fdcda 100644 --- a/tests/qemu-iotests/linters.py +++ b/tests/qemu-iotests/linters.py @@ -16,6 +16,7 @@ import os import re import subprocess +import sys from typing import List, Mapping, Optional @@ -74,3 +75,29 @@ def run_linter( stderr=subprocess.STDOUT if suppress_output else None, universal_newlines=True, ) + + +def main() -> None: + """ + Used by the Python CI system as an entry point to run these linters. + """ + def show_usage() -> None: + print(f"Usage: {sys.argv[0]} < --mypy | --pylint >", file=sys.stderr) + sys.exit(1) + + if len(sys.argv) != 2: + show_usage() + + files = get_test_files() + + if sys.argv[1] == '--pylint': + run_linter('pylint', files) + elif sys.argv[1] == '--mypy': + run_linter('mypy', files) + else: + print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr) + show_usage() + + +if __name__ == '__main__': + main() From 558dbe9935445af6ab20e18b3664ba6c43eb2311 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:16 -0400 Subject: [PATCH 13/22] iotests/linters: Add workaround for mypy bug #9852 This one is insidious: if you write an import as "from {namespace} import {subpackage}" as mirror-top-perms (now) does, mypy will fail on every-other invocation *if* the package being imported is a typed, installed, namespace-scoped package. Upsettingly, that's exactly what 'qemu.[aqmp|qmp|machine]' et al are in the context of Python CI tests. Now, I could just edit mirror-top-perms to avoid this invocation, but since I tripped on a landmine, I might as well head it off at the pass and make sure nobody else trips on that same landmine. It seems to have something to do with the order in which files are checked as well, meaning the random order in which set(os.listdir()) produces the list of files to test will cause problems intermittently and not just strictly "every other run". This will be fixed in mypy >= 0.920, which is not released yet. The workaround for now is to disable incremental checking, which avoids the issue. Note: This workaround is not applied when running iotest 297 directly, because the bug does not surface there! Given the nature of CI jobs not starting with any stale cache to begin with, this really only has a half-second impact on manual runs of the Python test suite when executed directly by a developer on their local machine. The workaround may be removed when the Python package requirements can stipulate mypy 0.920 or higher, which can happen as soon as it is released. (Barring any unforseen compatibility issues that 0.920 may bring with it.) See also: https://github.com/python/mypy/issues/11010 https://github.com/python/mypy/issues/9852 Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-14-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/linters.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py index 46c28fdcda..65c4c4e827 100644 --- a/tests/qemu-iotests/linters.py +++ b/tests/qemu-iotests/linters.py @@ -93,7 +93,9 @@ def show_usage() -> None: if sys.argv[1] == '--pylint': run_linter('pylint', files) elif sys.argv[1] == '--mypy': - run_linter('mypy', files) + # mypy bug #9852; disable incremental checking as a workaround. + args = ['--no-incremental'] + files + run_linter('mypy', args) else: print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr) show_usage() From 461044ceb4fe58eb919dedfeb62f619f79d4d552 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:17 -0400 Subject: [PATCH 14/22] python: Add iotest linters to test suite Run mypy and pylint on the iotests files directly from the Python CI test infrastructure. This ensures that any accidental breakages to the qemu.[qmp|aqmp|machine|utils] packages will be caught by that test suite. It also ensures that these linters are run with well-known versions and test against a wide variety of python versions, which helps to find accidental cross-version python compatibility issues. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-15-jsnow@redhat.com Signed-off-by: John Snow --- python/tests/iotests-mypy.sh | 4 ++++ python/tests/iotests-pylint.sh | 4 ++++ 2 files changed, 8 insertions(+) create mode 100755 python/tests/iotests-mypy.sh create mode 100755 python/tests/iotests-pylint.sh diff --git a/python/tests/iotests-mypy.sh b/python/tests/iotests-mypy.sh new file mode 100755 index 0000000000..ee76470819 --- /dev/null +++ b/python/tests/iotests-mypy.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --mypy diff --git a/python/tests/iotests-pylint.sh b/python/tests/iotests-pylint.sh new file mode 100755 index 0000000000..4cae03424b --- /dev/null +++ b/python/tests/iotests-pylint.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --pylint From b9420e4f4b899c96221fa5a1f7f025214e756c50 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:05 -0400 Subject: [PATCH 15/22] python/machine: remove has_quit argument If we spy on the QMP commands instead, we don't need callers to remember to pass it. Seems like a fair trade-off. The one slightly weird bit is overloading this instance variable for wait(), where we use it to mean "don't issue the qmp 'quit' command". This means that wait() will "fail" if the QEMU process does not terminate of its own accord. In most cases, we probably did already actually issue quit -- some iotests do this -- but in some others, we may be waiting for QEMU to terminate for some other reason, such as a test wherein we tell the guest (directly) to shut down. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20211026175612.4127598-2-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/machine/machine.py | 34 +++++++++++++++++++--------------- tests/qemu-iotests/040 | 7 +------ tests/qemu-iotests/218 | 2 +- tests/qemu-iotests/255 | 2 +- 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 056d340e35..0bd40bc2f7 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -170,6 +170,7 @@ def __init__(self, self._console_socket: Optional[socket.socket] = None self._remove_files: List[str] = [] self._user_killed = False + self._quit_issued = False def __enter__(self: _T) -> _T: return self @@ -368,6 +369,7 @@ def _post_shutdown(self) -> None: command = '' LOG.warning(msg, -int(exitcode), command) + self._quit_issued = False self._user_killed = False self._launched = False @@ -443,15 +445,13 @@ def _hard_shutdown(self) -> None: self._subp.kill() self._subp.wait(timeout=60) - def _soft_shutdown(self, timeout: Optional[int], - has_quit: bool = False) -> None: + def _soft_shutdown(self, timeout: Optional[int]) -> None: """ Perform early cleanup, attempt to gracefully shut down the VM, and wait for it to terminate. :param timeout: Timeout in seconds for graceful shutdown. A value of None is an infinite wait. - :param has_quit: When True, don't attempt to issue 'quit' QMP command :raise ConnectionReset: On QMP communication errors :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for @@ -460,21 +460,19 @@ def _soft_shutdown(self, timeout: Optional[int], self._early_cleanup() if self._qmp_connection: - if not has_quit: + if not self._quit_issued: # Might raise ConnectionReset - self._qmp.cmd('quit') + self.qmp('quit') # May raise subprocess.TimeoutExpired self._subp.wait(timeout=timeout) - def _do_shutdown(self, timeout: Optional[int], - has_quit: bool = False) -> None: + def _do_shutdown(self, timeout: Optional[int]) -> None: """ Attempt to shutdown the VM gracefully; fallback to a hard shutdown. :param timeout: Timeout in seconds for graceful shutdown. A value of None is an infinite wait. - :param has_quit: When True, don't attempt to issue 'quit' QMP command :raise AbnormalShutdown: When the VM could not be shut down gracefully. The inner exception will likely be ConnectionReset or @@ -482,13 +480,13 @@ def _do_shutdown(self, timeout: Optional[int], may result in its own exceptions, likely subprocess.TimeoutExpired. """ try: - self._soft_shutdown(timeout, has_quit) + self._soft_shutdown(timeout) except Exception as exc: self._hard_shutdown() raise AbnormalShutdown("Could not perform graceful shutdown") \ from exc - def shutdown(self, has_quit: bool = False, + def shutdown(self, hard: bool = False, timeout: Optional[int] = 30) -> None: """ @@ -498,7 +496,6 @@ def shutdown(self, has_quit: bool = False, If the VM has not yet been launched, or shutdown(), wait(), or kill() have already been called, this method does nothing. - :param has_quit: When true, do not attempt to issue 'quit' QMP command. :param hard: When true, do not attempt graceful shutdown, and suppress the SIGKILL warning log message. :param timeout: Optional timeout in seconds for graceful shutdown. @@ -512,7 +509,7 @@ def shutdown(self, has_quit: bool = False, self._user_killed = True self._hard_shutdown() else: - self._do_shutdown(timeout, has_quit) + self._do_shutdown(timeout) finally: self._post_shutdown() @@ -529,7 +526,8 @@ def wait(self, timeout: Optional[int] = 30) -> None: :param timeout: Optional timeout in seconds. Default 30 seconds. A value of `None` is an infinite wait. """ - self.shutdown(has_quit=True, timeout=timeout) + self._quit_issued = True + self.shutdown(timeout=timeout) def set_qmp_monitor(self, enabled: bool = True) -> None: """ @@ -574,7 +572,10 @@ def qmp(self, cmd: str, conv_keys = True qmp_args = self._qmp_args(conv_keys, args) - return self._qmp.cmd(cmd, args=qmp_args) + ret = self._qmp.cmd(cmd, args=qmp_args) + if cmd == 'quit' and 'error' not in ret and 'return' in ret: + self._quit_issued = True + return ret def command(self, cmd: str, conv_keys: bool = True, @@ -585,7 +586,10 @@ def command(self, cmd: str, On failure raise an exception. """ qmp_args = self._qmp_args(conv_keys, args) - return self._qmp.command(cmd, **qmp_args) + ret = self._qmp.command(cmd, **qmp_args) + if cmd == 'quit': + self._quit_issued = True + return ret def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]: """ diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index f3677de9df..6af5ab9e76 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -92,10 +92,9 @@ class TestSingleDrive(ImageCommitTestCase): self.vm.add_device('virtio-scsi') self.vm.add_device("scsi-hd,id=scsi0,drive=drive0") self.vm.launch() - self.has_quit = False def tearDown(self): - self.vm.shutdown(has_quit=self.has_quit) + self.vm.shutdown() os.remove(test_img) os.remove(mid_img) os.remove(backing_img) @@ -127,8 +126,6 @@ class TestSingleDrive(ImageCommitTestCase): result = self.vm.qmp('quit') self.assert_qmp(result, 'return', {}) - self.has_quit = True - # Same as above, but this time we add the filter after starting the job @iotests.skip_if_unsupported(['throttle']) def test_commit_plus_filter_and_quit(self): @@ -147,8 +144,6 @@ class TestSingleDrive(ImageCommitTestCase): result = self.vm.qmp('quit') self.assert_qmp(result, 'return', {}) - self.has_quit = True - def test_device_not_found(self): result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % mid_img) self.assert_qmp(result, 'error/class', 'DeviceNotFound') diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218 index 325d8244fb..4922b4d3b6 100755 --- a/tests/qemu-iotests/218 +++ b/tests/qemu-iotests/218 @@ -187,4 +187,4 @@ with iotests.VM() as vm, \ log(vm.qmp('quit')) with iotests.Timeout(5, 'Timeout waiting for VM to quit'): - vm.shutdown(has_quit=True) + vm.shutdown() diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255 index c43aa9c67a..3d6d0e80cb 100755 --- a/tests/qemu-iotests/255 +++ b/tests/qemu-iotests/255 @@ -123,4 +123,4 @@ with iotests.FilePath('src.qcow2') as src_path, \ vm.qmp_log('block-job-cancel', device='job0') vm.qmp_log('quit') - vm.shutdown(has_quit=True) + vm.shutdown() From 49a608b8c22db72d47e7676ff9c3b87d1d408736 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:06 -0400 Subject: [PATCH 16/22] python/machine: Handle QMP errors on close more meticulously To use the AQMP backend, Machine just needs to be a little more diligent about what happens when closing a QMP connection. The operation is no longer a freebie in the async world; it may return errors encountered in the async bottom half on incoming message receipt, etc. (AQMP's disconnect, ultimately, serves as the quiescence point where all async contexts are gathered together, and any final errors reported at that point.) Because async QMP continues to check for messages asynchronously, it's almost certainly likely that the loop will have exited due to EOF after issuing the last 'quit' command. That error will ultimately be bubbled up when attempting to close the QMP connection. The manager class here then is free to discard it -- if it was expected. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20211026175612.4127598-3-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/machine/machine.py | 48 +++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 0bd40bc2f7..a0cf69786b 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -342,9 +342,15 @@ def _post_shutdown(self) -> None: # Comprehensive reset for the failed launch case: self._early_cleanup() - if self._qmp_connection: - self._qmp.close() - self._qmp_connection = None + try: + self._close_qmp_connection() + except Exception as err: # pylint: disable=broad-except + LOG.warning( + "Exception closing QMP connection: %s", + str(err) if str(err) else type(err).__name__ + ) + finally: + assert self._qmp_connection is None self._close_qemu_log_file() @@ -420,6 +426,31 @@ def _launch(self) -> None: close_fds=False) self._post_launch() + def _close_qmp_connection(self) -> None: + """ + Close the underlying QMP connection, if any. + + Dutifully report errors that occurred while closing, but assume + that any error encountered indicates an abnormal termination + process and not a failure to close. + """ + if self._qmp_connection is None: + return + + try: + self._qmp.close() + except EOFError: + # EOF can occur as an Exception here when using the Async + # QMP backend. It indicates that the server closed the + # stream. If we successfully issued 'quit' at any point, + # then this was expected. If the remote went away without + # our permission, it's worth reporting that as an abnormal + # shutdown case. + if not (self._user_killed or self._quit_issued): + raise + finally: + self._qmp_connection = None + def _early_cleanup(self) -> None: """ Perform any cleanup that needs to happen before the VM exits. @@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None: self._early_cleanup() if self._qmp_connection: - if not self._quit_issued: - # Might raise ConnectionReset - self.qmp('quit') + try: + if not self._quit_issued: + # May raise ExecInterruptedError or StateError if the + # connection dies or has *already* died. + self.qmp('quit') + finally: + # Regardless, we want to quiesce the connection. + self._close_qmp_connection() # May raise subprocess.TimeoutExpired self._subp.wait(timeout=timeout) From 0f71c9a93693de54ca80a47c3996d64cd65f72b7 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:07 -0400 Subject: [PATCH 17/22] python/aqmp: Remove scary message The scary message interferes with the iotests output. Coincidentally, if iotests works by removing this, then it's good evidence that we don't really need to scare people away from using it. Signed-off-by: John Snow Acked-by: Hanna Reitz Reviewed-by: Kevin Wolf Reviewed-by: Hanna Reitz Message-id: 20211026175612.4127598-4-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py index d1b0e4dc3d..880d5b6fa7 100644 --- a/python/qemu/aqmp/__init__.py +++ b/python/qemu/aqmp/__init__.py @@ -22,7 +22,6 @@ # the COPYING file in the top-level directory. import logging -import warnings from .error import AQMPError from .events import EventListener @@ -31,17 +30,6 @@ from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient -_WMSG = """ - -The Asynchronous QMP library is currently in development and its API -should be considered highly fluid and subject to change. It should -not be used by any other scripts checked into the QEMU tree. - -Proceed with caution! -""" - -warnings.warn(_WMSG, FutureWarning) - # Suppress logging unless an application engages it. logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler()) From 3bd559467d979165065f8406d2016aaa31f2cee2 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:08 -0400 Subject: [PATCH 18/22] iotests: Accommodate async QMP Exception classes (But continue to support the old ones for now, too.) There are very few cases of any user of QEMUMachine or a subclass thereof relying on a QMP Exception type. If you'd like to check for yourself, you want to grep for all of the derivatives of QMPError, excluding 'AQMPError' and its derivatives. That'd be these: - QMPError - QMPConnectError - QMPCapabilitiesError - QMPTimeoutError - QMPProtocolError - QMPResponseError - QMPBadPortError Signed-off-by: John Snow Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20211026175612.4127598-5-jsnow@redhat.com Signed-off-by: John Snow --- scripts/simplebench/bench_block_job.py | 3 ++- tests/qemu-iotests/tests/mirror-top-perms | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py index 4f03c12169..a403c35b08 100755 --- a/scripts/simplebench/bench_block_job.py +++ b/scripts/simplebench/bench_block_job.py @@ -28,6 +28,7 @@ sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python')) from qemu.machine import QEMUMachine from qemu.qmp import QMPConnectError +from qemu.aqmp import ConnectError def bench_block_job(cmd, cmd_args, qemu_args): @@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args): vm.launch() except OSError as e: return {'error': 'popen failed: ' + str(e)} - except (QMPConnectError, socket.timeout): + except (QMPConnectError, ConnectError, socket.timeout): return {'error': 'qemu failed: ' + str(vm.get_log())} try: diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms index 3d475aa3a5..a2d5c269d7 100755 --- a/tests/qemu-iotests/tests/mirror-top-perms +++ b/tests/qemu-iotests/tests/mirror-top-perms @@ -21,8 +21,9 @@ import os -from qemu import qmp +from qemu.aqmp import ConnectError from qemu.machine import machine +from qemu.qmp import QMPConnectError import iotests from iotests import qemu_img @@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase): self.vm_b.launch() print('ERROR: VM B launched successfully, this should not have ' 'happened') - except qmp.QMPConnectError: + except (QMPConnectError, ConnectError): assert 'Is another process using the image' in self.vm_b.get_log() result = self.vm.qmp('block-job-cancel', From 206dc475484c198d2bbfaf2aacb7bc370a734277 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:09 -0400 Subject: [PATCH 19/22] iotests: Conditionally silence certain AQMP errors AQMP likes to be very chatty about errors it encounters. In general, this is good because it allows us to get good diagnostic information for otherwise complex async failures. For example, during a failed QMP connection attempt, we might see: +ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError +ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError This might be nice in iotests output, because failure scenarios involving the new QMP library will be spelled out plainly in the output diffs. For tests that are intentionally causing this scenario though, filtering that log output could be a hassle. For now, add a context manager that simply lets us toggle this output off during a critical region. (Additionally, a forthcoming patch allows the use of either legacy or async QMP to be toggled with an environment variable. In this circumstance, we can't amend the iotest output to just always expect the error message, either. Just suppress it for now. More rigorous log filtering can be investigated later if/when it is deemed safe to permanently replace the legacy QMP library.) Signed-off-by: John Snow Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20211026175612.4127598-6-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/iotests.py | 20 +++++++++++++++++++- tests/qemu-iotests/tests/mirror-top-perms | 12 ++++++++---- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index e5fff6ddcf..e2f9d873ad 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -30,7 +30,7 @@ import subprocess import sys import time -from typing import (Any, Callable, Dict, Iterable, +from typing import (Any, Callable, Dict, Iterable, Iterator, List, Optional, Sequence, TextIO, Tuple, Type, TypeVar) import unittest @@ -114,6 +114,24 @@ sample_img_dir = os.environ['SAMPLE_IMG_DIR'] +@contextmanager +def change_log_level( + logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]: + """ + Utility function for temporarily changing the log level of a logger. + + This can be used to silence errors that are expected or uninteresting. + """ + _logger = logging.getLogger(logger_name) + current_level = _logger.level + _logger.setLevel(level) + + try: + yield + finally: + _logger.setLevel(current_level) + + def unarchive_sample_image(sample, fname): sample_fname = os.path.join(sample_img_dir, sample + '.bz2') with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out: diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms index a2d5c269d7..0a51a613f3 100755 --- a/tests/qemu-iotests/tests/mirror-top-perms +++ b/tests/qemu-iotests/tests/mirror-top-perms @@ -26,7 +26,7 @@ from qemu.machine import machine from qemu.qmp import QMPConnectError import iotests -from iotests import qemu_img +from iotests import change_log_level, qemu_img image_size = 1 * 1024 * 1024 @@ -100,9 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase): self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}') self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on') try: - self.vm_b.launch() - print('ERROR: VM B launched successfully, this should not have ' - 'happened') + # Silence AQMP errors temporarily. + # TODO: Remove this and just allow the errors to be logged when + # AQMP fully replaces QMP. + with change_log_level('qemu.aqmp'): + self.vm_b.launch() + print('ERROR: VM B launched successfully, ' + 'this should not have happened') except (QMPConnectError, ConnectError): assert 'Is another process using the image' in self.vm_b.get_log() From 8f05aee5333a78e2244008021f72e7b21fdc147f Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:10 -0400 Subject: [PATCH 20/22] iotests/300: avoid abnormal shutdown race condition Wait for the destination VM to close itself instead of racing to shut it down first, which produces different error log messages from AQMP depending on precisely when we tried to shut it down. (For example: We may try to issue 'quit' immediately prior to the target VM closing its QMP socket, which will cause an ECONNRESET error to be logged. Waiting for the VM to exit itself avoids the race on shutdown behavior.) Reported-by: Hanna Reitz Signed-off-by: John Snow Reviewed-by: Kevin Wolf Reviewed-by: Hanna Reitz Message-id: 20211026175612.4127598-7-jsnow@redhat.com Signed-off-by: John Snow --- tests/qemu-iotests/300 | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300 index 10f9f2a8da..dbd28384ec 100755 --- a/tests/qemu-iotests/300 +++ b/tests/qemu-iotests/300 @@ -24,8 +24,6 @@ import random import re from typing import Dict, List, Optional -from qemu.machine import machine - import iotests @@ -461,12 +459,11 @@ class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration): f"'{self.src_node_name}': Name is longer than 255 bytes", log) - # Expect abnormal shutdown of the destination VM because of - # the failed migration - try: - self.vm_b.shutdown() - except machine.AbnormalShutdown: - pass + # Destination VM will terminate w/ error of its own accord + # due to the failed migration. + self.vm_b.wait() + rc = self.vm_b.exitcode() + assert rc is not None and rc > 0 def test_aliased_bitmap_name_too_long(self) -> None: # Longer than the maximum for bitmap names From f122be6093f5fc61fe5784e342781fab07f5defc Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:11 -0400 Subject: [PATCH 21/22] python/aqmp: Create sync QMP wrapper for iotests This is a wrapper around the async QMPClient that mimics the old, synchronous QEMUMonitorProtocol class. It is designed to be interchangeable with the old implementation. It does not, however, attempt to mimic Exception compatibility. Signed-off-by: John Snow Acked-by: Hanna Reitz Reviewed-by: Kevin Wolf Reviewed-by: Hanna Reitz Message-id: 20211026175612.4127598-8-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/legacy.py | 138 +++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 python/qemu/aqmp/legacy.py diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py new file mode 100644 index 0000000000..9e7b9fb80b --- /dev/null +++ b/python/qemu/aqmp/legacy.py @@ -0,0 +1,138 @@ +""" +Sync QMP Wrapper + +This class pretends to be qemu.qmp.QEMUMonitorProtocol. +""" + +import asyncio +from typing import ( + Awaitable, + List, + Optional, + TypeVar, + Union, +) + +import qemu.qmp +from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT + +from .qmp_client import QMPClient + + +# pylint: disable=missing-docstring + + +class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol): + def __init__(self, address: SocketAddrT, + server: bool = False, + nickname: Optional[str] = None): + + # pylint: disable=super-init-not-called + self._aqmp = QMPClient(nickname) + self._aloop = asyncio.get_event_loop() + self._address = address + self._timeout: Optional[float] = None + + _T = TypeVar('_T') + + def _sync( + self, future: Awaitable[_T], timeout: Optional[float] = None + ) -> _T: + return self._aloop.run_until_complete( + asyncio.wait_for(future, timeout=timeout) + ) + + def _get_greeting(self) -> Optional[QMPMessage]: + if self._aqmp.greeting is not None: + # pylint: disable=protected-access + return self._aqmp.greeting._asdict() + return None + + # __enter__ and __exit__ need no changes + # parse_address needs no changes + + def connect(self, negotiate: bool = True) -> Optional[QMPMessage]: + self._aqmp.await_greeting = negotiate + self._aqmp.negotiate = negotiate + + self._sync( + self._aqmp.connect(self._address) + ) + return self._get_greeting() + + def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage: + self._aqmp.await_greeting = True + self._aqmp.negotiate = True + + self._sync( + self._aqmp.accept(self._address), + timeout + ) + + ret = self._get_greeting() + assert ret is not None + return ret + + def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage: + return dict( + self._sync( + # pylint: disable=protected-access + + # _raw() isn't a public API, because turning off + # automatic ID assignment is discouraged. For + # compatibility with iotests *only*, do it anyway. + self._aqmp._raw(qmp_cmd, assign_id=False), + self._timeout + ) + ) + + # Default impl of cmd() delegates to cmd_obj + + def command(self, cmd: str, **kwds: object) -> QMPReturnValue: + return self._sync( + self._aqmp.execute(cmd, kwds), + self._timeout + ) + + def pull_event(self, + wait: Union[bool, float] = False) -> Optional[QMPMessage]: + if not wait: + # wait is False/0: "do not wait, do not except." + if self._aqmp.events.empty(): + return None + + # If wait is 'True', wait forever. If wait is False/0, the events + # queue must not be empty; but it still needs some real amount + # of time to complete. + timeout = None + if wait and isinstance(wait, float): + timeout = wait + + return dict( + self._sync( + self._aqmp.events.get(), + timeout + ) + ) + + def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]: + events = [dict(x) for x in self._aqmp.events.clear()] + if events: + return events + + event = self.pull_event(wait) + return [event] if event is not None else [] + + def clear_events(self) -> None: + self._aqmp.events.clear() + + def close(self) -> None: + self._sync( + self._aqmp.disconnect() + ) + + def settimeout(self, timeout: Optional[float]) -> None: + self._timeout = timeout + + def send_fd_scm(self, fd: int) -> None: + self._aqmp.send_fd_scm(fd) From 76cd358671e6b8e7c435ec65b1c44200254514a9 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:12 -0400 Subject: [PATCH 22/22] python, iotests: replace qmp with aqmp Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync wrapper from qemu.aqmp instead. Add an escape hatch in the form of the environment variable QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using the old implementation, proving that both implementations work concurrently. Signed-off-by: John Snow Reviewed-by: Kevin Wolf Reviewed-by: Hanna Reitz Message-id: 20211026175612.4127598-9-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/machine/machine.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index a0cf69786b..a487c39745 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -41,7 +41,6 @@ ) from qemu.qmp import ( # pylint: disable=import-error - QEMUMonitorProtocol, QMPMessage, QMPReturnValue, SocketAddrT, @@ -50,6 +49,12 @@ from . import console_socket +if os.environ.get('QEMU_PYTHON_LEGACY_QMP'): + from qemu.qmp import QEMUMonitorProtocol +else: + from qemu.aqmp.legacy import QEMUMonitorProtocol + + LOG = logging.getLogger(__name__)