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.

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

Design concern (non-blocking, already partially acknowledged via the TODO(4.2.1) markers): these per-model reverse_diff_defaults tables are hand-maintained and pinned to the ND 4.2.1 OpenAPI schema. The two failure modes on an ND upgrade that changes a template default are asymmetric, and one is dangerous:

• Stale/wrong value → the existing-side key survives → false changed=True + redundant PUT (noisy but safe).
• User sets a field on-device to the table's (now-wrong) value and omits it from proposed → the reverse pass strips it → the reset is silently skipped. That's a correctness regression with no test to catch it.

The /nd-workaround-audit tooling and vault note mitigate but don't eliminate this. Could we track a firm follow-up to derive these defaults at runtime from the GET-echoes schema rather than transcribing them by hand? That would remove the version-pinning fragility across all ten tables.

[allenrobel]

The silent-skip direction (user resets a field to the table's now-stale value, omits it from proposed, reverse pass strips it) is the dangerous one, agreed — noisy false-changed is self-announcing, this isn't. Filed #497 to track deriving these defaults at runtime from the template schema ND serves, replacing the hand-maintained tables. The issue captures the asymmetry, the dumped-form coercion requirement, and the schema-fetch-failure fallback question. Note reverse_diff_exclude stays regardless: it covers fields ND echoes but does not declare in any schema (ptp, peerSwitchId), which no runtime derivation would return.

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

[mtarking]

Minor: a dict of only-empty values is collapsed to empty (the recursive branch below), but a list of empty markers ([""], [{}]) is not — only an exact == [] counts. That's probably intentional, but the asymmetry with the dict case is surprising; a short comment noting lists are only empty-normalized when literally [] would help.

[allenrobel]

Comment added in "Document reverse-pass list handling and dumped-form table requirement". The asymmetry is intentional conservatism: ND has only been observed echoing literal [] for never-configured list fields, so only that form is empty-normalized. Collapsing [""] / [{}] without lab evidence of ND echoing those shapes could mask a real pending removal, so lists stay literal-[]-only until we see otherwise.

[mtarking]

This routeMapTag: "12345" (int→str GET-side drift) is a good illustration of the broader brittleness: reverse_diff_defaults values must match the serialized/dumped form exactly. A subtle form mismatch (int vs str, enum vs value, casing) silently reopens #410 for that field with no failing test. Each entry effectively needs lab verification in dumped form — worth calling out in the review/merge notes as a maintenance cost, not just for loopback.

[allenrobel]

Generalized in "Document reverse-pass list handling and dumped-form table requirement": the base-class comment over reverse_diff_defaults now states that table values must be in the model's dumped form, not the schema-declared form, citing routeMapTag (schema integer 12345, stored "12345") as the failure shape — a form mismatch silently reopens #410 for that field with no failing test. Will also call this out in the merge notes as a maintenance cost applying to all ten tables; #497 (runtime derivation) is the longer-term fix for the transcription fragility.

[mtarking]

The PR body notes the vPC families were not lab-verified in this PR, and that TrunkVpcHostPolicyModel's peer1AllowedVlans/peer2AllowedVlans schema defaults have no corresponding model fields (per-peer → collapsed access_vlan workaround). Since vPC is the most complex interface family and this table drives replaced/overridden idempotency, could we file an explicit tracked follow-up to lab-verify both vPC tables (access + trunk-host) against a real ND read before users rely on replaced/overridden here? Flagging so the residual doesn't get lost after merge.

[allenrobel]

We'll lab-verify both vPC tables (access + trunk-host) against a real ND read today and reply here with the results by end of today.

[allenrobel]

Lab verification completed as promised — both vPC reverse_diff_defaults tables checked against live ND 4.2.1 reads (SITE1, clean greenfield rebuild earlier today; physical-peer-link vPC pair S1_LE1/S1_LE2).

Method: created minimal trunkVpcHost and accessVpcHost interfaces sending only identity + port-channel ids + member ports (every optional field unset), then compared the GET echo field-by-field against the tables.

Results:

• All 23 trunk-table and 22 access-table entries match the live echoes value-for-value. No stale or wrong-valued entries in either table.
• One gap found and fixed: ND echoes a collapsed allowedVlans: "none" on trunkVpcHost GETs when the user never set it. The trunk table had no entry for it, so replaced/overridden reported changed on every run — lab-reproduced with the module (merged idempotent, replaced never converged). Fixed in commit "Cover the collapsed allowedVlans echo in the vPC trunk reverse pass (Detect field removals in replaced/overridden states (one-way subset diff) #422 lab-verify)".
• The fix required the dumped-form keys — exactly the subtlety the dumped-form table comment on this line warns about. A collapsed-form "allowedVlans" entry does NOT work (lab-reproduced): the write-side dump fans the field out per vpc-interface-peer-vlan-collapse, so the entries are peer1AllowedVlans/peer2AllowedVlans = "none". Regression unit test added (test_base_model_reverse_diff_00590) covering both the unset-echo no-diff and explicit-value change-detection paths. Post-fix lab rerun: merged ×2 and replaced ×2 all idempotent.
• nativeVlan and accessVlan are NOT echoed when unset (verified on both types), so their absence from the tables is correct — the asymmetry vs allowedVlans is now recorded in the vault note (vpc-interface-peer-vlan-collapse).
• ptp: ND injects ptp: false into vPC GETs too (same as port-channel), but the vPC policy models don't declare ptp, so extra="ignore" drops it at parse time — no exclude needed, unlike port-channel where it's a declared field.

Incidental lab observation: a physical-peer-link vPC pair created with the bare pair payload (no vpcPairDetails, ND auto-selects the peer-link) config-saved and deployed cleanly on 4.2.1 / NX-OS 10.6(2) — peer adjacency ok, Po500 up. The vpc-physical-peer-link-configsave-fails 500 documented in the integration vars appears specific to explicitly supplying member ports in vpc_pair_details; worth a follow-up before reverting the integration substrate to physical.

Full suite: 3912 unit tests green; black/isort/pylint/mypy clean on the changed files.