Add gathered state for ethernet access, trunk-host, and fabric update group modules by deekpand-cisco · Pull Request #507 · CiscoDevNet/ansible-nd

deekpand-cisco · 2026-08-12T15:31:53Z

Related Issue(s)

Depends on PR #391 (gathered state framework + loopback/local_user filtering).
This PR is stacked on top of PR #391

Proposed Changes

Extends gathered state and Lucene filtering to additional modules:

nd_interface_ethernet_access
nd_interface_ethernet_trunk_host
nd_fabric_update_group

Changes include:

Model gathered_spec and gathered_transform definitions
Orchestrator gathered workflow with pagination in base_interface, ethernet_base, and fabric_update_group
gathered added to state choices with documentation and examples
Unit tests for gathered filtering round-trip
Integration test tasks for gathered state

Test Notes

All 4093 unit tests passing (full repo-wide suite)
Integration tested against live ND (ethernet_access, ethernet_trunk_host, fabric_update_group gathered states)

Cisco Nexus Dashboard Version

4.2.1.10

Related ND API Resource Category

Checklist

Latest commit is rebased from develop with merge conflicts resolved
New or updates to documentation has been made accordingly
Assigned the proper reviewers

allenrobel

Code review

The gathered-state extension looks solid overall — I traced the Lucene value escaping, per-switch fan-out, pagination, legacy-vs-lucene mutual exclusion, and identifier dedup and found no logic bugs. The inline comments below are consolidation/convention items plus one behavioral question on trunk-host gathered scope. One item couldn't be anchored inline: query_all's docstring in ethernet_base.py (

ansible-nd/plugins/module_utils/orchestrators/ethernet_base.py

Lines 675 to 677 in cc4367d

    
                   The set of switches queried is determined by `_switches_to_query`: fabric-wide for `state: overridden`, 
        
                   and limited to switches named in the user config for all other states.

) shares the stale "fabric-wide for state: overridden" claim raised on _switches_to_query.

🤖 Generated with Claude Code

allenrobel · 2026-08-13T17:35:30Z

+    def _switches_to_query(self) -> dict[str, str]:
+        """
+        # Summary
+
+        Return the `{switch_ip: switch_id}` subset that `query_all` should scan.
+
+        For `state: overridden` the scope is fabric-wide, so the full switch map is returned. For every other state
+        the state machine only consults existing interfaces identified by `switch_ip` values present in the user
+        config, so only those switches are returned. This keeps the interface-list request count proportional to
+        config size rather than fabric size.
+
+        ## Raises
+
+        ### RuntimeError
+
+        - Via `FabricContext.switch_map` if the switches API query fails.
+        """
+        switch_map = self.fabric_context.switch_map
+        if self.rest_send.params.get("state") in ("overridden", "gathered"):
+            return switch_map
+        config_items = self.rest_send.params.get("config") or []
+        config_ips = {item.get("switch_ip") for item in config_items if item.get("switch_ip")}
+        return {ip: sid for ip, sid in switch_map.items() if ip in config_ips}


This override reintroduces the _switches_to_query duplication that PR #390 deliberately hoisted into NDBaseInterfaceOrchestrator — it is near-identical to the base-class version except for adding "gathered" to the state check. The gathered branch also appears unreachable: both ethernet orchestrators set gathered_lucene_spec, so query_all short-circuits to _query_all_for_gathered() (which reads fabric_context.switch_map directly) before _switches_to_query is ever consulted for that state. If the gathered condition is genuinely needed, it belongs in the base-class method; otherwise this override can be dropped. If kept, the docstring still says only state: overridden is fabric-wide.

Thanks Allen , this makes sense. I have dropped the override entirely and updated the query_all docstring to mention the gathered path.

allenrobel · 2026-08-13T17:35:30Z

+        return keys
+
+    @classmethod
+    def collect_secret_values(cls, config_item: Dict[str, Any]) -> Set[str]:


New code should use modern (PEP 585/604) annotations per team convention: dict[str, Any] / set[str] rather than Dict / Set.

Changed the declaration as per suggested.

allenrobel · 2026-08-13T17:35:30Z

        self._extra: Dict[str, Any] = {}
+        # Argument-spec ``config.options`` mapping used to prune gathered output
+        # down to valid module arguments so it round-trips as ``config``.
+        self._gathered_spec: Dict[str, Any] = {}


Modern annotations for new code: dict[str, Any] here, and dict[str, Any] | None for the new gathered_spec parameter in assign() (L136), rather than Dict / Optional[Dict[...]].

Chnages with self._gathered_spec as dict[str, Any] now, and gathered_spec param in assign() uses dict[str, Any] | None.

allenrobel · 2026-08-13T17:35:30Z

+
+from __future__ import annotations
+
+from typing import ClassVar, List, Optional


New file: prefer modern annotations — str | None, ClassVar[list[str] | None] — and drop the List / Optional imports (the file already has from __future__ import annotations).

Cleaned up — dropped List/Optional imports, using ClassVar[list[str] | None] and str | None throughout.

allenrobel · 2026-08-13T17:35:30Z

+from __future__ import absolute_import, annotations, division, print_function
+
+__metaclass__ = type  # pylint: disable=invalid-name


New file: only from __future__ import annotations is needed — absolute_import / division / print_function and __metaclass__ = type are Python 2 boilerplate we no longer add in new code.

Dropped these imports.

allenrobel · 2026-08-13T17:35:30Z

+def build_lucene_expressions(
+    filters: list[dict[str, Any]],
+    spec: GatheredLuceneSpec,
+) -> list[str]:
+    """Build one server-side AND expression per gathered config item.
+
+    Separate config items intentionally remain separate expressions. Callers
+    can union and deduplicate their responses to preserve gathered-state OR
+    semantics without relying on endpoint-specific Lucene OR support.
+    """


The one-expression-per-config-item design here is the workaround for the interface list endpoint's Lucene OR bug (OR returns empty instead of erroring). Per the team's workaround-marker convention it should carry # TODO(4.2.1) interface-lucene-or-silently-empty (the bug-tracker note for this discrepancy) so the workaround can be audited once ND fixes the endpoint. Each workaround site gets its own marker — see also the sibling sites in base_interface.py and loopback_interface.py.

I have added # TODO(4.2.1) interface-lucene-or-silently-empty right before the loop in build_lucene_expressions. This way it's easy to grep for when ND fixes the OR behavior.

allenrobel · 2026-08-13T17:35:30Z

+
+        Uses the orchestrator's ``gathered_lucene_spec`` ClassVar to map Ansible filter fields to Lucene
+        fields names. Each filter item produces a separate expression (the interface endpoint does not
+        reliably support Lucene OR). If a filter item includes ``switch_ip``, only that switch is targeted;


Second Lucene-OR workaround site — should also carry # TODO(4.2.1) interface-lucene-or-silently-empty per the workaround-marker convention.

I have added the same marker as the first line inside _build_gathered_query_plan body. Thank you!!

allenrobel · 2026-08-13T17:35:30Z

+        Build one or more Lucene expressions for each target switch.
+
+        Multiple filter items remain separate expressions because the
+        interface endpoint does not reliably support Lucene OR.


Third Lucene-OR workaround site — should also carry # TODO(4.2.1) interface-lucene-or-silently-empty per the workaround-marker convention (moot if this copy is consolidated into the base class as suggested below).

This one function is removed now — loopback's _build_gathered_query_plan was deleted as part of the consolidation. The marker lives in the single base-class copy.

allenrobel · 2026-08-13T17:35:30Z

+
+        return policy_type == "loopback"
+
+    def _build_gathered_query_plan(self, gathered_filters: list[dict]) -> dict[str, tuple[str, set[str]]]:


_build_gathered_query_plan / _query_interfaces_with_lucene / _MAX_EXPRESSIONS_PER_SWITCH here are near-byte-identical to the versions this PR adds to NDBaseInterfaceOrchestrator — the only functional difference is loopback setting endpoint_params.config_only = False. Two independently maintained copies of the pagination/fan-out logic will drift (any future fix to one is easily missed in the other). Suggest the same consolidation PR #390 did for _switches_to_query: a small hook on the base class for the config_only divergence, and delete loopback's local copies in favor of the inherited implementation.

I do agree with this, thank you for pointing it out.
I have deleted all three duplicated members from LoopbackInterfaceOrchestrator — it now inherits them from NDBaseInterfaceOrchestrator. The only real difference was loopback needing endpoint_params.config_only = False, so I added a _configure_lucene_endpoint hook on the base class (no-op by default) that loopback overrides with a one-liner. Also cleaned up the unused build_lucene_expressions import.

allenrobel · 2026-08-13T17:35:30Z

+        if self.rest_send.params.get("state") == "gathered":
+            return result
        return [iface for iface in result if not self._is_unconfigured_default(iface)]


This early return skips _is_unconfigured_default, so an unscoped state: gathered run returns every factory-default trunkHost interface on the queried switches (ND echoes policyType: trunkHost for ports the user never configured). That contradicts the module DOCUMENTATION's promise that gathered reads "user-managed trunkHost interfaces", and diverges from loopback, where the managed-only filter applies to both the management and gathered paths. If returning raw defaults for gathered is intentional, the docs and this method's docstring should say so and a unit test should pin the behavior; otherwise the filter should apply on the gathered path too.

Hey Allen, Thank you for pointing this out. It was miss from my part. I believe it would be better to return only managed trunk hosts, and not all factory default interfaces. I have removed the early-return so _is_unconfigured_default now runs on all paths including gathered.

- Add TODO(4.2.1) interface-lucene-or-silently-empty workaround markers - Remove unreachable _switches_to_query override from ethernet_base - Update query_all docstring to cover gathered path - Consolidate loopback duplicated pagination/query-plan into base class via _configure_lucene_endpoint hook - Remove gathered early-return in trunk-host so _is_unconfigured_default filters on all states - Use modern annotations (set[str], dict[str, Any]) in new code - Remove Python 2 boilerplate from new test file - Fix trailing whitespace

Add gathered_all state to the state machine for modules that retrieve all instances, mask_secrets helper for sensitive field redaction, and associated unit tests. Note: gathered-all framework cherry-picked from PR CiscoDevNet#312

Add server-side Lucene candidate filtering for gathered loopback interfaces with pagination, deduplication, and final local matching. Support gather-all and login-ID filtering for local users while rejecting unsupported gathered criteria. Add unit and integration coverage for filtering, validation, secrecy, and reusable output.

- Remove early return in filter_gathered_response() so deduplication runs unconditionally regardless of whether filters are provided - Add validate_gathered_filters() pre-flight check called before query_all() to reject invalid filters without wasted API calls

- Add gathered_filter_properties ClassVar to NDBaseModel for declarative filter whitelisting per module - Add pre-flight property validation (_extract_active_leaf_paths, _reject_unsupported_filter_properties) that rejects unsupported filter fields before any API call - Replace local_user custom normalize_gathered_filter validation with shared gathered_filter_properties tuple (login_id, email, first_name, last_name) - Add gathered_filter_properties to loopback model (switch_ip, interface_name, admin_state, ip, ipv6, vrf) - Pass supported_properties from model to validate_gathered_filters in state machine - Add gathered_transform support in NDOutput for modules with input/output shape differences - Fix loopback pagination: add max_pages safety cap, handle missing or invalid remaining metadata gracefully - Update nd_local_user DOCUMENTATION with supported filter properties - Update local_user unit tests to use shared validation path

- Apply black formatting to all gathered filtering module and test files - Add gathered state as no-op pass in manage_state() so modules can call manage_state unconditionally without raising InvalidState - Address review formatting feedback (trailing whitespace, blank lines)

- Extract gathered query logic into _query_existing() private helper - Move user-input validation outside try block for clean error messages - Build proposed before querying ND (fail-fast on bad config) - Declare get_argument_spec on NDBaseModel and gathered_transform on NDBaseOrchestrator; remove getattr probes for discoverability - Rename supports_gathered_lucene_filtering to supports_gathered_server_filtering (mechanism-neutral) - Normalize filters once in state machine; pass normalize_filter=None downstream to eliminate triple normalization - Return models from filter_gathered_response to avoid double Pydantic validation; use pre-built models for NDConfigCollection construction - Cap Lucene query fan-out at 3 expressions per switch; collapse to base expression beyond threshold - Raise ValueError for unknown switch_ip in gathered filters instead of silently returning empty results - Raise RuntimeError on pagination limit exhaustion instead of silently truncating gathered results - Update unit tests for new return types, renamed flags, and pre-normalized filter inputs

… group modules Enable gathered state and Lucene filtering for: - nd_interface_ethernet_access - nd_interface_ethernet_trunk_host - nd_fabric_update_group Includes: - Model gathered_spec and gathered_transform definitions - Orchestrator gathered workflow with pagination - Integration test tasks for gathered state - Unit tests for gathered filtering round-trip

- Add TODO(4.2.1) interface-lucene-or-silently-empty workaround markers - Remove unreachable _switches_to_query override from ethernet_base - Update query_all docstring to cover gathered path - Consolidate loopback duplicated pagination/query-plan into base class via _configure_lucene_endpoint hook - Remove gathered early-return in trunk-host so _is_unconfigured_default filters on all states - Use modern annotations (set[str], dict[str, Any]) in new code - Remove Python 2 boilerplate from new test file - Fix trailing whitespace

nikhilsrikrishna · 2026-08-27T07:45:32Z

state: gathered currently appears to run the same fabric prerequisite validation as mutation states.

The gathered path initializes the state machine and calls the interface orchestrator's query_all(). Both the loopback and shared Ethernet query paths call validate_prerequisites(), which delegates to FabricContext.validate_for_mutation(). That validation rejects a fabric when deployment freeze is enabled.

As a result, read-only gathering fails on a deployment-frozen fabric, even though no configuration is created, updated, deleted, saved, or deployed. This currently affects loopback, Ethernet access, and Ethernet trunk-host gathered operations, and the same behavior would be inherited by future interface modules using this base path.

Could we separate read prerequisites from mutation prerequisites? For example:

Add FabricContext.validate_for_read() to validate that the fabric exists and is accessible through the targeted controller.
Have validate_for_mutation() call validate_for_read() and then additionally enforce the deployment-freeze restriction.
Use validate_for_read() when state: gathered.
Continue using validate_for_mutation() for merged, replaced, overridden, and deleted.

The state machine could pass the operation intent explicitly to the orchestrator, or the base interface orchestrator could select the appropriate validation based on the current state.

This would preserve the existing mutation protection while allowing gathered state to inspect configuration during deployment freeze, which seems consistent with its read-only contract.

deekpand-cisco self-assigned this Aug 12, 2026

deekpand-cisco force-pushed the feature/gathered-ethernet-fabric-update-group branch 2 times, most recently from 4a4c967 to cc4367d Compare August 12, 2026 16:10

allenrobel reviewed Aug 13, 2026

View reviewed changes

deekpand-cisco force-pushed the feature/gathered-ethernet-fabric-update-group branch 2 times, most recently from 8ab0d7e to 2398cc8 Compare August 14, 2026 11:30

deekpand-cisco added 10 commits August 19, 2026 09:05

Fix implicit-str-concat pylint errors

ccec148

removed gathered_filtering changelog fragment

f9153a0

deekpand-cisco force-pushed the feature/gathered-ethernet-fabric-update-group branch from 2398cc8 to 24e1d39 Compare August 19, 2026 03:48


	The set of switches queried is determined by `_switches_to_query`: fabric-wide for `state: overridden`,
	and limited to switches named in the user config for all other states.


		from __future__ import annotations

		from typing import ClassVar, List, Optional

		from __future__ import absolute_import, annotations, division, print_function

		__metaclass__ = type # pylint: disable=invalid-name


		return policy_type == "loopback"

		def _build_gathered_query_plan(self, gathered_filters: list[dict]) -> dict[str, tuple[str, set[str]]]:

Uh oh!

Conversation

deekpand-cisco commented Aug 12, 2026

Related Issue(s)

Proposed Changes

Test Notes

Cisco Nexus Dashboard Version

Related ND API Resource Category

Checklist

Uh oh!

allenrobel left a comment

Choose a reason for hiding this comment

Code review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deekpand-cisco Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikhilsrikrishna commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deekpand-cisco Aug 14, 2026 •

edited

Loading