Introduce Broken Strategy State, Prevent Further Use and Skip Subsequent Tests

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:
 

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
 ~~~~~~~~~~~~~~~~~~~~

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):

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):

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):

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):

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

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

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

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):

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"

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):

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):

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]

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