From 9ce8eb56beac4dcc138d14948003b0792be5ecd5 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Wed, 10 May 2023 20:03:35 -0700 Subject: [stable-2.13] ansible-test - Fix timeout handling (#80764). (#80767) (cherry picked from commit 4c6aa70662e6f2804686a32bea611a8aa870b363) --- changelogs/fragments/ansible-test-timeout-fix.yml | 5 ++ test/lib/ansible_test/_internal/__init__.py | 4 ++ .../lib/ansible_test/_internal/cli/commands/env.py | 2 +- .../_internal/commands/env/__init__.py | 29 ++++----- test/lib/ansible_test/_internal/test.py | 2 +- test/lib/ansible_test/_internal/timeout.py | 73 ++++++++++++++++------ test/lib/ansible_test/_internal/util.py | 4 ++ 7 files changed, 83 insertions(+), 36 deletions(-) create mode 100644 changelogs/fragments/ansible-test-timeout-fix.yml diff --git a/changelogs/fragments/ansible-test-timeout-fix.yml b/changelogs/fragments/ansible-test-timeout-fix.yml new file mode 100644 index 0000000000..046d5b46d3 --- /dev/null +++ b/changelogs/fragments/ansible-test-timeout-fix.yml @@ -0,0 +1,5 @@ +bugfixes: + - ansible-test - Fix various cases where the test timeout could expire without terminating the tests. +minor_changes: + - ansible-test - Refactored ``env`` command logic and timeout handling. + - ansible-test - Allow float values for the ``--timeout`` option to the ``env`` command. This simplifies testing. diff --git a/test/lib/ansible_test/_internal/__init__.py b/test/lib/ansible_test/_internal/__init__.py index 9b08833e97..d3902ed967 100644 --- a/test/lib/ansible_test/_internal/__init__.py +++ b/test/lib/ansible_test/_internal/__init__.py @@ -18,6 +18,7 @@ from .constants import ( from .util import ( ApplicationError, HostConnectionError, + TimeoutExpiredError, display, ) @@ -103,6 +104,9 @@ def main(cli_args=None): # type: (t.Optional[t.List[str]]) -> None except ApplicationError as ex: display.fatal(u'%s' % ex) sys.exit(1) + except TimeoutExpiredError as ex: + display.fatal('%s' % ex) + sys.exit(1) except KeyboardInterrupt: sys.exit(2) except BrokenPipeError: diff --git a/test/lib/ansible_test/_internal/cli/commands/env.py b/test/lib/ansible_test/_internal/cli/commands/env.py index 53437a1f96..2a957a1b96 100644 --- a/test/lib/ansible_test/_internal/cli/commands/env.py +++ b/test/lib/ansible_test/_internal/cli/commands/env.py @@ -55,7 +55,7 @@ def do_env( env.add_argument( '--timeout', - type=int, + type=float, metavar='MINUTES', help='timeout for future ansible-test commands (0 clears)', ) diff --git a/test/lib/ansible_test/_internal/commands/env/__init__.py b/test/lib/ansible_test/_internal/commands/env/__init__.py index 967f091faa..90078553a9 100644 --- a/test/lib/ansible_test/_internal/commands/env/__init__.py +++ b/test/lib/ansible_test/_internal/commands/env/__init__.py @@ -42,16 +42,20 @@ from ...ci import ( get_ci_provider, ) +from ...timeout import ( + TimeoutDetail, +) + class EnvConfig(CommonConfig): """Configuration for the `env` command.""" def __init__(self, args): # type: (t.Any) -> None super().__init__(args, 'env') - self.show = args.show - self.dump = args.dump - self.timeout = args.timeout - self.list_files = args.list_files + self.show: bool = args.show + self.dump: bool = args.dump + self.timeout: int | float | None = args.timeout + self.list_files: bool = args.list_files if not self.show and not self.dump and self.timeout is None and not self.list_files: # default to --show if no options were given @@ -124,25 +128,18 @@ def set_timeout(args): # type: (EnvConfig) -> None if args.timeout is None: return - if args.timeout: - deadline = (datetime.datetime.utcnow() + datetime.timedelta(minutes=args.timeout)).strftime('%Y-%m-%dT%H:%M:%SZ') + timeout = TimeoutDetail.create(args.timeout) - display.info('Setting a %d minute test timeout which will end at: %s' % (args.timeout, deadline), verbosity=1) + if timeout: + display.info(f'Setting a {timeout.duration} minute test timeout which will end at: {timeout.deadline}', verbosity=1) else: - deadline = None - display.info('Clearing existing test timeout.', verbosity=1) if args.explain: return - if deadline: - data = dict( - duration=args.timeout, - deadline=deadline, - ) - - write_json_file(TIMEOUT_PATH, data) + if timeout: + write_json_file(TIMEOUT_PATH, timeout.to_dict()) elif os.path.exists(TIMEOUT_PATH): os.remove(TIMEOUT_PATH) diff --git a/test/lib/ansible_test/_internal/test.py b/test/lib/ansible_test/_internal/test.py index 2eac09c601..437e2201e5 100644 --- a/test/lib/ansible_test/_internal/test.py +++ b/test/lib/ansible_test/_internal/test.py @@ -127,7 +127,7 @@ class TestResult: class TestTimeout(TestResult): """Test timeout.""" - def __init__(self, timeout_duration): # type: (int) -> None + def __init__(self, timeout_duration: int | float) -> None: super().__init__(command='timeout', test='') self.timeout_duration = timeout_duration diff --git a/test/lib/ansible_test/_internal/timeout.py b/test/lib/ansible_test/_internal/timeout.py index c255f5ce9f..341cb8fd4f 100644 --- a/test/lib/ansible_test/_internal/timeout.py +++ b/test/lib/ansible_test/_internal/timeout.py @@ -1,6 +1,7 @@ """Timeout management for tests.""" from __future__ import annotations +import dataclasses import datetime import functools import os @@ -19,7 +20,7 @@ from .config import ( from .util import ( display, - ApplicationError, + TimeoutExpiredError, ) from .thread import ( @@ -35,15 +36,56 @@ from .test import ( ) -def get_timeout(): # type: () -> t.Optional[t.Dict[str, t.Any]] - """Return details about the currently set timeout, if any, otherwise return None.""" - if not os.path.exists(TIMEOUT_PATH): - return None +@dataclasses.dataclass(frozen=True) +class TimeoutDetail: + """Details required to enforce a timeout on test execution.""" + + _DEADLINE_FORMAT = '%Y-%m-%dT%H:%M:%SZ' # format used to maintain backwards compatibility with previous versions of ansible-test + + deadline: datetime.datetime + duration: int | float # minutes + + @property + def remaining(self) -> datetime.timedelta: + """The amount of time remaining before the timeout occurs. If the timeout has passed, this will be a negative duration.""" + return self.deadline - datetime.datetime.now(tz=datetime.timezone.utc).replace(microsecond=0) + + def to_dict(self) -> dict[str, t.Any]: + """Return timeout details as a dictionary suitable for JSON serialization.""" + return dict( + deadline=self.deadline.strftime(self._DEADLINE_FORMAT), + duration=self.duration, + ) - data = read_json_file(TIMEOUT_PATH) - data['deadline'] = datetime.datetime.strptime(data['deadline'], '%Y-%m-%dT%H:%M:%SZ') + @staticmethod + def from_dict(value: dict[str, t.Any]) -> TimeoutDetail: + """Return a TimeoutDetail instance using the value previously returned by to_dict.""" + return TimeoutDetail( + deadline=datetime.datetime.strptime(value['deadline'], TimeoutDetail._DEADLINE_FORMAT).replace(tzinfo=datetime.timezone.utc), + duration=value['duration'], + ) - return data + @staticmethod + def create(duration: int | float) -> TimeoutDetail | None: + """Return a new TimeoutDetail instance for the specified duration (in minutes), or None if the duration is zero.""" + if not duration: + return None + + if duration == int(duration): + duration = int(duration) + + return TimeoutDetail( + deadline=datetime.datetime.now(datetime.timezone.utc).replace(microsecond=0) + datetime.timedelta(seconds=int(duration * 60)), + duration=duration, + ) + + +def get_timeout() -> TimeoutDetail | None: + """Return details about the currently set timeout, if any, otherwise return None.""" + try: + return TimeoutDetail.from_dict(read_json_file(TIMEOUT_PATH)) + except FileNotFoundError: + return None def configure_timeout(args): # type: (CommonConfig) -> None @@ -59,27 +101,22 @@ def configure_test_timeout(args): # type: (TestConfig) -> None if not timeout: return - timeout_start = datetime.datetime.utcnow() - timeout_duration = timeout['duration'] - timeout_deadline = timeout['deadline'] - timeout_remaining = timeout_deadline - timeout_start + timeout_remaining = timeout.remaining - test_timeout = TestTimeout(timeout_duration) + test_timeout = TestTimeout(timeout.duration) if timeout_remaining <= datetime.timedelta(): test_timeout.write(args) - raise ApplicationError('The %d minute test timeout expired %s ago at %s.' % ( - timeout_duration, timeout_remaining * -1, timeout_deadline)) + raise TimeoutExpiredError(f'The {timeout.duration} minute test timeout expired {timeout_remaining * -1} ago at {timeout.deadline}.') - display.info('The %d minute test timeout expires in %s at %s.' % ( - timeout_duration, timeout_remaining, timeout_deadline), verbosity=1) + display.info(f'The {timeout.duration} minute test timeout expires in {timeout_remaining} at {timeout.deadline}.', verbosity=1) def timeout_handler(_dummy1, _dummy2): """Runs when SIGUSR1 is received.""" test_timeout.write(args) - raise ApplicationError('Tests aborted after exceeding the %d minute time limit.' % timeout_duration) + raise TimeoutExpiredError(f'Tests aborted after exceeding the {timeout.duration} minute time limit.') def timeout_waiter(timeout_seconds): # type: (int) -> None """Background thread which will kill the current process if the timeout elapses.""" diff --git a/test/lib/ansible_test/_internal/util.py b/test/lib/ansible_test/_internal/util.py index 6a427551f9..d82bacf3fb 100644 --- a/test/lib/ansible_test/_internal/util.py +++ b/test/lib/ansible_test/_internal/util.py @@ -902,6 +902,10 @@ class ApplicationWarning(Exception): """General application warning which interrupts normal program flow.""" +class TimeoutExpiredError(SystemExit): + """Error raised when the test timeout has been reached or exceeded.""" + + class SubprocessError(ApplicationError): """Error resulting from failed subprocess execution.""" def __init__( -- cgit v1.2.1