feat(graph_watchdog): param_drift detector (GRAPH_PARAM_DRIFT) - #580
feat(graph_watchdog): param_drift detector (GRAPH_PARAM_DRIFT)#580bburda wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new param_drift graph_watchdog detector that detects ROS 2 node parameter drift (self-captured baseline and/or config-pinned expect values) and reports it as GRAPH_PARAM_DRIFT, with bounded, off-executor parameter-service reads and end-to-end config plumbing validation.
Changes:
- Implement
param_driftdetector (background reader thread + round-robin, budgeted parameter reads; aggregated WARN-level fault). - Add new unit/integration/e2e tests for policy logic, detector behavior, and nested config delivery through the real gateway.
- Update graph_watchdog documentation (README + design doc) and wire new tests into CMake.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_param_drift_policy.cpp | New unit tests for drift comparison + config parsing/flattening rules. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_param_drift_integration.cpp | New integration tests driving real parameter services and ReportFault behavior. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py | New launch_testing e2e coverage for nested config plumbing into detectors. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/param_drift_detector.cpp | New detector implementation (budgeted read loop + aggregation + prune/forget). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/README.md | Documentation for param_drift keys/behavior and “Closing the loop” healing guidance. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/param_drift_policy.hpp | New ROS-free policy core and config parser/validator for param_drift. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/design/graph_watchdog.rst | Design documentation extended to include param_drift. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/CMakeLists.txt | Adds new gtest + integration + e2e targets for param_drift and config plumbing. |
Comments suppressed due to low confidence (2)
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py:131
- This unittest.skipUnless scenario gating is a skipped test, which the project review guidelines disallow. Consider restructuring so only the scenario-relevant tests are discovered/executed for each CTest target (no skips).
class TestConfigPlumbingModeOff(unittest.TestCase):
"""Mode suppression: the nested `mode` config disables the detector."""
@unittest.skipUnless(SCENARIO == 'mode_off', 'runs only under the "mode_off" scenario launch')
def test_mode_off_suppresses_detector(self):
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py:160
- unittest.skipUnless is used to gate the scenario-specific test case, but skipped tests are disallowed by the project's test rules. Restructure so each scenario target runs only its tests without skip decorators.
class TestConfigPlumbingModeOffYamlBool(unittest.TestCase):
"""A BARE `off` disables the detector, not just a quoted one."""
@unittest.skipUnless(
SCENARIO == 'mode_off_yaml_bool',
'runs only under the "mode_off_yaml_bool" scenario launch',
)
| // node's parameter service is slow. So the reads run on a dedicated background thread and the | ||
| // tick thread only ever reads the cache. State shared between the two threads lives here, | ||
| // behind a shared_ptr so a reader stuck in a hung read keeps it alive (the detector detaches | ||
| // rather than joins on teardown - a hung blocking read cannot be interrupted, so joining it | ||
| // would hang the gateway shutdown). |
| target = io->targets[idx % io->targets.size()]; | ||
| ++idx; | ||
| const int reads = io->max_reads_per_tick > 0 ? io->max_reads_per_tick : 1; | ||
| min_gap = std::chrono::milliseconds(io->tick_interval_ms / reads); | ||
| } |
| class TestConfigPlumbingPositive(unittest.TestCase): | ||
| """Positive control: the nested `expect` config reaches param_drift.""" | ||
|
|
||
| @unittest.skipUnless(SCENARIO == 'positive', 'runs only under the "positive" scenario launch') | ||
| def test_expect_rule_raises_from_nested_config(self): |
48e3f77 to
df83df1
Compare
| constexpr std::uint8_t kParamDriftSeverity = ros2_medkit_msgs::msg::Fault::SEVERITY_WARN; | ||
| constexpr double kParamServiceTimeoutSec = 0.5; // per-read timeout, same as the lifecycle GetState read | ||
| constexpr double kNegativeCacheTtlSec = 10.0; // unreachable-node backoff | ||
| constexpr int kForgetGrace = 2; // missed-sweep grace before forgetting, mirrors WarmupTracker |
There was a problem hiding this comment.
A restart faster than this grace keeps the stale baseline forever. A node absent for <= kForgetGrace sweeps (~2 s at the default 1 s tick, or never observed absent at all) is not forgotten, and WarmupTracker's forget grace is the same 2, so it comes back still armed and gets evaluated against the pre-restart baseline. The standard flow - edit YAML, respawn the node (launch respawn defaults to 0 s delay) - then raises GRAPH_PARAM_DRIFT permanently on a healthy robot, and the README's recovery path ("restart the node, and the detector captures the new value as a fresh baseline") silently fails; the only exits are a deliberately slow restart or a gateway restart. Consider dropping the baseline on any observed absence - a churn-triggered re-capture loses at most one drift window, while a stale baseline latches a false positive with no way out - or document that only an outage longer than the prune grace re-baselines.
There was a problem hiding this comment.
I tried dropping the baseline on any absence and reverted it. On the first missed sweep, normal discovery churn re-captures a node on an already drifted value, so a confirmed fault heals while the parameter is still wrong and can never fire again.
It drops on the third missed sweep now, and the README says what that means in practice: a respawn with no delay is often never seen as an absence, so it does not re-baseline. Use respawn_delay, or stop and start. ChurnMustNotLaunderARaisedDrift and ARestartLengthAbsenceReBaselinesInsteadOfReportingDrift pin both sides.
| return false; // shutting down mid-read: incomplete view -> skip (no spurious clear) | ||
| } | ||
| } | ||
| const auto result = io.transport->get_parameter(fqn, name); |
There was a problem hiding this comment.
max_reads_per_tick paces node visits, but the docs promise service calls. Each pinned name here costs the watched node three service round-trips (list + get + describe inside get_parameter), issued back to back inside one budget slot, so E pins fire ~3E calls at a single node per visit; baseline mode is 2 RPCs per visit and 4-5 on first contact (cache_default_values runs its own list + get before list_parameters repeats them). The README table says "Parameter-service calls this detector may spend per tick" and reader_loop's comment says "max_reads_per_tick transport calls per tick period" - an operator lowering the knob to protect a loaded node still gets a multiple of it. Keep the per-node read atomic (ExpectBudgetTruncationDoesNotFlap pins that), but charge the slot for the calls actually made, e.g. scale the pacing wait by the call count, or add a batch get-by-names to the transport so an expect visit is one round trip.
There was a problem hiding this comment.
Agreed, and your numbers were right. It is charged in round trips now: 2 for a baseline read, 3 for a declared pin, 2 for an undeclared one.
A new e2e measures it against a node that counts inbound requests: baseline 4 cold and 2 warm, declared pin 3, undeclared pin 1. Your "4-5 on first contact" is 4.
The undeclared pin is charged 2 while it costs 1, because a second path returns NOT_FOUND after list + get and the error code does not separate them. The README table splits cost from charge.
Still no pacing inside a visit: 4 pins at budget 8 means 12 round trips in one tick period, then idle. It is an average rate, not a per-tick ceiling, and the README says so now. I did not add the batch get-by-names, that is gateway transport.
df83df1 to
66f0d51
Compare
Raises GRAPH_PARAM_DRIFT when a node parameter no longer matches its reference. The reference is either self-captured, the value the parameter had when the node armed, or pinned in config with expect. Only the pinned form catches a value that was already wrong at startup, because self-capture treats whatever it first sees as correct. The detector reports that a value moved and does not grade the consequence. Whether the new value breaks the robot is a question about the machine, not about the graph, so it raises at WARN. Unlike the detectors shipped so far, this one makes real service calls to other nodes instead of reading the local graph cache, so the sweep is bounded by max_reads_per_tick and walks the graph round-robin. That bounds the load on watched nodes and pays with coverage latency, in both directions: a drift is invisible until its node's turn comes, and so is a repair. The reads run on a thread the plugin owns, never on the gateway executor, so a node that stops answering stalls only this sweep. Also adds the config-plumbing e2e, three real gateway launches that prove nested per-detector config reaches a detector at all. Every C++ test calls configure() directly with hand-built JSON, so nothing else in the package covers the delivery path. The three launches cover the nested expect reader, mode: "off" as a string, and the bare mode: off that the ROS parser types as a YAML 1.1 boolean, which is the form operators write. Restores the healing-config section the README referred to twice without carrying, since it belongs with the first detector whose clear story depends on it. Raise the Pixi job timeout from 45 to 60 minutes. Its build step alone is about 32 minutes and the job has been finishing at 37-40 minutes for weeks, so it had no room left for a package that grows.
66f0d51 to
68a273b
Compare
Summary
Adds the
param_driftdetector. It raisesGRAPH_PARAM_DRIFTwhen a node parameter no longer matches its reference, either the value self-captured when the node armed or a value pinned withexpect.Unlike the two detectors already here, this one makes service calls into other nodes instead of reading the local graph cache. The sweep is therefore bounded by
max_reads_per_tickand walks the graph round-robin, which bounds the load on watched nodes and pays with coverage latency in both directions: a drift is invisible until its node's turn comes, and so is a repair. The reads run on a thread the plugin owns, never on the gateway executor.It also brings the config-plumbing e2e, which is the first proof in this package that nested per-detector config reaches a detector at all.
The README referred twice to a "Closing the loop" section on healing configuration that was never carried over. It belongs with the first detector whose clear story depends on it, so it is restored here.
Issue
Type
Testing
26/26 green locally on Jazzy.
test_param_drift_policy: the pure comparison and config rules, glob matching, thebaselineandexpectcombinations.test_param_drift_integration: real nodes with real parameters over the real parameter service against a fakeReportFaultservice. Covers the round-robin budget, the grace-based prune when a node vanishes, the clear path, and a node that never answers, which must not hang the tick.test_config_plumbing_e2e: three real gateway launches. One proves the nestedexpectreader, onemode: "off"as a string, one the baremode: offthat the ROS parser types as a YAML 1.1 boolean, which is the form operators actually write.Checklist