fix(optimize-scale): make the load spike actually inject (timeout + kind)

devops_bench/chaos/faults/generate_load.py

@@ -28,6 +28,7 @@
 import contextlib
 import json
 import os
+import re
 import shlex
 import textwrap
 import threading
@@ -57,9 +58,19 @@
 # active. The harness watches the shared event to coordinate measurements.
 _LOAD_MARKER = "fortio load"
 
-# Wall-clock ceiling for a single chaos command.
+# Wall-clock ceiling for a single chaos command that is not a load spike.
 _COMMAND_TIMEOUT = 40
 
+# A load spike has to outlive its own ``-t`` duration, so its ceiling is derived
+# from the command rather than fixed. The flat 40s ceiling silently killed every
+# spike a task declared for longer than that: optimize-scale asks for 300s
+# deliberately (so the spike is still running when verification starts), fortio
+# was SIGKILLed at 40s, the fault recorded exit -1, and the run was scored
+# ``chaos_invalidated`` with the reason "load did not reach the workload" — which
+# reads as unreachable rather than cut short.
+_LOAD_TIMEOUT_SLACK_SEC = 60
+_LOAD_TIMEOUT_CEILING_SEC = 900
+
 # The workload's in-cluster (remote) port for chaos load generation, and the
 # default local side of the port-forward. Parallel runs override only the local
 # side via ``_ENV_LOCAL_PORT`` so two concurrent forwards do not contend.
@@ -85,6 +96,41 @@
 _TARGET_READY_TIMEOUT_SEC = 120
 
 
+def _go_duration_seconds(value: str) -> float | None:
+    """Parse a Go-style duration (``300s``, ``5m``, ``1h30m``) into seconds.
+
+    fortio takes its ``-t`` in Go's format. Returns ``None`` for anything not
+    understood, so the caller falls back to the fixed ceiling rather than
+    inventing a budget from a value it misread.
+    """
+    parts = re.findall(r"([0-9]*\.?[0-9]+)\s*(ms|h|m|s)", value.strip())
+    if not parts:
+        return None
+    unit = {"h": 3600.0, "m": 60.0, "s": 1.0, "ms": 0.001}
+    total = 0.0
+    for amount, suffix in parts:
+        total += float(amount) * unit[suffix]
+    return total or None
+
+
+def _command_timeout(argv: list[str], *, is_load: bool) -> float:
+    """Wall-clock ceiling for this command.
+
+    A load spike gets its declared duration plus slack (bounded), so the
+    generator is never killed mid-spike. Everything else keeps the flat
+    ceiling.
+    """
+    if not is_load:
+        return _COMMAND_TIMEOUT
+    for index, token in enumerate(argv):
+        if token == "-t" and index + 1 < len(argv):
+            declared = _go_duration_seconds(argv[index + 1])
+            if declared is None:
+                break
+            return min(declared + _LOAD_TIMEOUT_SLACK_SEC, _LOAD_TIMEOUT_CEILING_SEC)
+    return _COMMAND_TIMEOUT
+
+
 def build_system_instruction(target_url: str = _DEFAULT_TARGET_URL) -> str:
     """Build the SRE system instruction, targeting ``target_url`` for load.
 
@@ -199,7 +245,7 @@ def run_chaos_command(
             _log.info("load spike detected; signaling harness via chaos event")
             chaos_active_event.set()
 
-        completed = run(argv, check=False, timeout=_COMMAND_TIMEOUT)
+        completed = run(argv, check=False, timeout=_command_timeout(argv, is_load=is_load))
         if is_load and load_result is not None:
             # Record the spike's real exit status so the fault can fail closed:
             # a non-zero fortio exit means it could not reach the workload.

tasks/common/optimize-scale/README.md

@@ -92,24 +92,28 @@ at 15s, the objectives above are still evaluated against a cluster that was neve
 entry's status in `results.json` before reading a passing score as evidence the workload absorbed
 anything.
 
-## Why GKE
+## Why kind
 
-This task is pinned to `gcp`, which is what every published run of it was graded against.
+Two things used to argue for GKE, and neither survives measurement.
 
-The metrics objective needs a working metrics pipeline. GKE ships metrics-server; a stock kind
-cluster does not, so `ScalingActive` would read `False` for reasons that have nothing to do with
-the agent. The stack now installs metrics-server itself under `infra_provider=kind`, so that
-reason alone no longer forces GKE.
+Metrics: the HPA objective grades `ScalingActive=True`, which needs a live
+metrics pipeline. A stock kind cluster ships none — but the stack now installs
+metrics-server itself under `infra_provider=kind`, so the objective is decided
+by the agent again.
 
-The load path is what still does. On GKE the Service is a `LoadBalancer` and the chaos generator
-reaches it directly; on kind it is a `ClusterIP` behind the harness port-forward. The planned load
-spike failed to inject in **8 of 8** published runs, so the fewer moving parts in that path the
-better until a spike is demonstrably landing.
+The load path: on GKE the Service is a LoadBalancer the chaos generator was
+meant to reach directly. In practice it cannot — fortio times out on the
+external IP (`dial tcp <ip>:8080: i/o timeout`) because the project's firewall
+admits only 22/3389/443, so the spike never injects. On kind the Service is a
+`ClusterIP` behind the harness port-forward, which connects and serves the
+spike.
 
-`INFRA_PROVIDER=kind` is a genuinely working alternative now — metrics included — and much cheaper
-for local iteration on the fixture. Whichever you pick, **every runner must pick the same one**:
-the provider changes the Service type and the load path, so a kind arm and a GKE arm are not
-comparable on this task.
+The historic 8-of-8 injection failures were a third thing entirely: every chaos
+command shared a 40s ceiling, and this task declares a 300s spike, so fortio was
+killed mid-run and the fault reported "load did not reach the workload".
+
+`INFRA_PROVIDER=gcp` still selects GKE and is the better load path once a runner
+can reach a LoadBalancer.
 
 ## Run
 

tasks/common/optimize-scale/task.yaml

@@ -7,28 +7,31 @@ name: "optimize-scale"
 # target_deployment_name/namespace can be substituted straight into the
 # {{TARGET_DEPLOYMENT_NAME}}/{{NAMESPACE}} placeholders below.
 #
-# Pinned to gcp, which is what every published run of this task was graded
-# against. Two reasons, and the second is the one that bites:
+# Runs on kind. This task was pinned to gcp for two reasons; measurement has
+# now retired both:
 #
 #   1. The objectives depend on a working metrics pipeline. GKE ships
 #      metrics-server; a stock kind cluster does not, so ScalingActive would
 #      read False for reasons that have nothing to do with the agent. The
 #      stack now installs metrics-server itself under infra_provider=kind, so
 #      this reason no longer holds on its own.
-#   2. On GKE the Service is a LoadBalancer and the chaos load reaches it
-#      directly. On kind it needs a port-forward — one more moving part in the
-#      exact path that already failed to inject the spike in 8 of 8 published
-#      runs. Until a spike demonstrably lands on kind, gcp is the arm to
-#      compare on.
+#   2. The load path was the stronger argument for GKE: a LoadBalancer the
+#      chaos generator reaches directly, versus a port-forward on kind. It ran
+#      the other way in practice. On GKE fortio never connects at all — the
+#      LB's external IP is unreachable from the runner (`dial tcp <ip>:8080:
+#      i/o timeout`; the project's firewall admits only 22/3389/443). On kind
+#      the port-forward connects and serves the spike. The 8-of-8 injection
+#      failures were not the port-forward: every chaos command shared a 40s
+#      ceiling that killed this task's declared 300s spike outright.
 #
-# INFRA_PROVIDER=kind is now a genuinely working alternative (metrics included)
-# for cheap local iteration. Whichever is used, every runner must use the SAME
-# one: the provider changes the Service type and the load path, so a kind arm
-# and a GKE arm are not comparable on this task.
+# INFRA_PROVIDER=gcp still selects GKE, and remains the better load path the
+# day the runner can reach a LoadBalancer. Whichever is used, every runner must
+# use the SAME one: the provider changes the Service type and the load path, so
+# a kind arm and a GKE arm are not comparable on this task.
 infrastructure:
   deployer: "tofu"
   stack: "prebuilt/optimize-scale"
-  provider: "gcp"
+  provider: "kind"
   teardown: true
   variables:
     namespace: "default"

tests/unit/chaos/test_generate_load.py

@@ -27,6 +27,8 @@
 from typing import Any
 from unittest.mock import MagicMock, patch
 
+import pytest
+
 from devops_bench.chaos.base import ChaosResult
 from devops_bench.chaos.faults import generate_load as gl
 from devops_bench.chaos.faults.generate_load import (
@@ -456,3 +458,37 @@ def test_inject_port_forward_setup_failure_becomes_failed_result() -> None:
     assert result.success is False
     assert result.error is not None
     assert "kubectl missing" in result.error
+
+
+class TestLoadCommandTimeout:
+    """A spike must outlive its own ``-t``; everything else keeps the flat cap."""
+
+    def test_spike_timeout_covers_the_declared_duration(self):
+        # 300s is what optimize-scale declares. Under the old flat 40s ceiling
+        # fortio was killed mid-spike and the fault reported "load did not reach
+        # the workload", which reads as unreachable rather than cut short.
+        argv = ["fortio", "load", "-qps", "300", "-t", "300s", "-c", "2", "http://localhost:8080"]
+        assert gl._command_timeout(argv, is_load=True) > 300
+
+    def test_spike_timeout_is_bounded(self):
+        argv = ["fortio", "load", "-t", "24h", "http://localhost:8080"]
+        assert gl._command_timeout(argv, is_load=True) == gl._LOAD_TIMEOUT_CEILING_SEC
+
+    def test_non_load_command_keeps_the_flat_ceiling(self):
+        assert gl._command_timeout(["kubectl", "get", "pods"], is_load=False) == gl._COMMAND_TIMEOUT
+
+    def test_unparsable_duration_falls_back_rather_than_guessing(self):
+        argv = ["fortio", "load", "-t", "banana", "http://localhost:8080"]
+        assert gl._command_timeout(argv, is_load=True) == gl._COMMAND_TIMEOUT
+
+    def test_load_without_a_duration_flag_keeps_the_flat_ceiling(self):
+        assert gl._command_timeout(["fortio", "load", "http://x"], is_load=True) == (
+            gl._COMMAND_TIMEOUT
+        )
+
+    @pytest.mark.parametrize(
+        ("value", "expected"),
+        [("300s", 300.0), ("5m", 300.0), ("1h30m", 5400.0), ("250ms", 0.25), ("nope", None)],
+    )
+    def test_go_duration_parsing(self, value, expected):
+        assert gl._go_duration_seconds(value) == expected