From c68e1aafeb9e4efbf42ff132d2061e336eda26e5 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Wed, 3 Oct 2018 21:41:27 -0700 Subject: [PATCH] Improve ansible-test environment checking between tests. (#46459) * Add unified diff output to environment validation. This makes it easier to see where the environment changed. * Compare Python interpreters by version to pip shebangs. This helps expose cases where pip executables use a different Python interpreter than is expected. * Query `pip.__version__` instead of using `pip --version`. This is a much faster way to query the pip version. It also more closely matches how we invoke pip within ansible-test. * Remove redundant environment scan between tests. This reuses the environment scan from the end of the previous test as the basis for comparison during the next test. (cherry picked from commit 0dc7f3878794f6deecfcc642da45c951ca376069) --- test/runner/lib/executor.py | 116 +++++++++++++++++++++++++++++++----- test/runner/versions.py | 15 +++++ 2 files changed, 117 insertions(+), 14 deletions(-) create mode 100755 test/runner/versions.py diff --git a/test/runner/lib/executor.py b/test/runner/lib/executor.py index 8fce3752856..6953264eabb 100644 --- a/test/runner/lib/executor.py +++ b/test/runner/lib/executor.py @@ -14,6 +14,8 @@ import functools import pipes import sys import hashlib +import difflib +import filecmp import lib.pytar import lib.thread @@ -741,6 +743,8 @@ def command_integration_filtered(args, targets, all_targets): results = {} + current_environment = None # type: EnvironmentDescription | None + for target in targets_iter: if args.start_at and not found: found = target.name == args.start_at @@ -757,7 +761,8 @@ def command_integration_filtered(args, targets, all_targets): cloud_environment = get_cloud_environment(args, target) - original_environment = EnvironmentDescription(args) + original_environment = current_environment if current_environment else EnvironmentDescription(args) + current_environment = None display.info('>>> Environment Description\n%s' % original_environment, verbosity=3) @@ -816,9 +821,11 @@ def command_integration_filtered(args, targets, all_targets): display.verbosity = args.verbosity = 6 start_time = time.time() - original_environment.validate(target.name, throw=True) + current_environment = EnvironmentDescription(args) end_time = time.time() + EnvironmentDescription.check(original_environment, current_environment, target.name, throw=True) + results[target.name]['validation_seconds'] = int(end_time - start_time) passed.append(target) @@ -1534,27 +1541,84 @@ class EnvironmentDescription(object): self.data = {} return + warnings = [] + versions = [''] versions += SUPPORTED_PYTHON_VERSIONS versions += list(set(v.split('.')[0] for v in SUPPORTED_PYTHON_VERSIONS)) python_paths = dict((v, find_executable('python%s' % v, required=False)) for v in sorted(versions)) - python_versions = dict((v, self.get_version([python_paths[v], '-V'])) for v in sorted(python_paths) if python_paths[v]) - pip_paths = dict((v, find_executable('pip%s' % v, required=False)) for v in sorted(versions)) - pip_versions = dict((v, self.get_version([pip_paths[v], '--version'])) for v in sorted(pip_paths) if pip_paths[v]) + program_versions = dict((v, self.get_version([python_paths[v], 'test/runner/versions.py'], warnings)) for v in sorted(python_paths) if python_paths[v]) pip_interpreters = dict((v, self.get_shebang(pip_paths[v])) for v in sorted(pip_paths) if pip_paths[v]) known_hosts_hash = self.get_hash(os.path.expanduser('~/.ssh/known_hosts')) + for version in sorted(versions): + self.check_python_pip_association(version, python_paths, pip_paths, pip_interpreters, warnings) + + for warning in warnings: + display.warning(warning, unique=True) + self.data = dict( python_paths=python_paths, - python_versions=python_versions, pip_paths=pip_paths, - pip_versions=pip_versions, + program_versions=program_versions, pip_interpreters=pip_interpreters, known_hosts_hash=known_hosts_hash, + warnings=warnings, ) + @staticmethod + def check_python_pip_association(version, python_paths, pip_paths, pip_interpreters, warnings): + """ + :type version: str + :param python_paths: dict[str, str] + :param pip_paths: dict[str, str] + :param pip_interpreters: dict[str, str] + :param warnings: list[str] + """ + python_label = 'Python%s' % (' %s' % version if version else '') + + pip_path = pip_paths.get(version) + python_path = python_paths.get(version) + + if not python_path and not pip_path: + # neither python or pip is present for this version + return + + if not python_path: + warnings.append('A %s interpreter was not found, yet a matching pip was found at "%s".' % (python_label, pip_path)) + return + + if not pip_path: + warnings.append('A %s interpreter was found at "%s", yet a matching pip was not found.' % (python_label, python_path)) + return + + pip_shebang = pip_interpreters.get(version) + + match = re.search(r'#!\s*(?P[^\s]+)', pip_shebang) + + if not match: + warnings.append('A %s pip was found at "%s", but it does not have a valid shebang: %s' % (python_label, pip_path, pip_shebang)) + return + + pip_interpreter = os.path.realpath(match.group('command')) + python_interpreter = os.path.realpath(python_path) + + if pip_interpreter == python_interpreter: + return + + try: + identical = filecmp.cmp(pip_interpreter, python_interpreter) + except OSError: + identical = False + + if identical: + return + + warnings.append('A %s pip was found at "%s", but it uses interpreter "%s" instead of "%s".' % ( + python_label, pip_path, pip_interpreter, python_interpreter)) + def __str__(self): """ :rtype: str @@ -1569,18 +1633,40 @@ class EnvironmentDescription(object): """ current = EnvironmentDescription(self.args) - original_json = str(self) + return self.check(self, current, target_name, throw) + + @staticmethod + def check(original, current, target_name, throw): + """ + :type original: EnvironmentDescription + :type current: EnvironmentDescription + :type target_name: str + :type throw: bool + :rtype: bool + """ + original_json = str(original) current_json = str(current) if original_json == current_json: return True + unified_diff = '\n'.join(difflib.unified_diff( + a=original_json.splitlines(), + b=current_json.splitlines(), + fromfile='original.json', + tofile='current.json', + lineterm='', + )) + message = ('Test target "%s" has changed the test environment!\n' 'If these changes are necessary, they must be reverted before the test finishes.\n' '>>> Original Environment\n' '%s\n' '>>> Current Environment\n' - '%s' % (target_name, original_json, current_json)) + '%s\n' + '>>> Environment Diff\n' + '%s' + % (target_name, original_json, current_json, unified_diff)) if throw: raise ApplicationError(message) @@ -1590,17 +1676,19 @@ class EnvironmentDescription(object): return False @staticmethod - def get_version(command): + def get_version(command, warnings): """ :type command: list[str] - :rtype: str + :type warnings: list[str] + :rtype: list[str] """ try: stdout, stderr = raw_command(command, capture=True, cmd_verbosity=2) - except SubprocessError: + except SubprocessError as ex: + warnings.append(u'%s' % ex) return None # all failures are equal, we don't care why it failed, only that it did - return (stdout or '').strip() + (stderr or '').strip() + return [line.strip() for line in ((stdout or '').strip() + (stderr or '').strip()).splitlines()] @staticmethod def get_shebang(path): @@ -1609,7 +1697,7 @@ class EnvironmentDescription(object): :rtype: str """ with open(path) as script_fd: - return script_fd.readline() + return script_fd.readline().strip() @staticmethod def get_hash(path): diff --git a/test/runner/versions.py b/test/runner/versions.py new file mode 100755 index 00000000000..e2a5db87675 --- /dev/null +++ b/test/runner/versions.py @@ -0,0 +1,15 @@ +#!/usr/bin/env python +"""Show python and pip versions.""" + +import os +import sys + +try: + import pip +except ImportError: + pip = None + +print(sys.version) + +if pip: + print('pip %s from %s' % (pip.__version__, os.path.dirname(pip.__file__)))