diff --git a/testing/chaos/scenarios/bottleneck-parent.yaml b/testing/chaos/scenarios/bottleneck-parent.yaml index 396059d..c43a97b 100644 --- a/testing/chaos/scenarios/bottleneck-parent.yaml +++ b/testing/chaos/scenarios/bottleneck-parent.yaml @@ -87,6 +87,21 @@ traffic: duration_secs: {min: 5, max: 10} parallel_streams: 2 +# The two test subjects named at the top of this file, encoded. Both +# existed only as comments and were checked by nothing. +# +# n06 has a Bluetooth parent (n02) and a fiber one (n03) at the same depth +# and should take the fiber. n09 has only Bluetooth, so asserting it keeps +# n05 is asserting that the absence of an alternative is handled without +# thrashing, not that Bluetooth was preferred. +# +# From the archived corpus: across the six provably-completed runs, n06's +# parent is n03 in all six and n09's is n05 in all six. +assertions: + tree_parents: + n06: n03 + n09: n05 + logging: rust_log: "info" output_dir: "./sim-results" diff --git a/testing/chaos/scenarios/cost-avoidance.yaml b/testing/chaos/scenarios/cost-avoidance.yaml index c89861a..dec22fe 100644 --- a/testing/chaos/scenarios/cost-avoidance.yaml +++ b/testing/chaos/scenarios/cost-avoidance.yaml @@ -61,6 +61,21 @@ traffic: duration_secs: {min: 5, max: 10} parallel_streams: 2 +# The validation line at the top of this file, encoded. It existed only as +# that comment and was checked by nothing, so the scenario could not fail +# on the one thing it was built to test. +# +# From the archived corpus: of the six runs that carry a status.txt and are +# therefore provably completed, n04's parent is n03 in all six. Among the +# 176 older runs the picture is quite different -- n04 is absent from the +# snapshot in 107 of them and is its own root in 49 -- but those are runs +# whose tree had not converged when the snapshot was taken rather than runs +# that chose the wrong parent, which is why the assertion fails an absent +# or self-parented node explicitly instead of skipping it. +assertions: + tree_parents: + n04: n03 + logging: rust_log: "info" output_dir: "./sim-results" diff --git a/testing/chaos/scenarios/depth-vs-cost.yaml b/testing/chaos/scenarios/depth-vs-cost.yaml index ab2d288..4f779c2 100644 --- a/testing/chaos/scenarios/depth-vs-cost.yaml +++ b/testing/chaos/scenarios/depth-vs-cost.yaml @@ -23,6 +23,23 @@ # # Validation: tree snapshot shows n04's parent selection reflects the # actual cost tradeoff between depth and link quality. +# +# NO PARENT IS ASSERTED HERE, and unlike the other cost scenarios that is +# not because the implementation disagrees with the comment. It is because +# the validation line above does not name an outcome: "reflects the actual +# cost tradeoff" is satisfied by either answer, so there is nothing to +# encode. The comment above even says n04 "may still pick n01". +# +# The corpus agrees that both answers occur. Across the five provably +# completed archived runs, n04's parent is n01 in three and n03 in two, +# with the same seed each time, so the choice is not deterministic under +# this configuration. +# +# Pinning whichever answer is more common would convert an unspecified +# behaviour into a regression guard for something nobody decided, and +# would go red roughly two runs in five besides. What this scenario needs +# first is a decision about which parent is correct given the stated +# effective depths, and that is a protocol question, not a harness one. scenario: name: "depth-vs-cost" diff --git a/testing/chaos/scenarios/mixed-technology.yaml b/testing/chaos/scenarios/mixed-technology.yaml index 0fb240e..bf52c35 100644 --- a/testing/chaos/scenarios/mixed-technology.yaml +++ b/testing/chaos/scenarios/mixed-technology.yaml @@ -30,6 +30,25 @@ # - n08 has fiber (n03) and Bluetooth (n04) parents — should pick n03 # # Netem mutation shifts fiber-only links between normal and degraded. +# +# NEITHER TEST SUBJECT IS ASSERTED, because the implementation does not do +# what the two lines above say. Across the five archived runs that carry a +# status.txt and are therefore provably completed, n06's parent is n10 in +# three and n03 in two, and n08's is n04 in four and n03 in one. Encoding +# either as written would red the scenario most runs; encoding what it +# actually does would pin behaviour nobody specified and would silently +# bless it. +# +# So this is an open question rather than a missing assertion: either +# cost-based selection is not preferring fiber here, or the criteria were +# written for a topology this file no longer has. n06 taking n10 is the +# more interesting half — n10 reaches n06 by fiber, so the choice may be +# defensible and the comment simply stale. It needs someone to work out +# which, and until then this scenario proves nothing about parent choice. +# +# That leaves this scenario asserting nothing about its own subject. The +# baseline convergence assertion is the right floor for it and is tracked +# separately; this note is here so the gap is visible until that lands. scenario: name: "mixed-technology" diff --git a/testing/chaos/sim/assertions.py b/testing/chaos/sim/assertions.py index 6a391d1..467da25 100644 --- a/testing/chaos/sim/assertions.py +++ b/testing/chaos/sim/assertions.py @@ -24,6 +24,7 @@ from .scenario import ( MaxErrorsAssertion, MaxParentSwitchesAssertion, MinParentSwitchesAssertion, + TreeParentsAssertion, ) from .topology import SimTopology @@ -172,6 +173,81 @@ def evaluate_max_parent_switches( ) +def evaluate_tree_parents( + cfg: TreeParentsAssertion, + snapshot: dict | None, +) -> AssertionOutcome: + """Check each node's parent in the final tree snapshot. + + Parents are compared by node address, resolved from the snapshot's own + ``my_node_addr`` fields, so the check does not depend on the display + name a node happened to publish. + + Every way of not knowing the answer is a failure: no snapshot, the + node absent from it, the expected parent absent from it, or the node + still claiming to be its own root. Each of those produces the same + "no match" that a genuinely wrong parent does, and only saying so + separately keeps a harness problem from reading as a routing verdict. + """ + if not snapshot: + return AssertionOutcome( + name="tree_parents", + passed=False, + detail=( + "FAIL tree_parents: no final tree snapshot was taken, so no " + "node's parent was observed. This is a harness failure, not " + "a statement about the tree." + ), + ) + + addr_of = { + nid: data.get("my_node_addr") + for nid, data in snapshot.items() + if data.get("my_node_addr") + } + id_of = {addr: nid for nid, addr in addr_of.items()} + + good, bad = [], [] + for child, want_parent in sorted(cfg.expected.items()): + entry = snapshot.get(child) + if entry is None: + bad.append( + f"{child} is absent from the snapshot ({len(snapshot)} node(s) " + f"present: {', '.join(sorted(snapshot))})" + ) + continue + want_addr = addr_of.get(want_parent) + if want_addr is None: + bad.append( + f"{child}: expected parent {want_parent} is absent from the " + f"snapshot, so its address cannot be resolved" + ) + continue + got_addr = entry.get("parent") + if got_addr == entry.get("my_node_addr"): + bad.append( + f"{child} is its own parent — it still believes it is root, " + f"so the tree never converged around it (wanted {want_parent})" + ) + continue + if got_addr == want_addr: + good.append(f"{child}->{want_parent}") + continue + got_id = id_of.get(got_addr) or entry.get("parent_display_name") or got_addr + bad.append(f"{child} chose {got_id}, wanted {want_parent}") + + if bad: + detail = f"FAIL tree_parents: {'; '.join(bad)}" + if good: + detail += f". Correct: {', '.join(good)}" + return AssertionOutcome(name="tree_parents", passed=False, detail=detail) + return AssertionOutcome( + name="tree_parents", + passed=True, + detail=f"PASS tree_parents: {', '.join(good)}", + ) + + _CONGESTION_FLOORS = ( ("min_nodes_detected", "congestion_detected"), ("min_nodes_ce_forwarded", "ce_forwarded"), diff --git a/testing/chaos/sim/runner.py b/testing/chaos/sim/runner.py index 97ad1a8..0327508 100644 --- a/testing/chaos/sim/runner.py +++ b/testing/chaos/sim/runner.py @@ -17,6 +17,7 @@ from .assertions import ( BloomSendRateMonitor, evaluate_congestion_signals, evaluate_max_errors, + evaluate_tree_parents, evaluate_max_parent_switches, evaluate_min_parent_switches, ) @@ -80,6 +81,7 @@ class SimRunner: # which the congestion assertion must treat as a failure rather # than as an absence of congestion. self.final_congestion: dict | None = None + self.final_tree: dict | None = None def _evaluate_max_parent_switches( self, cfg, parent_switches: list[tuple[str, str]] @@ -622,6 +624,15 @@ class SimRunner: else: log.error("%s", outcome.detail) + tp_cfg = self.scenario.assertions.tree_parents + if tp_cfg is not None: + outcome = evaluate_tree_parents(tp_cfg, self.final_tree) + self.assertion_outcomes.append(outcome) + if outcome.passed: + log.info("%s", outcome.detail) + else: + log.error("%s", outcome.detail) + cong_cfg = self.scenario.assertions.congestion_signals if cong_cfg is not None: outcome = evaluate_congestion_signals( @@ -698,6 +709,7 @@ class SimRunner: # failure present as an assertion that saw nothing. if label == "final": self.final_congestion = congestion_snap + self.final_tree = tree_snap os.makedirs(self.output_dir, exist_ok=True) with open(tree_path, "w") as f: json.dump(tree_snap, f, indent=2) diff --git a/testing/chaos/sim/scenario.py b/testing/chaos/sim/scenario.py index 39171a5..80d80f7 100644 --- a/testing/chaos/sim/scenario.py +++ b/testing/chaos/sim/scenario.py @@ -3,10 +3,16 @@ from __future__ import annotations import os +import re from dataclasses import dataclass, field import yaml +# Node ids are rendered nNN by the topology generator, zero-padded to two +# digits. Matching the shape here keeps a typo such as "no4" or "n4x" from +# reaching an assertion as a node that simply never appears. +_NODE_ID_RE = re.compile(r"n\d+") + @dataclass class Range: @@ -218,6 +224,24 @@ class MaxParentSwitchesAssertion: node: str | None = None +@dataclass +class TreeParentsAssertion: + """Expected parent per node in the final tree snapshot. + + ``expected`` maps a node id to the node id its parent must be, e.g. + ``{"n04": "n03"}``. Both sides are node ids rather than node + addresses: an address is a per-run key derived from the generated + identity, so a scenario could not name one ahead of time. + + A node absent from the snapshot fails rather than being skipped. That + case is not hypothetical — more than half the archived runs of these + scenarios have no entry for the node under test, and a skip would + have reported those as satisfied. + """ + + expected: dict[str, str] = field(default_factory=dict) + + @dataclass class CongestionSignalsAssertion: """Floors on how many nodes observed each congestion signal. @@ -269,6 +293,7 @@ class AssertionsConfig: max_parent_switches: MaxParentSwitchesAssertion | None = None max_errors: MaxErrorsAssertion | None = None congestion_signals: CongestionSignalsAssertion | None = None + tree_parents: TreeParentsAssertion | None = None @dataclass @@ -304,10 +329,13 @@ class Scenario: # YAML still looks correct — a mistyped "assertion:" disarms a scenario's only # assertions, a mistyped "link_flap:" turns off the chaos it was meant to inject. # -# Three mappings are deliberately NOT listed, because their keys are names the +# Four mappings are deliberately NOT listed, because their keys are names the # scenario author chooses rather than a schema: netem.mutation.policies, -# link_swap.policies and topology.transport_mix. Two sub-trees are passed through -# whole and are likewise not checked: fips_overrides and topology.params. +# link_swap.policies, topology.transport_mix and assertions.tree_parents (whose +# keys are node ids). Two sub-trees are passed through whole and are likewise +# not checked: fips_overrides and topology.params. tree_parents is not thereby +# unchecked -- both sides of every entry are validated as node ids that exist in +# this scenario's topology, which is the check that matters for it. # # NOTE: adding a new assertion type means registering it in TWO places below — # _SECTION_KEYS["assertions"] (so the block accepts its name) and _ASSERTION_KEYS @@ -345,7 +373,7 @@ _SECTION_KEYS = { "link_swap.edges[]": {"edge", "policy"}, "assertions": { "bloom_send_rate", "min_parent_switches", "max_parent_switches", - "max_errors", "congestion_signals", + "max_errors", "congestion_signals", "tree_parents", }, "logging": {"rust_log", "output_dir"}, } @@ -697,6 +725,39 @@ def load_scenario(path: str) -> Scenario: "min_nodes_ce_received); a block with none asserts nothing" ) s.assertions.congestion_signals = CongestionSignalsAssertion(**floors) + if "tree_parents" in asrt: + tp = asrt["tree_parents"] + if not isinstance(tp, dict) or not tp: + raise ValueError( + "assertions.tree_parents: give it at least one " + "': ' entry; an empty block asserts " + "nothing" + ) + expected = {} + for child, parent in tp.items(): + for role, val in (("node", child), ("parent", parent)): + if not isinstance(val, str) or not _NODE_ID_RE.fullmatch(val): + raise ValueError( + f"assertions.tree_parents: {role} {val!r} is not a node " + f"id of the form 'n04'" + ) + idx = int(val[1:]) + if idx < 1 or idx > s.topology.num_nodes: + raise ValueError( + f"assertions.tree_parents: {role} '{val}' is outside " + f"this scenario's {s.topology.num_nodes} nodes. An " + f"assertion about a node that cannot exist would fail " + f"for the wrong reason every run." + ) + if child == parent: + raise ValueError( + f"assertions.tree_parents: '{child}' is given itself as " + f"its parent. A node is its own parent only when it " + f"believes it is root, which is what an unconverged tree " + f"looks like; assert that some other way." + ) + expected[child] = parent + s.assertions.tree_parents = TreeParentsAssertion(expected=expected) # Logging section lg = raw.get("logging", {})