feat(broker): support Genie-driven command execution in broker mode#863
feat(broker): support Genie-driven command execution in broker mode#863oboehmer wants to merge 15 commits into
Conversation
…y calls Route supplementary device.execute() calls through the connection broker by patching testbed_device.execute at parse time. This fixes Genie parsers that issue additional commands (e.g. 'show running-config | section router ospf') which previously failed silently in broker mode. - Add _patch_device_execute_for_broker() using run_coroutine_threadsafe - Make parse_output() async (runs Genie in executor thread) - Update all 9 test callers to await parse_output() - Add OSPF TE parser e2e test proving the fix (supplementary execute)
Route all device commands (both explicit test calls and Genie supplementary parser calls) through testbed_device.execute, which is patched in broker mode to route through the connection broker. This eliminates the async/sync branching in execute_command and establishes a single execution path regardless of connection mode. - Simplify _create_execute_command_method: always run_in_executor - Add command caching to broker_execute patch (supplementary calls benefit from cache, avoiding duplicate round-trips) - Make CommandCache thread-safe (threading.Lock on get/set/clear) - Extract DEVICE_EXECUTE_TIMEOUT constant to pyats_core/constants.py - Add unit tests for unified execution and cache integration
… mode Enable parse_output(command) without pre-fetched output in broker mode. Genie's parser internals access device.cli.execute() to fire supplementary commands — these now route through the connection broker. Changes to _patch_device_execute_for_broker(): - Set device.connected = True (Genie checks this before parsing) - Patch connectionmgr.is_connected() to return True (Genie's connected property delegates to connection manager) - Create _BrokerCliShim class with execute() delegating to broker_execute, assigned to device.cli (Genie accesses device.cli.execute() directly for command execution) This enables two usage patterns for parse_output(): 1. parse_output(cmd, output=output) - parse pre-fetched output (existing) 2. parse_output(cmd) - let Genie execute via the broker (new)
…broker
Add test coverage proving that Genie's internal device.execute() calls
route through the connection broker and benefit from command caching
when parse_output() is called without pre-fetched output.
New test fixture files:
- test_broker_genie_parse_1.py: Calls parse_output('show ip ospf mpls
traffic-eng link') WITHOUT pre-fetched output. Genie internally
executes both the primary command and a supplementary
'show running-config | section router ospf 1' (to resolve VRF names).
Both commands route through broker_execute via _BrokerCliShim.
- test_broker_genie_parse_2.py: Identical test — on second execution,
both commands should hit the broker's server-side command cache.
Updated test_broker.py:
- Refactored _validate_broker_statistics() to accept explicit expected
values instead of computing from a simple formula (the old formula
assumed 1 command per test file, which breaks with Genie parsers that
fire supplementary commands).
- Updated test_connection_broker_pooling_and_caching() to validate the
expanded fixture set: 5 test files (3 sdwan + 2 genie-driven) on 2
devices, expecting command_misses=6 and command_hits=8.
Updated e2e fixture:
- verify_iosxe_no_critical_errors_in_system_logs.py now uses the
Genie-driven pattern: calls parse_output(command) without pre-fetched
output, letting Genie execute 'show logging' via the broker.
Remove if/else branching and functools.partial import. Genie treats output=None identically to omitting the kwarg (both trigger device execution), so a single lambda covers both code paths. Assign testbed_device to local variable for mypy narrowing (instance attribute cannot be narrowed inside a lambda closure). Aligns with codebase convention: all other run_in_executor calls use positional args or lambdas, none use partial().
…tion fixtures from pytest collection
…arameter - Enforce testbed_device presence at the top of _async_setup() with a clear ConnectionError instead of half-supporting broker-without-testbed (which crashes downstream anyway). - Remove dead 'connection' parameter from _create_execute_command_method() — it was unused after the unified testbed_device.execute refactor. - Make _patch_device_execute_for_broker() unconditional in broker path (invariant guarantees testbed_device is present). - self.connection attribute retained for lifecycle/cleanup branching. - Update unit tests: add invalid-socket+no-testbed case, adjust broker test to provide mock testbed device.
…ly_broker_patch consistently All 7 tests in the class now use the shared helper instead of 4 doing inline setup and 3 using the helper. Reduces repetition (~50 lines) while keeping test intent clear and self-contained.
oboehmer
added
bug
aitestino
left a comment
aitestino
left a comment
There was a problem hiding this comment.
Hey @oboehmer, thank you for the PR — the broker architecture is solid and I really like the unified execution path eliminating mode-specific branching. The _BrokerCliShim approach to make Genie's supplementary device.execute() calls work transparently is clever.
I did a couple of reviews on this, and I think we need some changes before merge. The main themes are test coverage gaps for the new thread-safe components and a few structural items.
Things that need adjustment:
-
Thread-safety test coverage for
CommandCache—CommandCacheusesthreading.Lockfor all its operations, which is the right call. But there are zero concurrency tests exercising the lock under contention. From a best practices standpoint, I think we want at least one test that hammersget/setfrom multiple threads concurrently to prove the lock actually protects against data races. The lock is load-bearing here since multiple Genie parser threads could hit the cache simultaneously. -
Post-context assertions in
_apply_broker_patchtests — Intest_patched_execute_calls_broker, the helper'sExitStackexits before the outside assertions run. The secondmock_device.execute("show version")at line 305 hits the cache (populated duringcall_after_patch), not the broker path. The test passes but for the wrong reason. Themock_future.result.assert_called_with(...)at line 306 is valid (verifies what happened inside the context), but line 305 is testing cache behavior, not broker routing. Cleaner approach: use thecall_resultreturn value from_apply_broker_patchdirectly:mock_device, result = self._apply_broker_patch( ssh_instance, extra_patches=[patch(self._RUN_CORO_PATH, return_value=mock_future)], call_after_patch=lambda dev: dev.execute("show version"), ) assert result == "command output" mock_future.result.assert_called_with(timeout=DEVICE_EXECUTE_TIMEOUT)
-
Test file placement —
test_ssh_base_test_validation.pylives attests/unit/root. Looking at the existing structure, tests fornac_test/pyats_core/common/live intests/unit/pyats_core/common/(e.g.,test_base_test_defaults.py,test_defaults_resolver.py). This file should move totests/unit/pyats_core/common/test_ssh_base_test.pyto follow the mirror convention. -
Near-identical fixture templates —
test_broker_genie_parse_1.pyandtest_broker_genie_parse_2.pyintests/integration/fixtures/templates_broker/tests/are identical except for the TITLE suffix(1)/(2)and the class name. All module constants (DESCRIPTION,SETUP,PROCEDURE,PASS_FAIL_CRITERIA,TEST_CONFIG) and the entireverify_itemmethod are duplicated verbatim. I understand these test caching behavior (file 1 = misses, file 2 = hits), but the DRY violation is significant. Consider extracting a shared base class or a parametrized template that only varies the title/class name. -
DEVICE_EXECUTE_TIMEOUTshould be env-configurable — This constant is hardcoded as120inconstants.py, but the same file usesget_positive_numeric_env()forPYATS_OUTPUT_BUFFER_LIMIT,PIPE_DRAIN_DELAY_SECONDS,BATCH_SIZE, etc. A 120-second timeout for broker-routed commands is a reasonable default, but some environments (slow WAN links, large outputs) may need to tune it. Suggested approach:DEVICE_EXECUTE_TIMEOUT: int = get_positive_numeric_env( "NAC_TEST_DEVICE_EXECUTE_TIMEOUT", 120, int )
What do you think?
P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂
Item 1 — CommandCache concurrency tests
Add tests/unit/pyats_core/common/test_command_cache.py with a dedicated
TestCommandCacheConcurrency class containing two tests:
- test_concurrent_get_set_no_exceptions: 10 writer + 10 reader threads
(100 iterations each, 5 commands) verify no exceptions are raised and
get() never returns a partially-constructed value.
- test_concurrent_get_cache_stats_no_exceptions: continuous writer vs
repeated get_cache_stats() calls verify no exceptions and that
valid + expired == total always holds.
Also adds TestCommandCacheBasic covering get/set/clear/TTL/stats
correctness (6 tests).
Class docstring documents the CPython GIL caveat: due to the GIL, simple
dict mutations are effectively atomic, so these tests will not fail if the
lock is removed under CPython. The tests are kept as smoke tests and for
forward compatibility with free-threaded Python (PEP 703 / Python 3.13+),
where the lock is load-bearing.
Item 2 — Fix post-context assertion in TestPatchDeviceExecuteForBroker
In test_patched_execute_calls_broker (and the analogous
test_sets_cli_shim_with_broker_execute), the original code called
mock_device.execute() / mock_device.cli.execute() *after* the ExitStack
exited. Because call_after_patch had already run the same command inside
the context, the post-context call hit the CommandCache rather than the
broker path — the test passed for the wrong reason.
Fix: capture the return value of call_after_patch as 'result' (the second
element _apply_broker_patch already returns) and assert against that
instead of making a fresh post-context call. The broker path is now
exercised and verified entirely within the ExitStack lifetime.
Item 3 — Move test file to mirror source layout convention
Rename tests/unit/test_ssh_base_test_validation.py to
tests/unit/pyats_core/common/test_ssh_base_test.py to mirror the
nac_test/pyats_core/common/ source layout (consistent with e.g.
test_base_test_defaults.py, test_defaults_resolver.py in the same dir).
All fixtures (ssh_instance, iosxe_controller_env, socket_dir) are
inherited from parent conftest.py files and work unchanged.
Item 5 — DEVICE_EXECUTE_TIMEOUT env-configurable
Replace the hardcoded constant:
DEVICE_EXECUTE_TIMEOUT: int = 120
with:
DEVICE_EXECUTE_TIMEOUT: int = get_positive_numeric_env(
'NAC_TEST_DEVICE_EXECUTE_TIMEOUT', 120, int)
Consistent with the pattern used for PYATS_OUTPUT_BUFFER_LIMIT,
PIPE_DRAIN_DELAY_SECONDS, BATCH_SIZE, etc. in the same file. Allows
environments with slow WAN links or large command outputs to tune the
timeout without a code change.
|
Ciao @aitestino .. thanks for the review, I addressed all (via a73f6a2) but item 4, which I would still push back on: The test fixture files (test_broker_genie_parse_1.py and test_broker_genie_parse_2.py) are processed in a full nac-test integration test run, and thus are subject to our test type discovery which AST-parses the files and looks for classes, aetest decorators and the like. Subclassing will break this, and I feel integration and e2e tests should process "real" test files. I acknowledge that there is a lot of duplication of test case fixtures in tests/integration and tests/e2e, but I feel this is acceptable and meets the test objective of processing "real" test cases. please re-review. |
2 participants
Description
Enable Genie-driven command execution through the connection broker. The broker remains a dumb SSH multiplexer — Genie runs client-side, with supplementary commands transparently routed through the broker via IPC.
Problem: Some Genie parsers internally call
device.execute()for supplementary commands (e.g.show running-config | section router ospf 1for VRF resolution). In broker mode, the test client's testbed device has no live SSH session — only the broker server holds connections. Without intervention, supplementary commands fail.Solution: Patch the client-side testbed device so it appears fully connected to Genie. Any command Genie fires transparently routes through the broker via IPC:
This keeps the broker as a dumb execution pipe (no Genie dependency), preserves test writer flexibility (
device.parse()works transparently in both modes), and requires no test template signature changes beyond addingawait.Closes
Related Issue(s)
Type of Change
Test Framework Affected
Network as Code (NaC) Architecture Affected
Platform Tested
Key Changes
Core feature:
_patch_device_execute_for_broker(): patchesdevice.connected,connectionmgr.is_connected, anddevice.cliwith_BrokerCliShimthat delegates to brokerparse_output()made async — runs Genie in worker thread viarun_in_executorso supplementary broker calls don't deadlock the event looptestbed_device.executein both broker and direct mode (eliminates mode-specific branching)Code quality (from @aitestino PR #835 review):
CommandCache.get_cache_stats(): made thread-safe (snapshot under lock)# type: ignorewithcast()for type narrowing (with motivating comments)DEVICE_EXECUTE_TIMEOUTtoconstants.__all__connected,connectionmgr.is_connected,clishim)test_broker_genie_parse_2.pytests/integration/fixturesfrom pytest collectionCleanup (addressing dead code from unified execution refactor):
connectionparameter from_create_execute_command_method()(was unused after thetestbed_device.executeunification)_async_setup()— fail fast with clear error instead of half-supporting an impossible code pathif self.testbed_device:guard around_patch_device_execute_for_broker()— now unconditional since invariant is enforcedTestPatchDeviceExecuteForBroker— all 7 tests now use the shared_apply_broker_patch()helper consistentlyTesting Done
pytest/pre-commit run -a)Test Commands Used
Checklist
pre-commit run -apasses)Additional Notes
Why this approach over the one from PR #835
PR #835 moves Genie parsing into the broker, coupling the broker to Genie and requiring a new RPC per parser method. This PR instead patches the client-side device, keeping the broker as a dumb SSH pipe while making
device.parse(),device.execute(), anddevice.learn()work transparently in both modes without signature changes.Response to @aitestino's review findings
get_cache_stats()not thread-safeAttributeError_async_setup(). Both broker and direct mode require a testbed device; the method now fails fast with a clearConnectionErrorinstead of silently deferring the crash to first command execution.connectionparameter_create_execute_command_method(). It became dead after the unifiedtestbed_device.executerefactor.self.connectionattribute is retained for lifecycle management (broker cleanup branching).genie_parse_2DEVICE_EXECUTE_TIMEOUTmissing from__all__# type: ignorewithassert ... is not Nonecast()instead. Note: recommendingassertfor type narrowing contradicts @aitestino's own issue #679 where she stated "using assert for type narrowing in production code is an anti-pattern".cast()has no runtime cost and is the established pattern in this codebase.