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.

[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.