Detect field removals in replaced/overridden states (one-way subset diff)

[allenrobel]

Related Issue(s)

Fixes #410

Merge order

• This PR
• nd_interface_loopback: align deploy control to config_actions.deploy #366
• nd_interface_loopback: policy_type discriminated union #403
• nd_fabric_prepare_update - ND 4.2 software staging #291
• Future PR to fix issue [ND 4.2+] Add L3 routed ethernet interface support (routedHost) via the ethernet discriminated-union module #447

Proposed Changes

• NDBaseModel.get_diff gains a reverse pass on the exclude_unset=False (replaced/overridden) path: after the forward subset check, the new pure util utils.has_removals() walks the existing side's payload-scoped dump (exclude_from_diff | payload_exclude_fields) and classifies any non-empty field absent from proposed as a difference, so the full-payload PUT that resets omitted fields is actually issued. Merged-state semantics are unchanged.
• Empty existing-side values ("", [], {}) normalize to absent, generalizing the PrefixListModel description precedent; its ad-hoc get_diff override is removed (its tests pass against the inherited behavior).
• Default-echo normalization: ND echoes the OpenAPI template default for every field the user never set (lab-verified: ~22 concrete fields on trunkHost; ND injects routeMapTag: 12345 even on user-created loopbacks). New per-model reverse_diff_defaults ClassVars (alias → template default, sourced from the ND 4.2.1 OpenAPI template schemas, values in the model's dumped form) let to_reverse_diff_dict() strip default-valued echoes recursively, keeping replaced/overridden runs idempotent. Tables added for all nine interface policy models (loopback, ethernet access/trunkHost, port-channel access/trunkHost, vPC access/trunkHost, SVI, managed subinterface).
• Known residual: TrunkVpcHostPolicyModel — the schema defaults for peer1AllowedVlans/peer2AllowedVlans have no model fields (per-peer→collapsed access_vlan workaround); vPC families were not lab-verified in this PR.

Post-review hardening (three follow-up commits addressing all five findings from Claude's pre-merge code review):

• Commit "Fix reverse-pass idempotency for server-populated existing-side data" — three confirmed bugs, one mechanism: the reverse pass fired on existing-side data the proposed config can never express, permanently classifying items as changed.
  • New alias-keyed reverse_diff_exclude ClassVar, applied at each nested model's own level during the reverse scrub; both vPC policy models exclude the orchestrator-injected peerSwitchId (not in the argspec, injected only at payload-build time, echoed by ND on reads).
  • The reverse scrub drops extra="allow" server keys (model_extra) at every nesting level, so undeclared ND GET keys on the fabric models (top-level or nested management) never count as removals.
  • LocalUserModel gains a reverse_diff_defaults table for ND's falsy echoes (xLaunch=false, reuseLimitation=0, timeIntervalLimitation=0 — the module's own integration tests assert these before values); False/0 are real values, not empty markers, so empty-normalization alone could not cover them.
• Commit "Add TODO(4.2.1) workaround markers to all reverse_diff_defaults tables" — all ten table sites (nine interface policy models + LocalUserModel) carry TODO(4.2.1) get-echoes-schema-defaults-for-unset-fields, backed by a new bug-tracker vault note of the same id (resolvable via get_bug_by_id) documenting the defaults-echo behavior across both the interfaces and localUsers endpoint families, so /nd-workaround-audit can surface the tables for re-verification against future ND releases.
• Commit "Derive reverse-pass dicts from the forward dumps (halve get_diff cost)" — to_reverse_diff_dict now derives from to_diff_dict plus in-place scoping, and get_diff reuses its already-computed forward dumps: 2 dumps per no-diff comparison instead of 4, pinned by a dump-count regression test. Side benefit: subclass to_diff_dict overrides (e.g. the AI-eBGP nxapiHttp pop) now scope the reverse pass symmetrically. NDOutput's two-directional changed loop is deliberately unchanged — payload-excluded-but-diff-compared fields (loopback switch_ip, SVI/subinterface oper_data, prefix-list ip_version) rely on the second direction, so removing it would be a semantics change, not an optimization (documented in the commit body).

Test Notes

• New unit tests: tests/unit/module_utils/test_utils.py (first coverage for issubset + has_removals), tests/unit/module_utils/models/test_base_model_reverse_diff.py (removal detection, empty/default normalization, per-family default tables incl. the verbatim lab-captured trunkHost echo), state-machine replaced/overridden/merged classification tests, and the previously missing exclude_unset=False storm-control case.
• test_loopback_interface_00620 updated: proposed-with-fewer-fields is now a difference by default (the old assertion documented the replaced/overridden states cannot detect field removals (one-way subset diff) #410 bug).
• Post-review commits add a 005xx test section (per-bug phantom-removal reproductions built TDD-first, each paired with a genuine-removal guard proving the fix does not over-strip) and a 006xx efficiency section (dump-count contract: exactly one model_dump per side on the no-diff replaced path, plus standalone to_reverse_diff_dict behavior parity).
• Full unit suite passes: 3865 tests via ndpytest tests/unit/ (nd-dev container machine).
• Review re-verification: the pre-merge code review's five findings (three confirmed idempotency bugs, the missing workaround markers, the redundant reverse dumps) were re-verified fixed at branch HEAD by re-running the original reproduction scripts — phantom changes now classify no_diff, genuine removals still classify changed — with no new findings.
• black/isort/pylint/mypy clean on changed files; ndtest sanity passes except the pre-existing action-plugin-docs findings on plugins/action/tests/integration/* (untouched by this PR).
• Lab verification (SITE1): the issue's repro passes — state: replaced omitting storm_control_broadcast_level: 80.0 reports changed: true, issues the PUT, and clears the value on ND; replaced double-runs on nd_interface_ethernet_trunk_host and nd_interface_loopback report changed: false on the second run.

Cisco Nexus Dashboard Version

4.2.1

Related ND API Resource Category

• analyze
• infra
• manage
• onemanage
• other

Checklist

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

🤖 Generated with Claude Code

https://claude.ai/code/session_01WqAuV2pWYJTno2bfdcNZCm

[Copilot]

Pull request overview

This PR fixes diff classification for replaced/overridden Gen-3 state-machine runs by detecting “removal-only” changes (fields present in existing device config but omitted from proposed config), while preserving merged semantics and idempotency in the presence of ND default/empty echoes.

Changes:

• Add a reverse-pass removal detector (utils.has_removals) and integrate it into NDBaseModel.get_diff for exclude_unset=False to correctly trigger updates on field removals.
• Normalize ND “empty marker” echoes ("", [], {}, nested-empty dicts) and strip schema-template default echoes via per-model reverse_diff_defaults tables to maintain idempotency.
• Expand/adjust unit tests to cover removal detection, merged vs replaced behavior, storm-control removal, and default-echo normalization; remove the now-unnecessary PrefixListModel get_diff override.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/unit/module_utils/test_utils.py	Adds unit coverage for issubset and new has_removals behavior, including empty normalization.
tests/unit/module_utils/test_nd_state_machine.py	Adds state-machine tests ensuring removal-only diffs update under replaced/overridden but not under merged.
tests/unit/module_utils/models/test_storm_control_mutex.py	Adds regression coverage for storm-control removal being detected on the exclude_unset=False path.
tests/unit/module_utils/models/test_loopback_interface.py	Updates expectations to reflect fixed removal detection under default (exclude_unset=False) vs merged behavior.
tests/unit/module_utils/models/test_base_model_reverse_diff.py	Adds comprehensive tests for reverse-pass removals, scoping, empty/default normalization, and per-policy default tables.
plugins/module_utils/utils.py	Introduces _is_effectively_empty and has_removals to detect removals safely for replace-style diffs.
plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py	Removes PrefixListModel’s ad-hoc get_diff override now covered by shared reverse-pass logic.
plugins/module_utils/models/interfaces/vpc_trunk_host_interface.py	Adds reverse_diff_defaults table for vPC trunk host policy default-echo normalization.
plugins/module_utils/models/interfaces/vpc_access_interface.py	Adds reverse_diff_defaults table for vPC access host policy default-echo normalization.
plugins/module_utils/models/interfaces/svi_interface.py	Adds reverse_diff_defaults table for SVI policy default-echo normalization.
plugins/module_utils/models/interfaces/subinterface_managed_interface.py	Adds reverse_diff_defaults table for managed subinterface policy default-echo normalization.
plugins/module_utils/models/interfaces/port_channel_trunk_host_interface.py	Adds reverse_diff_defaults table for port-channel trunk host policy default-echo normalization.
plugins/module_utils/models/interfaces/port_channel_access_interface.py	Adds reverse_diff_defaults table for port-channel access policy default-echo normalization.
plugins/module_utils/models/interfaces/loopback_interface.py	Adds reverse_diff_defaults table for loopback policy (incl. routeMapTag type-drift handling).
plugins/module_utils/models/interfaces/ethernet_trunk_host_interface.py	Adds reverse_diff_defaults table for ethernet trunk host policy default-echo normalization.
plugins/module_utils/models/interfaces/ethernet_access_interface.py	Adds reverse_diff_defaults table for ethernet access policy default-echo normalization.
plugins/module_utils/models/base.py	Implements reverse-pass export/default stripping and integrates has_removals into NDBaseModel.get_diff for replace-style semantics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mikewiebe]

High: Missing ptp: false default breaks port-channel idempotency

Issue

The new reverse-diff normalization omits Nexus Dashboard's normal ptp: false
echo from the port-channel trunk-host default table, so unchanged
replaced/overridden configurations are classified as changed.

Evidence

• port_channel_trunk_host_interface.py line 237
  adds reverse_diff_defaults for the controller's normal schema defaults but
  omits ptp.
• port_channel_trunk_host_interface.py line 302
  declares ptp as a user-configurable policy field, and normal Nexus
  Dashboard responses populate it as false when the user did not set it.
• An exact-head reproduction using the existing controller-response fixture
  retained {"ptp": false} in the reverse dictionary and reported a
  difference against an otherwise unchanged desired configuration.

Practical example

Assume this port-channel is already configured correctly and PTP was never
enabled:

- name: Maintain the server trunk
  cisco.nd.nd_interface_port_channel_trunk_host:
    fabric_name: FABRIC1
    state: replaced
    config:
      - switch_ip: 192.0.2.11
        interface_name: port-channel10
        config_data:
          network_os:
            policy:
              admin_state: true
              allowed_vlans: "100-110"
              ports:
                - Ethernet1/1
                - Ethernet1/2
              # ptp is intentionally omitted

Nexus Dashboard returns the existing policy with schema defaults populated:

adminState: true
allowedVlans: "100-110"
ports:
  - Ethernet1/1
  - Ethernet1/2
ptp: false

The desired and actual PTP state are effectively identical: PTP is disabled.
However, the new reverse comparison retains ptp: false because that normal
controller echo is missing from reverse_diff_defaults.

Run	Actual configuration	Module result	Controller actions
1	Already correct; PTP disabled	changed: true	PUT and deploy
2	Still correct; PTP disabled	changed: true	Same PUT and deploy
3	Still correct; PTP disabled	changed: true	Same PUT and deploy

The controller continues returning ptp: false, so every run detects the same
false difference.

Existing PR overlap

No matching existing PR comment found. PR #422 currently has no inline or
top-level conversation comments identifying this port-channel default.

Existing open issue overlap

No matching open issue found. Issue
#410 tracks the broader
one-way removal-detection defect that PR #422 implements; it does not track
this missing-default regression in the proposed fix.

Impact

An idempotent replaced or overridden task can report changed=True, issue a
redundant PUT, and deploy the same port-channel on every playbook run. Across
many port-channels, this produces unnecessary controller and switch churn and
makes change reporting unreliable.

Suggested fix

Add "ptp": False to
PortChannelTrunkHostPolicyModel.reverse_diff_defaults. Add a regression test
built from a normal controller response containing ptp: false, with desired
configuration omitting PTP, and assert that both replaced and overridden
remain unchanged and schedule no PUT or deployment.

---

[allenrobel]

Confirmed — the reverse pass retained the ND-injected ptp and misclassified unchanged port-channels as changed. Fixed in "Strip ND-injected ptp from the port-channel reverse pass", but via reverse_diff_exclude rather than the defaults table, for two reasons: (1) intPortChannelTrunkHostTemplate declares no ptp property, and the reverse_diff_defaults tables are schema-sourced (slug get-echoes-schema-defaults-for-unset-fields) — this injection is the sibling deviation interface-get-undocumented-ptp-field; (2) the injected value isn't constant: after a fabric-PTP deploy, ND rewrites all physical/port-channel records to ptp: true fabric-wide (lab-verified, see the vault note), so normalizing only false would re-break idempotency there. The unconditional strip follows the peerSwitchId precedent on the vPC models. Regression tests cover the ptp: false echo, the post-deploy ptp: true rewrite, and that a user-set ptp still forward-diffs.

[mikewiebe]

High: Unconditional ptp exclusion silently defeats replaced/overridden removal

Issue

The fix for ND's injected ptp echo unconditionally removes ptp from the
reverse comparison even though the module still exposes it as a writable
port-channel field. A replaced or overridden task that omits a previously
enabled PTP value is classified as unchanged, so the requested reset is never
sent.

Evidence

• port_channel_trunk_host_interface.py lines 264-270
  adds reverse_diff_exclude = {"ptp"}, stripping either false or true
  from removal detection.

• The same model still declares ptp as “Enable Precision Time Protocol on
  the interface” at
  line 310,
  includes it in the argspec, and the module documents it as writable at
  nd_interface_port_channel_trunk_host.py line 195.

• The current regression at
  test_base_model_reverse_diff.py lines 708-730
  deliberately asserts that existing ptp: true plus a proposed replacement
  omitting ptp is “no difference.” Test 00580 proves only that an explicitly
  supplied opposite value still forward-diffs.

• Exact-head probe:

  existing response: ptp=true
  proposed replaced config: ptp omitted
  get_diff(..., exclude_unset=False): True  # interpreted as no_diff
  

• A practical two-run workflow demonstrates the user-visible failure. First,
  the user enables PTP through the documented field:

  - name: Enable PTP on the server port-channel
    cisco.nd.nd_interface_port_channel_trunk_host:
      fabric_name: FABRIC1
      state: replaced
      config:
        - switch_ip: 192.0.2.11
          interface_name: port-channel10
          config_data:
            network_os:
              policy:
                admin_state: true
                allowed_vlans: "100-110"
                ports:
                  - Ethernet1/1
                  - Ethernet1/2
                ptp: true

  Later, the user relies on replaced semantics and omits ptp to reset it:

  - name: Replace the policy and reset omitted settings
    cisco.nd.nd_interface_port_channel_trunk_host:
      fabric_name: FABRIC1
      state: replaced
      config:
        - switch_ip: 192.0.2.11
          interface_name: port-channel10
          config_data:
            network_os:
              policy:
                admin_state: true
                allowed_vlans: "100-110"
                ports:
                  - Ethernet1/1
                  - Ethernet1/2
                # ptp omitted: reset it to the default

  Outcome	Expected	Current PR behavior
  Module result	changed: true	changed: false
  Controller action	PUT resetting PTP	No PUT
  Final PTP state	Disabled/defaulted	Remains true

  The forward subset test passes because desired omits ptp. The reverse pass
  should detect the existing-only field, but reverse_diff_exclude removes it,
  so the state machine reports no difference and never sends the reset.

Existing PR overlap

Related to @mikewiebe's active
thread r3681578271.
That thread identified the original ptp: false false-positive loop. The
current finding shows that the unconditional-exclusion repair creates the
inverse false negative. The thread is not close-ready.

Existing open issue overlap

Related open issues: #410
broadly tracks omitted-field false negatives for replaced/overridden, but
does not resolve the new writable-field versus controller-owned ptp contract
conflict introduced by this repair. #497
tracks schema-derived default tables and explicitly leaves undeclared ptp
handling to reverse_diff_exclude; it does not track this conflict either.

Impact

A documented configuration field can remain enabled after a replacement says
to remove omitted settings. This is a silent convergence failure: Ansible
reports no change and never sends the reset. The author's lab evidence that ND
rewrites ptp fabric-wide explains why a simple False default is
insufficient, but it also demonstrates that the ownership contract must be
resolved rather than hiding the field in only one comparison direction.

Suggested fix

Choose one consistent contract. If ptp is controller/fabric-owned telemetry,
remove it from the model's writable fields, argspec, documentation, and payload.
If it is user-writable, preserve real replaced/ overridden reset
semantics while separately normalizing controller-injected fabric state. Add
tests for omitted reset, explicit true/false transitions, and both observed GET
echoes.

[allenrobel]

Confirmed — the exclusion traded the false-positive loop for the inverse false negative, exactly as described. The contract conflict is now resolved by removing ptp from the writable surface entirely (commit "Remove inert ptp field from port-channel trunk-host (#422 review)"), your first option. Two findings drove that choice over preserving reset semantics:

1. intPortChannelTrunkHostTemplate declares no ptp property (nor does intTrunkHostTemplate — no non-IPFM template does).
2. Lab probe, ND 4.2.1.10 (2026-08-12, intent-only, fabric PTP disabled): two trunkPoHost port-channels created identically except one sent ptp: true. ND persisted and echoed the sent value, but GET .../pendingConfig showed byte-identical generated CLI for both — no PTP command. On a non-PTP fabric a user-set ptp was persist-but-inert: it configured nothing while the echo faked success. Rendering with fabric PTP deployed is untested, but our 2026-06-10 lab evidence shows the fabric-PTP deploy rewrites the stored value fabric-wide regardless of per-interface intent — so per-interface ptp cannot durably express user intent in either regime, and there was no working "reset" to preserve.

The injected GET echo now drops at parse time (extra="ignore"), matching the sibling ethernet/vPC models; reverse_diff_exclude is gone, so no writable field is hidden from removal detection. Since the module has not shipped, the option removal has no user impact. Tests cover idempotency against both the ptp: false echo and the post-fabric-PTP true rewrite, plus a pin that ptp stays out of model_fields and every dump. The bug-tracker vault note (interface-get-undocumented-ptp-field) is updated with the probe evidence.

[allenrobel]

Follow-up closing the caveat: we deployed fabric PTP on SITE1 (ptpVlanId: 1000, ToR source SVIs, feature ptp live on all 7 switches) and re-ran the probe in the enabled-not-deployed and fully-deployed regimes. A client-sent ptp is persist-but-inert in all three regimes — echoed back, zero pending-CLI difference vs an identical control. Two structural confirmations from the deploy: ND's generated PTP CLI lands only on physical interfaces, never under port-channels (matching NX-OS's per-physical-port PTP model, so a port-channel-level ptp knob cannot render anything), and the fabric-wide record rewrite reproduced on 4.2.1.10 — every pre-deploy physical/port-channel record now stores ptp: true including port-channel500, which received no PTP CLI. The removal rationale is now unconditional; code comments and the vault note are updated (commit "Close the ptp probe caveat: inert in all three fabric-PTP regimes").

[mtarking]

Reviewed the reverse-pass diff logic — the core design is correct and well-tested, and I agree with the approach. The forward/reverse separation is clean (has_removals keys off presence only, values stay with the forward issubset pass), stripping default-valued existing keys is semantically justified for replaced/overridden (resetting an already-default field is a genuine no-op), and the dump-reuse efficiency refactor is nicely pinned by a regression test. CI is green and the prior ptp finding is resolved.

My comments are non-blocking. The one theme worth a firm follow-up is the hand-maintained, ND-4.2.1-pinned reverse_diff_defaults tables: the failure modes on a future ND default change are asymmetric and one (silently skipped reset) is a correctness risk with no test to catch it. Runtime-deriving those defaults from the GET-echoes schema would remove that fragility. The rest are small clarifying-comment nits (list non-recursion in has_removals, list-of-empties normalization) plus an ask to file an explicit follow-up to lab-verify the unverified vPC tables.

Overall: approve-with-follow-ups from my side.

[mtarking]

has_removals recurses into nested dicts present on both sides but not into lists, so a removal expressed inside a list element (e.g. a nested dict in a policy/ports list that loses a key) won't be detected by the reverse pass. In practice the forward issubset catches most list divergences, so this is likely fine — but the dict-vs-list asymmetry is surprising. Worth a one-line comment here so a future reader doesn't assume deep list coverage.

[allenrobel]

Comment added at the recursion site in "Document reverse-pass list handling and dumped-form table requirement". Worth stating here too: the dict-vs-list asymmetry is benign by construction, not just "likely fine" — the forward issubset pass matches list elements bidirectionally (issubset(item, candidate) and issubset(candidate, item)), so a list element that loses a key fails the forward pass and classifies as changed; a wholly-omitted list key is caught by the key-presence check in has_removals. List coverage is complete across the two passes — it just wasn't visible at this call site, which is what the new comment fixes.

[mikewiebe]

Medium: Reverse scrubbing skips models inside lists

Issue

_scrub_reverse_diff_dict removes controller-only extras from the current model and directly nested models, but it does not recurse into model instances inside lists, tuples, or dictionaries. A controller-added key in a nested list can therefore make an otherwise identical replaced or overridden resource appear changed forever.

Evidence

• base.py lines 304–311 recurses only when the direct field value is an NDBaseModel.

• utils.py lines 50–62 requires list elements to match in both directions, so a server-only key fails the forward comparison before reverse scrubbing occurs.

• NetflowExporterModel permits response extras and is held in netflowExporterCollection: list[NetflowExporterModel] at manage_fabric_common.py lines 110–127.

• The current-head probe retains the controller-only field:

  existing: {'children': [{'name': 'same', 'controllerOnly': 'x'}]}
  reverse:  {'children': [{'name': 'same', 'controllerOnly': 'x'}]}
  get_diff: False  # interpreted as changed
  

Practical example:

# Desired configuration
netflowExporterCollection:
  - exporterName: COLLECTOR1
    exporterIp: 192.0.2.50
    vrf: management
    sourceInterfaceName: loopback0
    udpPort: 2055

ND returns the same configuration with a generated identifier:

netflowExporterCollection:
  - exporterName: COLLECTOR1
    exporterIp: 192.0.2.50
    vrf: management
    sourceInterfaceName: loopback0
    udpPort: 2055
    controllerGeneratedId: exporter-74

Although every user-managed value matches, the list item still contains controllerGeneratedId during comparison. The module consequently reports changed: true and can repeat the same PUT/deploy on every run.

Existing PR overlap

Related to @mtarking’s active thread r3731410339. That thread discusses list recursion, and the reply claims the forward and reverse passes provide complete list coverage. This counterexample shows that user-field removals are covered, but controller-only extras inside list-contained models are not. The thread is not close-ready.

Existing open issue overlap

Related open issue #450 broadly tracks nested-resource idempotency when controller responses contain additional fields, primarily for merged nested resources. It does not identify this PR’s forward-before-scrub failure on the replaced/overridden path. #386 concerns nested-list merge semantics and is a different operation.

Impact

Any shared Gen-3 model that allows response extras in child models held by a container can repeatedly issue writes and deployments for an unchanged resource. Check mode also predicts a false change.

Suggested fix

Recursively scrub NDBaseModel instances inside lists, tuples, and dictionary values before the forward comparison, while preserving container shape and ordering. Add a NetFlow-exporter regression plus a control proving a real user-managed list change is still detected.

[allenrobel]

Confirmed with the real models — existing NetflowExporterModel list item carrying a controller-only key vs. an identical proposed item: forward issubset returns False before any scrub runs, permanent changed=true. Your counterexample also corrects my earlier reply: "complete list coverage" holds for user-field removals, not for controller extras inside list items.

One scoping fact: the failure is in the forward pass's bidirectional list-element match (issubset(item, candidate) and issubset(candidate, item)), and issubset is byte-identical to develop — this PR neither introduced nor touched it. The same false-changed fires on develop today, on the merged path as well; the reverse scrub this PR adds runs only after a forward match, so it cannot reach the defect (which is also why recursing the scrub into lists would not fix the user-visible symptom). Rather than grow #422 into forward-diff semantics, I have posted the counterexample and a proposed fix direction to #450 (nested-resource idempotency when controller responses carry additional fields): #450 (comment). Happy to pick it up there as a fast-follow.

[mikewiebe]

Re-reviewed at 9873648. The prior PTP blocker is resolved, the remaining nested-list behavior predates this PR and is tracked by #450, and the focused/full unit verification is green.