diff --git a/doc/development.rst b/doc/development.rst index 5a58abb52..2ded88871 100644 --- a/doc/development.rst +++ b/doc/development.rst @@ -226,7 +226,7 @@ Start by creating a strategy skeleton: import attr from labgrid.step import step - from labgrid.strategy import Strategy, StrategyError + from labgrid.strategy import Strategy, StrategyError, never_retry from labgrid.factory import target_factory class Status(enum.Enum): @@ -239,6 +239,7 @@ Start by creating a strategy skeleton: status = attr.ib(default=Status.unknown) + @never_retry @step() def transition(self, status, *, step): if not isinstance(status, Status): @@ -262,9 +263,21 @@ It is possible to reference drivers via their protocol, e.g. Note that drivers which implement multiple protocols must not be referenced multiple times via different protocols. The ``Status`` class needs to be extended to cover the states of your strategy, -then for each state an ``elif`` entry in the transition function needs to be +then for each state an ``elif`` entry in the ``transition()`` method needs to be added. +.. note:: + Since infrastructure failures or broken strategies typically cannot recover, + it makes little sense to continue operating with such a strategy after an + error has occurred. + To clearly mark a strategy as unusable after failure (and to avoid cascading + errors in subsequent calls) the strategy's ``transition()`` method (and + optionally its ``force()`` method) can be decorated with the + ``@never_retry`` decorator. + This decorator causes the strategy to store the encountered exception in its + ``broken`` attribute and raise a ``StrategyError`` for the original and all + subsequent calls to the decorated methods. + Lets take a look at the builtin `BareboxStrategy`. The Status enum for the BareboxStrategy: diff --git a/doc/usage.rst b/doc/usage.rst index d21776a93..69024f087 100644 --- a/doc/usage.rst +++ b/doc/usage.rst @@ -421,6 +421,10 @@ target (session scope) strategy (session scope) Used to access the :any:`Strategy` configured in the 'main' :any:`Target`. + If the Strategy enters broken state, all subsequent tests requesting it via + this fixture will be skipped. + See also :any:`never_retry` for an easy way to mark strategies broken on + error. Command-Line Options ~~~~~~~~~~~~~~~~~~~~ diff --git a/examples/qemu-networking/qemunetworkstrategy.py b/examples/qemu-networking/qemunetworkstrategy.py index 48becd0da..fa62e40c4 100644 --- a/examples/qemu-networking/qemunetworkstrategy.py +++ b/examples/qemu-networking/qemunetworkstrategy.py @@ -17,7 +17,7 @@ import attr from labgrid import target_factory, step -from labgrid.strategy import Strategy, StrategyError +from labgrid.strategy import Strategy, StrategyError, never_retry from labgrid.util import get_free_port @@ -77,6 +77,7 @@ def update_network_service(self): networkservice.address = new_address networkservice.port = self.__remote_port + @never_retry @step(args=["state"]) def transition(self, state, *, step): if not isinstance(state, Status): diff --git a/examples/strategy/bareboxrebootstrategy.py b/examples/strategy/bareboxrebootstrategy.py index 9b3542f82..d9faecaaf 100644 --- a/examples/strategy/bareboxrebootstrategy.py +++ b/examples/strategy/bareboxrebootstrategy.py @@ -5,7 +5,7 @@ from labgrid import target_factory, step from labgrid.driver import BareboxDriver, ShellDriver from labgrid.protocol import PowerProtocol -from labgrid.strategy import Strategy +from labgrid.strategy import Strategy, never_retry @attr.s(eq=False) @@ -58,6 +58,7 @@ class BareboxRebootStrategy(Strategy): def __attrs_post_init__(self): super().__attrs_post_init__() + @never_retry @step(args=["new_status"]) def transition(self, new_status, *, step): if not isinstance(new_status, Status): diff --git a/examples/strategy/quartusstrategy.py b/examples/strategy/quartusstrategy.py index 78f896aeb..9cf22b145 100644 --- a/examples/strategy/quartusstrategy.py +++ b/examples/strategy/quartusstrategy.py @@ -5,7 +5,7 @@ from labgrid import target_factory, step from labgrid.driver import QuartusHPSDriver, SerialDriver from labgrid.protocol import PowerProtocol -from labgrid.strategy import Strategy +from labgrid.strategy import Strategy, never_retry @attr.s(eq=False) @@ -37,6 +37,7 @@ class QuartusHPSStrategy(Strategy): def __attrs_post_init__(self): super().__attrs_post_init__() + @never_retry @step(args=["status"]) def transition(self, status, *, step): if not isinstance(status, Status): diff --git a/examples/usbpower/examplestrategy.py b/examples/usbpower/examplestrategy.py index 98bc4b56e..b47d3e780 100644 --- a/examples/usbpower/examplestrategy.py +++ b/examples/usbpower/examplestrategy.py @@ -5,7 +5,7 @@ from labgrid.driver import BareboxDriver, ShellDriver, USBSDMuxDriver from labgrid import step, target_factory from labgrid.protocol import PowerProtocol -from labgrid.strategy import Strategy +from labgrid.strategy import Strategy, never_retry @attr.s(eq=False) @@ -36,6 +36,7 @@ class ExampleStrategy(Strategy): def __attrs_post_init__(self): super().__attrs_post_init__() + @never_retry @step(args=["status"]) def transition(self, status, *, step): if not isinstance(status, Status): diff --git a/labgrid/pytestplugin/__init__.py b/labgrid/pytestplugin/__init__.py index 63324bdfc..9130e844c 100644 --- a/labgrid/pytestplugin/__init__.py +++ b/labgrid/pytestplugin/__init__.py @@ -1,2 +1,2 @@ from .fixtures import pytest_addoption, env, target, strategy -from .hooks import pytest_configure, pytest_collection_modifyitems, pytest_cmdline_main +from .hooks import pytest_configure, pytest_collection_modifyitems, pytest_cmdline_main, pytest_runtest_setup diff --git a/labgrid/pytestplugin/hooks.py b/labgrid/pytestplugin/hooks.py index f49c1c6c7..14945863f 100644 --- a/labgrid/pytestplugin/hooks.py +++ b/labgrid/pytestplugin/hooks.py @@ -7,6 +7,7 @@ from ..consoleloggingreporter import ConsoleLoggingReporter from ..util.helper import processwrapper from ..logging import StepFormatter, StepLogger +from ..exceptions import NoStrategyFoundError LABGRID_ENV_KEY = pytest.StashKey[Environment]() @@ -130,3 +131,23 @@ def pytest_collection_modifyitems(config, items): reason=f'Skipping because features "{missing_feature}" are not supported' ) item.add_marker(skip) + +@pytest.hookimpl(tryfirst=True) +def pytest_runtest_setup(item): + """ + Skip test if one of the targets uses a strategy considered broken. + """ + # Before any fixtures run for the test, check if the session-scoped strategy fixture was + # requested (might have been executed already for a prior test). If that's the case and the + # strategy is broken, skip the test. + if "strategy" in item.fixturenames: + env = item.config.stash[LABGRID_ENV_KEY] + # skip test even if only one of the targets in the env has a broken strategy + for target_name in env.config.get_targets(): + target = env.get_target(target_name) + try: + strategy = target.get_strategy() + if strategy.broken: + pytest.skip(f"{strategy.__class__.__name__} is in broken state") + except NoStrategyFoundError: + pass diff --git a/labgrid/strategy/__init__.py b/labgrid/strategy/__init__.py index 5d57ad3a2..8564f3451 100644 --- a/labgrid/strategy/__init__.py +++ b/labgrid/strategy/__init__.py @@ -1,4 +1,4 @@ -from .common import Strategy, StrategyError +from .common import Strategy, StrategyError, never_retry from .bareboxstrategy import BareboxStrategy from .shellstrategy import ShellStrategy from .ubootstrategy import UBootStrategy diff --git a/labgrid/strategy/bareboxstrategy.py b/labgrid/strategy/bareboxstrategy.py index 61c3d655e..8cae9eb6a 100644 --- a/labgrid/strategy/bareboxstrategy.py +++ b/labgrid/strategy/bareboxstrategy.py @@ -4,7 +4,7 @@ from ..factory import target_factory from ..step import step -from .common import Strategy, StrategyError +from .common import Strategy, StrategyError, never_retry class Status(enum.Enum): @@ -30,6 +30,7 @@ class BareboxStrategy(Strategy): def __attrs_post_init__(self): super().__attrs_post_init__() + @never_retry @step(args=['status']) def transition(self, status, *, step): # pylint: disable=redefined-outer-name if not isinstance(status, Status): @@ -63,6 +64,7 @@ def transition(self, status, *, step): # pylint: disable=redefined-outer-name ) self.status = status + @never_retry @step(args=['status']) def force(self, status): if not isinstance(status, Status): diff --git a/labgrid/strategy/common.py b/labgrid/strategy/common.py index 6263f7b96..c29f86e0b 100644 --- a/labgrid/strategy/common.py +++ b/labgrid/strategy/common.py @@ -1,9 +1,33 @@ +import functools + import attr from ..binding import BindingError from ..driver import Driver +def never_retry(func): + """ + Strategy method decorator storing the original exception in strategy.broken and raising a + StrategyError from it. All subsequent calls to the decorated method will lead to the same + exception. + """ + @functools.wraps(func) + def wrapper(self, *args, **kwargs): + if self.broken: + # a previous call broke the strategy, raise the original exception again + raise StrategyError(f"{self.__class__.__name__} is in broken state") from self.broken + + try: + return func(self, *args, **kwargs) + except Exception as e: + # store original exception and raise StrategyError from it + self.broken = e + raise StrategyError(f"{self.__class__.__name__} is in broken state") from self.broken + + return wrapper + + @attr.s(eq=False) class StrategyError(Exception): msg = attr.ib(validator=attr.validators.instance_of(str)) @@ -26,6 +50,10 @@ class Strategy(Driver): # reuse driver handling def __attrs_post_init__(self): super().__attrs_post_init__() + + # contains exception that broke the strategy or None + self.broken = None + if self.target is None: raise BindingError( "Strategies can only be created on a valid target" diff --git a/labgrid/strategy/dockerstrategy.py b/labgrid/strategy/dockerstrategy.py index 420c48c6a..b1bb44a75 100644 --- a/labgrid/strategy/dockerstrategy.py +++ b/labgrid/strategy/dockerstrategy.py @@ -3,7 +3,7 @@ import attr from ..factory import target_factory -from .common import Strategy, StrategyError +from .common import Strategy, StrategyError, never_retry from ..driver.dockerdriver import DockerDriver from ..step import step @@ -33,6 +33,7 @@ class DockerStrategy(Strategy): def __attrs_post_init__(self): super().__attrs_post_init__() + @never_retry @step(args=['status']) def transition(self, status): if not isinstance(status, Status): diff --git a/labgrid/strategy/shellstrategy.py b/labgrid/strategy/shellstrategy.py index 13291b7e0..0f2c7ed7a 100644 --- a/labgrid/strategy/shellstrategy.py +++ b/labgrid/strategy/shellstrategy.py @@ -4,7 +4,7 @@ from ..factory import target_factory from ..step import step -from .common import Strategy, StrategyError +from .common import Strategy, StrategyError, never_retry class Status(enum.Enum): @@ -28,6 +28,7 @@ class ShellStrategy(Strategy): def __attrs_post_init__(self): super().__attrs_post_init__() + @never_retry @step(args=['status']) def transition(self, status, *, step): # pylint: disable=redefined-outer-name if not isinstance(status, Status): @@ -53,6 +54,7 @@ def transition(self, status, *, step): # pylint: disable=redefined-outer-name ) self.status = status + @never_retry @step(args=['status']) def force(self, status, *, step): # pylint: disable=redefined-outer-name if not isinstance(status, Status): diff --git a/labgrid/strategy/ubootstrategy.py b/labgrid/strategy/ubootstrategy.py index 7befb819a..8cbafc452 100644 --- a/labgrid/strategy/ubootstrategy.py +++ b/labgrid/strategy/ubootstrategy.py @@ -3,7 +3,7 @@ import attr from ..factory import target_factory -from .common import Strategy, StrategyError +from .common import Strategy, StrategyError, never_retry class Status(enum.Enum): @@ -29,6 +29,7 @@ class UBootStrategy(Strategy): def __attrs_post_init__(self): super().__attrs_post_init__() + @never_retry def transition(self, status): if not isinstance(status, Status): status = Status[status] @@ -58,6 +59,7 @@ def transition(self, status): raise StrategyError(f"no transition found from {self.status} to {status}") self.status = status + @never_retry def force(self, status): if not isinstance(status, Status): status = Status[status] diff --git a/tests/test_docker.py b/tests/test_docker.py index bfdaf3534..f78aa405e 100644 --- a/tests/test_docker.py +++ b/tests/test_docker.py @@ -308,10 +308,18 @@ def test_docker_without_daemon(docker_env, mocker): with pytest.raises(StrategyError): strategy.transition("unknown") + # test and reset broken state + assert isinstance(strategy.broken, StrategyError) + strategy.broken = None + # Also bonus: How are invalid state names handled? - with pytest.raises(KeyError): + with pytest.raises(StrategyError): strategy.transition("this is not a valid state") + # test and reset broken state + assert isinstance(strategy.broken, KeyError) + strategy.broken = None + # Return to "gone" state - to also use that part of the DockerDriver code. strategy.transition("gone") from labgrid.strategy.dockerstrategy import Status