Consolidation of Genie parsing logic to Connection Broker

[seojumper]

Description

PR consolidates genir parsing logic to ConnectionBroker with storage of parsed output retained in CommandCache.

Related Issue(s)

This is related to #663 where previously parsing of previously fetched command output that already existed in cache was not working correctly based on discovered logic in Genie Parsers.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactoring / Technical debt (internal improvements with no user-facing changes)
• Documentation update
• Chore (build process, CI, tooling, dependencies)
• Other (please describe):

Test Framework Affected

• PyATS
• Robot Framework
• Both
• N/A (not test-framework specific)

Network as Code (NaC) Architecture Affected

• ACI (APIC)
• NDO (Nexus Dashboard Orchestrator)
• NDFC / VXLAN-EVPN (Nexus Dashboard Fabric Controller)
• Catalyst SD-WAN (SDWAN Manager / vManage)
• Catalyst Center (DNA Center)
• ISE (Identity Services Engine)
• FMC (Firepower Management Center)
• Meraki (Cloud-managed)
• NX-OS (Nexus Direct-to-Device)
• IOS-XE (Direct-to-Device)
• IOS-XR (Direct-to-Device)
• Hyperfabric
• All architectures
• N/A (architecture-agnostic)

Platform Tested

nac-test supports macOS and Linux only

• macOS (version tested: )
• Linux (distro/version tested: Unbuntu 24.04)

Key Changes

Testing Done

• Unit tests added/updated
• Integration tests performed
• Manual testing performed:
  • PyATS tests executed successfully
  • Robot Framework tests executed successfully
  • D2D/SSH tests executed successfully (if applicable)
  • HTML reports generated correctly
• All existing tests pass (pytest / pre-commit run -a)

Test Commands Used

uv run pytest -n auto --dist loadscope tests/

Checklist

• Code follows project style guidelines (pre-commit run -a passes)
• Self-review of code completed
• Code is commented where necessary (especially complex logic)
• Documentation updated (if applicable)
• No new warnings introduced
• Changes work on both macOS and Linux
• CHANGELOG.md updated (if applicable)

Additional Notes

This change is currently causing tests to directly call 'parse_output' instead of 'execute_command' > 'parse_output' which does impact visual of test results as raw command output is no longer present. Default genie api's do not return command output from a 'parse()' call. This could be changed with providing the 'return_raw=True' optional arg to the 'parse_output' fn.

[oboehmer]

Thanks for the fix, @seojumper ! Can we also fix this without having to adjust our already generated test cases?

[oboehmer]

Thanks for digging into this, Jeremy. I now reviewed the underlying issue, and I'd prefer a lighter fix that addresses the root cause directly rather than restructuring the parse flow into the broker.

Root cause: Some genie parsers call self.device.execute() internally for supplementary commands (e.g., show vrf, show run | sec address-family) to resolve VRF names. In broker mode, the testbed device in the test process isn't connected — those calls fail silently, so VRF resolution degrades and everything collapses to "default".

Proposed fix: Patch testbed_device.execute to route through the broker, so Genie's internal calls work transparently. No new RPC, no CacheEntry restructuring, no broker-side parse logic needed.

Sketch:

from functools import partial

# In _async_setup(), after broker connection is established:
if self.testbed_device:
    self._patch_device_execute_for_broker()

def _patch_device_execute_for_broker(self):
    """Patch testbed_device.execute so Genie's internal calls route through the broker."""
    broker_client = self.broker_client
    hostname = self.hostname
    loop = get_or_create_event_loop()

    def broker_execute(cmd, *args, **kwargs):
        """Sync execute that routes through connection broker.

        Must be called from a worker thread (via run_in_executor),
        not from the event loop thread, to avoid deadlock.
        """
        future = asyncio.run_coroutine_threadsafe(
            broker_client.execute_command(hostname, cmd), loop
        )
        return future.result(timeout=120)

    self.testbed_device.execute = broker_execute

parse_output becomes async and runs Genie in a worker thread (required because the patched execute blocks waiting for the broker response — safe from a thread, deadlocks on the event loop thread):

async def parse_output(
    self, command: str, output: str | None = None
) -> dict[str, Any] | None:
    """Parse command output using Genie parser.

    Runs in a worker thread so Genie's supplementary device.execute()
    calls can safely route through the broker without deadlocking.
    """
    if not self.testbed_device:
        return None
    try:
        loop = get_or_create_event_loop()
        if output is not None:
            result = await loop.run_in_executor(
                None, partial(self.testbed_device.parse, command, output=output)
            )
        else:
            result = await loop.run_in_executor(
                None, partial(self.testbed_device.parse, command)
            )
        return dict(result) if result is not None else None
    except Exception as e:
        self.logger.warning(f"Genie parser failed for '{command}': {e}")
        return None

Impact on test templates — one keyword added:

# Before:
parsed_output = self.parse_output(command, output=output)

# After:
parsed_output = await self.parse_output(command, output=output)

Why I prefer this:

• Fixes the root cause (Genie can't execute supplementary commands) rather than working around it
• No new broker RPC (parse command), no CacheEntry class, no _parse_command method
• Raw command output stays in the test flow — available for reporting without return_raw plumbing
• Broker-side CommandCache already deduplicates repeated executions from supplementary calls
• Smaller diff, less surface area for bugs
• only minimal changes required for the test cases

Happy to discuss further — the key insight is that Genie just needs a working device.execute() and it handles everything else.

can you please try out this approach, @seojumper ?

[oboehmer]

I pushed the alternative approach to https://github.com/netascode/nac-test/compare/663-genie-device-execute-support, could you test this in your environment? I also added a test which reproduces the bug, so we can also catch regressions here in our pipeline. @aitestino @seojumper

[seojumper]

I pushed the alternative approach to https://github.com/netascode/nac-test/compare/663-genie-device-execute-support, could you test this in your environment? I also added a test which reproduces the bug, so we can also catch regressions here in our pipeline. @aitestino @seojumper

yeah i can try that out tonight/tomorrow, thanks.

[aitestino]

Hey @seojumper, thank you for the PR — the intent here is correct, and moving the Genie parsing logic into a more centralized path is definitely the right direction for solving #663.

Per our internal conversation but recapping here for business continuity, it looks like Oliver (@oboehmer) has created his own branch (663-genie-device-execute-support) with an alternative implementation. I haven't seen a PR for it yet, but I've gone through both implementations in detail. After comparing the two approaches, I think Oliver's is the stronger foundation here for a few key reasons:

Why Oliver's approach fits the codebase better:

1. Cache coherence — The implementation in this PR introduces two writers to the same CommandCache (_execute_command and _parse_command) that can overwrite each other's entries. When _parse_command is called with return_raw=False, it stores output=None. A subsequent _execute_command for the same command sees output is None, treats it as a miss, re-executes on the device, and then writes a new CacheEntry that destroys the cached parsed_output. The reverse scenario also applies. This is the cache-swapping concern you flagged yourself, and it's a real bug, not just theoretical.

2. Genie supplementary commands — The core issue ([bug] Genie BGP Parser Returns Inconsistent VRF Keys Causing False Test Failures #663) is that Genie parsers internally call device.execute() for supplementary data. The approach in this PR moves connection.parse() into the broker, but Genie's supplementary execute() calls during that parse still go through the broker's direct connection — which works, but only because the broker owns the connection.

   • Oliver's approach solves this transparently at the right layer: ALL device.execute() calls (explicit test calls AND Genie's internal ones) route through the same path.

3. Backward compatibility — The change in this PR removes the output= parameter from parse_output() and changes the return type from dict | None to tuple[dict | None, str | None]. This breaks downstream consumers in nac-sdwan-terraform (verify_bgp_peers.py, verify_ospf_neighbor_state.py).

   • Oliver's approach preserves the parameter signature.

4. SRP — From a best practices standpoint, I think ideally we want to keep the broker as a thin command transport layer. Adding parsing responsibility to the broker creates coupling between the connection management layer and Genie's parser internals.

We also discussed some concerns surrounding monkey patching annnnd I hear you, and I share the general instinct that monkey patching creates behavior that's harder to reason about. It's a legitimate tradeoff. In this specific case though, it's the only approach that transparently intercepts Genie's internal execute() calls without modifying pyATS source (which we can't do).

Oliver's patching is well-isolated to one method (_patch_device_execute_for_broker), well-documented with clear docstrings explaining WHY each patch exists, and the integration tests would immediately surface any breakage if pyATS changes their dispatch mechanism. Given the alternative is a design-level cache coherence problem, I think the monkey-patch tradeoff is the pragmatic call here.

---

@oboehmer — Looking at the work in your branch, here are my findings/thoughts:

Things that likely need adjustment:

1. get_cache_stats() is not thread-safe — You correctly added threading.Lock to get(), set(), and clear(), but get_cache_stats() iterates self.cache.values() without the lock. Under concurrent access from worker threads this can raise RuntimeError: dictionary changed size during iteration. One-line fix: wrap it in with self._lock.

2. Broker-without-testbed regression — _create_execute_command_method captures device = test_instance.testbed_device and always calls device.execute. If the broker is active but no testbed file is loaded, device is None and you get an AttributeError. The old code dispatched correctly on both paths. Either guard with an explicit assert or restore the fallback.

3. Dead connection parameter — _create_execute_command_method(self, connection, command_cache) accepts connection but never uses it. The docstring says "kept for API compatibility" but it's a private method with one call site. Remove it or use it as the fallback for the device is None case (which would also fix issue relax robotframework req #2).

4. test_broker_genie_parse_2.py TITLE copy-paste — Both files say "(1)" in the TITLE constant. File 2 should say "(2)".

5. DEVICE_EXECUTE_TIMEOUT missing from __all__ — Every other constant in that file is exported. Add it for consistency.

6. # type: ignore[assignment] on hostname/command_cache — Replace with explicit assert self.hostname is not None / assert self.command_cache is not None guards. That way the precondition is enforced in code, not suppressed.

7. Unit test gap for the shim/connected-flag patching — The existing unit tests cover broker_execute routing well, but there's no test verifying that testbed_device.connected = True, connectionmgr.is_connected, and device.cli = _BrokerCliShim() are set correctly. These are the exact attributes that make Genie's parser dispatch work — worth a focused test.

8. DRY on genie parse fixtures — test_broker_genie_parse_1.py and _2.py are 216-line clones differing only in class name. I'd suggest a shared base module with the verify_item logic, and two thin subclass files. Same feedback applies to the pre-existing test_broker_pooling_1/2/3.py if you want to tackle it in the same pass.

Rest looks good to me.

Once those items are addressed, I think this is ready to go. Would you mind opening a PR for the branch so we can track it properly?

What do you both 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! 🙂

[oboehmer]

Thanks for your comprehensive review of the situation and the two approaches, @aitestino ! I share the same concern about putting more logic into the broker (and we couldn't stop here if people would leverage other genie applications like learn() in their tests), so we would end up keeping enhancing the RPC. Keeping it slim and focused on device connection and command execution is IMHO the right call and triggered me trying the alternative approach in my branch (wanted to see if it works before commenting).

To your concern about patching device.execute(): I have been following the same design approach in radkit_genie for a few years (since its inception), and ran it with the pyATS architects back then (as I was also not sure if this is solid). They voiced no concern about this, and I have not seen any issues with this approach in LazyMaestro/RADKit over the years it has been in production and heavily used. The pyATS API has proven to be very stable, and if it ever changes, we will see it early as we have e2e tests with real pyATS testbed devices exercising all relevant call paths.

I will raise a PR soon, taking your comments into account.

[oboehmer]

I raised PR #863