mirror of https://github.com/ansible/ansible.git
Clean up TE error handling, wrap sigalrm handler (#85232)
* Clean up TE error handling, wrap sigalrm handler
* Preserve error detail on AnsibleAction and Connection exceptions.
* Remove multiple layers of unreachable or redundant error handling.
* Wrap manual alarm signal/timeout handling into a context manager, add tests.
Co-authored-by: Matt Clay <matt@mystile.com>
* update error message check in test
* update test timeout message assertions
---------
Co-authored-by: Matt Clay <matt@mystile.com>
(cherry picked from commit cbcefc53a3)
pull/85255/head
Matt Davis
6 months ago
committed by
Matt Davis
parent
311ef75245
commit
85283e7238
@ -0,0 +1,7 @@
|
||||
bugfixes:
|
||||
- task timeout - Specifying a negative task timeout now results in an error.
|
||||
- error handling - Error details and tracebacks from connection and built-in action exceptions are preserved.
|
||||
Previously, much of the detail was lost or mixed into the error message.
|
||||
|
||||
minor_changes:
|
||||
- task timeout - Specifying a timeout greater than 100,000,000 now results in an error.
|
||||
@ -0,0 +1,66 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import contextlib
|
||||
import signal
|
||||
import types
|
||||
import typing as _t
|
||||
|
||||
from ansible.module_utils import datatag
|
||||
|
||||
|
||||
class AnsibleTimeoutError(BaseException):
|
||||
"""A general purpose timeout."""
|
||||
|
||||
_MAX_TIMEOUT = 100_000_000
|
||||
"""
|
||||
The maximum supported timeout value.
|
||||
This value comes from BSD's alarm limit, which is due to that function using setitimer.
|
||||
"""
|
||||
|
||||
def __init__(self, timeout: int) -> None:
|
||||
self.timeout = timeout
|
||||
|
||||
super().__init__(f"Timed out after {timeout} second(s).")
|
||||
|
||||
@classmethod
|
||||
@contextlib.contextmanager
|
||||
def alarm_timeout(cls, timeout: int | None) -> _t.Iterator[None]:
|
||||
"""
|
||||
Context for running code under an optional timeout.
|
||||
Raises an instance of this class if the timeout occurs.
|
||||
|
||||
New usages of this timeout mechanism are discouraged.
|
||||
"""
|
||||
if timeout is not None:
|
||||
if not isinstance(timeout, int):
|
||||
raise TypeError(f"Timeout requires 'int' argument, not {datatag.native_type_name(timeout)!r}.")
|
||||
|
||||
if timeout < 0 or timeout > cls._MAX_TIMEOUT:
|
||||
# On BSD based systems, alarm is implemented using setitimer.
|
||||
# If out-of-bounds values are passed to alarm, they will return -1, which would be interpreted as an existing timer being set.
|
||||
# To avoid that, bounds checking is performed in advance.
|
||||
raise ValueError(f'Timeout {timeout} is invalid, it must be between 0 and {cls._MAX_TIMEOUT}.')
|
||||
|
||||
if not timeout:
|
||||
yield # execute the context manager's body
|
||||
return # no timeout to deal with, exit immediately
|
||||
|
||||
def on_alarm(_signal: int, _frame: types.FrameType) -> None:
|
||||
raise cls(timeout)
|
||||
|
||||
if signal.signal(signal.SIGALRM, on_alarm):
|
||||
raise RuntimeError("An existing alarm handler was present.")
|
||||
|
||||
try:
|
||||
try:
|
||||
if signal.alarm(timeout):
|
||||
raise RuntimeError("An existing alarm was set.")
|
||||
|
||||
yield # execute the context manager's body
|
||||
finally:
|
||||
# Disable the alarm.
|
||||
# If the alarm fires inside this finally block, the alarm is still disabled.
|
||||
# This guarantees the cleanup code in the outer finally block runs without risk of encountering the `TaskTimeoutError` from the alarm.
|
||||
signal.alarm(0)
|
||||
finally:
|
||||
signal.signal(signal.SIGALRM, signal.SIG_DFL)
|
||||
@ -0,0 +1,28 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from collections import abc as _c
|
||||
|
||||
from ansible._internal._errors._alarm_timeout import AnsibleTimeoutError
|
||||
from ansible._internal._errors._error_utils import ContributesToTaskResult
|
||||
from ansible.module_utils.datatag import deprecate_value
|
||||
|
||||
|
||||
class TaskTimeoutError(AnsibleTimeoutError, ContributesToTaskResult):
|
||||
"""
|
||||
A task-specific timeout.
|
||||
|
||||
This exception provides a result dictionary via the ContributesToTaskResult mixin.
|
||||
"""
|
||||
|
||||
@property
|
||||
def result_contribution(self) -> _c.Mapping[str, object]:
|
||||
help_text = "Configure `DISPLAY_TRACEBACK` to see a traceback on timeout errors."
|
||||
|
||||
frame = deprecate_value(
|
||||
value=help_text,
|
||||
msg="The `timedout.frame` task result key is deprecated.",
|
||||
help_text=help_text,
|
||||
version="2.23",
|
||||
)
|
||||
|
||||
return dict(timedout=dict(frame=frame, period=self.timeout))
|
||||
@ -0,0 +1,2 @@
|
||||
shippable/posix/group4
|
||||
context/controller
|
||||
@ -0,0 +1,61 @@
|
||||
- name: run a task which times out
|
||||
command: sleep 10
|
||||
timeout: 1
|
||||
register: result
|
||||
ignore_errors: yes
|
||||
|
||||
- name: verify the task timed out
|
||||
assert:
|
||||
that:
|
||||
- result is failed
|
||||
- result is timedout
|
||||
- result.timedout.period == 1
|
||||
- result.msg is contains "Timed out after 1 second"
|
||||
|
||||
- name: run a task with a negative timeout
|
||||
command: sleep 3
|
||||
timeout: -1
|
||||
register: result
|
||||
ignore_errors: yes
|
||||
|
||||
- name: verify the task failed
|
||||
assert:
|
||||
that:
|
||||
- result is failed
|
||||
- result is not timedout
|
||||
- result.msg is contains "Timeout -1 is invalid"
|
||||
|
||||
- name: run a task with a timeout that is too large
|
||||
command: sleep 3
|
||||
timeout: 100000001
|
||||
register: result
|
||||
ignore_errors: yes
|
||||
|
||||
- name: verify the task failed
|
||||
assert:
|
||||
that:
|
||||
- result is failed
|
||||
- result is not timedout
|
||||
- result.msg is contains "Timeout 100000001 is invalid"
|
||||
|
||||
- name: run a task with a zero timeout
|
||||
command: sleep 3
|
||||
timeout: 0
|
||||
register: result
|
||||
|
||||
- name: verify the task did not time out
|
||||
assert:
|
||||
that:
|
||||
- result is not timedout
|
||||
- result.delta is search '^0:00:0[3-9]\.' # delta must be between 3 and 9 seconds
|
||||
|
||||
- name: run a task with a large timeout that is not triggered
|
||||
command: sleep 3
|
||||
timeout: 100000000
|
||||
register: result
|
||||
|
||||
- name: verify the task did not time out
|
||||
assert:
|
||||
that:
|
||||
- result is not timedout
|
||||
- result.delta is search '^0:00:0[3-9]\.' # delta must be between 3 and 9 seconds
|
||||
@ -0,0 +1,123 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import contextlib
|
||||
import signal
|
||||
import time
|
||||
import typing as t
|
||||
|
||||
import pytest
|
||||
|
||||
from ansible._internal._errors import _alarm_timeout
|
||||
from ansible._internal._errors._alarm_timeout import AnsibleTimeoutError
|
||||
|
||||
pytestmark = pytest.mark.usefixtures("assert_sigalrm_state")
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def assert_sigalrm_state() -> t.Iterator[None]:
|
||||
"""Fixture to ensure that SIGALRM state is as-expected before and after each test."""
|
||||
assert signal.alarm(0) == 0 # disable alarm before resetting the default handler
|
||||
assert signal.signal(signal.SIGALRM, signal.SIG_DFL) == signal.SIG_DFL
|
||||
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
assert signal.alarm(0) == 0
|
||||
assert signal.signal(signal.SIGALRM, signal.SIG_DFL) == signal.SIG_DFL
|
||||
|
||||
|
||||
@pytest.mark.parametrize("timeout", (0, 1, None))
|
||||
def test_alarm_timeout_success(timeout: int | None) -> None:
|
||||
"""Validate a non-timeout success scenario."""
|
||||
ran = False
|
||||
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(timeout):
|
||||
time.sleep(0.01)
|
||||
ran = True
|
||||
|
||||
assert ran
|
||||
|
||||
|
||||
def test_alarm_timeout_timeout() -> None:
|
||||
"""Validate a happy-path timeout scenario."""
|
||||
ran = False
|
||||
timeout_sec = 1
|
||||
|
||||
with pytest.raises(AnsibleTimeoutError) as error:
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(timeout_sec):
|
||||
time.sleep(timeout_sec + 1)
|
||||
ran = True # pragma: nocover
|
||||
|
||||
assert not ran
|
||||
assert error.value.timeout == timeout_sec
|
||||
|
||||
|
||||
@pytest.mark.parametrize("timeout,expected_error_type,expected_error_pattern", (
|
||||
(-1, ValueError, "Timeout.*invalid.*between"),
|
||||
(100_000_001, ValueError, "Timeout.*invalid.*between"),
|
||||
(0.1, TypeError, "requires 'int' argument.*'float'"),
|
||||
("1", TypeError, "requires 'int' argument.*'str'"),
|
||||
))
|
||||
def test_alarm_timeout_bad_values(timeout: t.Any, expected_error_type: type[Exception], expected_error_pattern: str) -> None:
|
||||
"""Validate behavior for invalid inputs."""
|
||||
ran = False
|
||||
|
||||
with pytest.raises(expected_error_type, match=expected_error_pattern):
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(timeout):
|
||||
ran = True # pragma: nocover
|
||||
|
||||
assert not ran
|
||||
|
||||
|
||||
def test_alarm_timeout_bad_state() -> None:
|
||||
"""Validate alarm state error handling."""
|
||||
def call_it():
|
||||
ran = False
|
||||
|
||||
with pytest.raises(RuntimeError, match="existing alarm"):
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(1):
|
||||
ran = True # pragma: nocover
|
||||
|
||||
assert not ran
|
||||
|
||||
try:
|
||||
# non-default SIGALRM handler present
|
||||
signal.signal(signal.SIGALRM, lambda _s, _f: None)
|
||||
call_it()
|
||||
finally:
|
||||
signal.signal(signal.SIGALRM, signal.SIG_DFL)
|
||||
|
||||
try:
|
||||
# alarm already set
|
||||
signal.alarm(10000)
|
||||
call_it()
|
||||
finally:
|
||||
signal.signal(signal.SIGALRM, signal.SIG_DFL)
|
||||
|
||||
ran_outer = ran_inner = False
|
||||
|
||||
# nested alarm_timeouts
|
||||
with pytest.raises(RuntimeError, match="existing alarm"):
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(1):
|
||||
ran_outer = True
|
||||
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(1):
|
||||
ran_inner = True # pragma: nocover
|
||||
|
||||
assert not ran_inner
|
||||
assert ran_outer
|
||||
|
||||
|
||||
def test_alarm_timeout_raise():
|
||||
"""Ensure that an exception raised in the wrapped scope propagates correctly."""
|
||||
with pytest.raises(NotImplementedError):
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(1):
|
||||
raise NotImplementedError()
|
||||
|
||||
|
||||
def test_alarm_timeout_escape_broad_exception():
|
||||
"""Ensure that the timeout exception can escape a broad exception handler in the wrapped scope."""
|
||||
with pytest.raises(AnsibleTimeoutError):
|
||||
with _alarm_timeout.AnsibleTimeoutError.alarm_timeout(1):
|
||||
with contextlib.suppress(Exception):
|
||||
time.sleep(3)
|
||||
@ -0,0 +1,64 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import collections.abc as c
|
||||
import typing as t
|
||||
|
||||
import pytest
|
||||
|
||||
from ansible._internal._errors import _error_utils
|
||||
from ansible.module_utils._internal import _messages
|
||||
from units.mock.error_helper import raise_exceptions
|
||||
|
||||
|
||||
class _TestContributesError(Exception, _error_utils.ContributesToTaskResult):
|
||||
@property
|
||||
def result_contribution(self) -> c.Mapping[str, object]:
|
||||
return dict(some_flag=True)
|
||||
|
||||
|
||||
class _TestContributesUnreachable(Exception, _error_utils.ContributesToTaskResult):
|
||||
@property
|
||||
def omit_failed_key(self) -> bool:
|
||||
return True
|
||||
|
||||
@property
|
||||
def result_contribution(self) -> c.Mapping[str, object]:
|
||||
return dict(unreachable=True)
|
||||
|
||||
|
||||
class _TestContributesMsg(Exception, _error_utils.ContributesToTaskResult):
|
||||
@property
|
||||
def result_contribution(self) -> c.Mapping[str, object]:
|
||||
return dict(msg="contributed msg")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("exceptions,expected", (
|
||||
(
|
||||
(Exception("e0"), _TestContributesError("e1"), ValueError("e2")),
|
||||
dict(failed=True, some_flag=True, msg="e0: e1: e2"),
|
||||
),
|
||||
(
|
||||
(Exception("e0"), ValueError("e1"), _TestContributesError("e2")),
|
||||
dict(failed=True, some_flag=True, msg="e0: e1: e2"),
|
||||
),
|
||||
(
|
||||
(Exception("e0"), _TestContributesUnreachable("e1")),
|
||||
dict(unreachable=True, msg="e0: e1"),
|
||||
),
|
||||
(
|
||||
(Exception("e0"), _TestContributesMsg()),
|
||||
dict(failed=True, msg="contributed msg"),
|
||||
),
|
||||
))
|
||||
def test_exception_result_contribution(exceptions: t.Sequence[BaseException], expected: dict[str, t.Any]) -> None:
|
||||
"""Validate result dict augmentation by exceptions conforming to the ContributeToTaskResult protocol."""
|
||||
|
||||
with pytest.raises(Exception) as error:
|
||||
raise_exceptions(exceptions)
|
||||
|
||||
result = _error_utils.result_dict_from_exception(error.value, accept_result_contribution=True)
|
||||
|
||||
summary = result.pop('exception')
|
||||
|
||||
assert isinstance(summary, _messages.ErrorSummary)
|
||||
assert result == expected
|
||||
@ -0,0 +1,27 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from ansible._internal._errors._task_timeout import TaskTimeoutError
|
||||
from ansible.module_utils._internal._datatag._tags import Deprecated
|
||||
|
||||
|
||||
def test_task_timeout_result_contribution() -> None:
|
||||
"""Validate the result contribution shape."""
|
||||
try:
|
||||
raise TaskTimeoutError(99)
|
||||
except TaskTimeoutError as tte:
|
||||
contrib = tte.result_contribution
|
||||
|
||||
assert isinstance(contrib, dict)
|
||||
|
||||
timedout = contrib.get('timedout')
|
||||
|
||||
assert isinstance(timedout, dict)
|
||||
|
||||
frame = timedout.get('frame')
|
||||
|
||||
assert isinstance(frame, str)
|
||||
assert Deprecated.is_tagged_on(frame)
|
||||
|
||||
period = timedout.get('period')
|
||||
|
||||
assert period == 99
|
||||
@ -0,0 +1,17 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import collections.abc as c
|
||||
|
||||
|
||||
def raise_exceptions(exceptions: c.Sequence[BaseException]) -> None:
|
||||
"""
|
||||
Raise a chain of exceptions from the given exception list.
|
||||
Exceptions will be raised starting from the end of the list.
|
||||
"""
|
||||
if len(exceptions) > 1:
|
||||
try:
|
||||
raise_exceptions(exceptions[1:])
|
||||
except Exception as ex:
|
||||
raise exceptions[0] from ex
|
||||
|
||||
raise exceptions[0]
|
||||
@ -0,0 +1,21 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from ansible import errors
|
||||
|
||||
from units.test_utils.controller.display import emits_warnings
|
||||
|
||||
|
||||
@pytest.mark.parametrize("name", (
|
||||
"AnsibleFilterTypeError",
|
||||
"_AnsibleActionDone",
|
||||
))
|
||||
def test_deprecated(name: str) -> None:
|
||||
with emits_warnings(deprecation_pattern='is deprecated'):
|
||||
getattr(errors, name)
|
||||
|
||||
|
||||
def test_deprecated_attribute_error() -> None:
|
||||
with pytest.raises(AttributeError):
|
||||
getattr(errors, 'bogus')
|
||||
Loading…
Reference in New Issue