From 0dc7f3878794f6deecfcc642da45c951ca376069 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. --- 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__)))