Add shared module utility support

[sivakasi-cisco]

Related Issue(s)

Fixes #450

Problem Description

This PR separates out a few shared framework changes that are needed before modules like nd_manage_vrf_lite can cleanly follow the generic ND 4.x module architecture - #281.

Some ND 4.x resources are simple: one playbook item maps directly to one controller object. In those cases, the existing generic state machine can compare “what the user wants” with “what exists on the controller” and decide create, update, delete, or no-op.

But some resources are nested. VRF Lite is a good example: the user gives VRF-level config, but the actual work happens under smaller child objects like VRF attachments and VRF Lite links. Without small shared framework improvements, each nested module has to write its own custom comparison logic.

This PR adds the common support needed for that style of module.

Why These Common Changes Are Needed

This PR updates the common state machine and utility logic used by the VRF Lite workflow.

The main change is to make merged state behave correctly when the controller already has extra data that the user did not provide in the playbook. In that case, the module should not report a change just because existing data has additional fields.

This PR also tracks deleted items in the state machine, so follow-up actions like config save and deploy can know which VRFs were changed by a delete operation.

[sivakasi-cisco]

Addressed your comments.

For the comment on data.py and utils.py files, I can combine them if required.

You can consider the latest changes for review and subsequent approval.

[sivakasi-cisco]

Resolved the comments

[gmicol]

LGTM

[allenrobel]

Code review

Found 4 issues:

1. _delete_items adds items to self.sent unconditionally. _execute_operation returns None without calling the API in check_mode, and orchestrators may skip items during delete (see NDStateMachine._delete_items removes orchestrator-skipped items from existing, corrupting 'after' state #307 for the same unconditional pattern with existing.delete_many), yet all items are still added to sent. This contradicts the guard used for creates/updates in _manage_create_update_state ("Mark as sent only after successful API operations", L182-L185). Since the PR's stated purpose for sent is to drive follow-up config save/deploy, check_mode runs and orchestrator-skipped items would produce false deploy triggers.

ansible-nd/plugins/module_utils/nd_state_machine.py

Lines 217 to 221 in 28f6cdc

	# Batch remove from collection (single index rebuild)
	keys_to_delete = [item.get_identifier_value() for item in items]
	self.existing.delete_many(keys_to_delete)
	self.sent.add_many(items)

2. The new issubset(allow_superset=True) path has no unit tests. It relaxes the bidirectional list-element match that was deliberately added as a bug fix in Ansible ND 4.X Fabric Modules for iBGP, eBGP and External Fabric Types #209, and the final diff contains no test files (the tests added earlier in this PR were removed along with common/data.py). An earlier review comment on this PR asked for unit tests for all the helper functions; that still applies to issubset and to get_diff(exclude_unset=True) with list-valued fields.

ansible-nd/plugins/module_utils/utils.py

Lines 32 to 43 in 28f6cdc

	def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool:
	"""Check if subset is contained in superset.
	Args:
	subset: The value to check.
	superset: The value to check against.
	allow_superset: When True, list element matching is one-directional:
	an element in ``subset`` is considered matched when it is a subset
	of a candidate in ``superset``, even if the candidate has
	additional keys. When False (default) both directions are
	required, which is equivalent to equality for lists of dicts.
	"""

3. Greedy first-match list matching can report false diffs with allow_superset=True. Each subset item consumes the first matching candidate (del remaining[index]), so a less-specific proposed item can consume a candidate needed by a more-specific one. Example: proposed [{"a": 1}, {"a": 1, "b": 2}] vs existing [{"a": 1, "b": 2}, {"a": 1, "b": 3}] — {"a": 1} consumes {"a": 1, "b": 2}, then {"a": 1, "b": 2} fails against {"a": 1, "b": 3}, reporting a diff although a valid pairing exists. One-directional matching makes this ambiguity much more likely than the bidirectional default. Result is spurious changed=True (broken idempotency) in merged state for lists of similar dicts — exactly the shape of VRF attachments.

ansible-nd/plugins/module_utils/utils.py

Lines 52 to 64 in 28f6cdc

	remaining = list(superset)
	for item in subset:
	for index, candidate in enumerate(remaining):
	if allow_superset:
	match = issubset(item, candidate, allow_superset=True)
	else:
	match = issubset(item, candidate) and issubset(candidate, item)
	if match:
	del remaining[index]
	break
	else:
	return False
	return True

4. The new docstring claim "When False (default) both directions are required, which is equivalent to equality for lists of dicts" is inaccurate: the dict branch skips None-valued keys (L68-L69), so {"a": 1, "b": None} and {"a": 1} match bidirectionally despite not being equal. Harmless for current callers (to_diff_dict uses exclude_none=True) but misleading for future direct callers of issubset.

ansible-nd/plugins/module_utils/utils.py

Lines 38 to 43 in 28f6cdc

	allow_superset: When True, list element matching is one-directional:
	an element in ``subset`` is considered matched when it is a subset
	of a candidate in ``superset``, even if the candidate has
	additional keys. When False (default) both directions are
	required, which is equivalent to equality for lists of dicts.
	"""

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[sivakasi-cisco]

Code review

Found 4 issues:

1. _delete_items adds items to self.sent unconditionally. _execute_operation returns None without calling the API in check_mode, and orchestrators may skip items during delete (see NDStateMachine._delete_items removes orchestrator-skipped items from existing, corrupting 'after' state #307 for the same unconditional pattern with existing.delete_many), yet all items are still added to sent. This contradicts the guard used for creates/updates in _manage_create_update_state ("Mark as sent only after successful API operations", L182-L185). Since the PR's stated purpose for sent is to drive follow-up config save/deploy, check_mode runs and orchestrator-skipped items would produce false deploy triggers.

ansible-nd/plugins/module_utils/nd_state_machine.py

Lines 217 to 221 in 28f6cdc

	# Batch remove from collection (single index rebuild)
	keys_to_delete = [item.get_identifier_value() for item in items]
	self.existing.delete_many(keys_to_delete)
	self.sent.add_many(items)

Reply: Fixed — only items actually pushed are marked as sent now, so check-mode and skipped/failed items no longer trigger a deploy.

2. The new issubset(allow_superset=True) path has no unit tests. It relaxes the bidirectional list-element match that was deliberately added as a bug fix in Ansible ND 4.X Fabric Modules for iBGP, eBGP and External Fabric Types #209, and the final diff contains no test files (the tests added earlier in this PR were removed along with common/data.py). An earlier review comment on this PR asked for unit tests for all the helper functions; that still applies to issubset and to get_diff(exclude_unset=True) with list-valued fields.

ansible-nd/plugins/module_utils/utils.py

Lines 32 to 43 in 28f6cdc

	def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool:
	"""Check if subset is contained in superset.
	Args:
	subset: The value to check.
	superset: The value to check against.
	allow_superset: When True, list element matching is one-directional:
	an element in ``subset`` is considered matched when it is a subset
	of a candidate in ``superset``, even if the candidate has
	additional keys. When False (default) both directions are
	required, which is equivalent to equality for lists of dicts.
	"""

Reply: Added tests/unit/module_utils/test_utils.py covering 'issubset' and 'get_diff'

3. Greedy first-match list matching can report false diffs with allow_superset=True. Each subset item consumes the first matching candidate (del remaining[index]), so a less-specific proposed item can consume a candidate needed by a more-specific one. Example: proposed [{"a": 1}, {"a": 1, "b": 2}] vs existing [{"a": 1, "b": 2}, {"a": 1, "b": 3}] — {"a": 1} consumes {"a": 1, "b": 2}, then {"a": 1, "b": 2} fails against {"a": 1, "b": 3}, reporting a diff although a valid pairing exists. One-directional matching makes this ambiguity much more likely than the bidirectional default. Result is spurious changed=True (broken idempotency) in merged state for lists of similar dicts — exactly the shape of VRF attachments.

ansible-nd/plugins/module_utils/utils.py

Lines 52 to 64 in 28f6cdc

	remaining = list(superset)
	for item in subset:
	for index, candidate in enumerate(remaining):
	if allow_superset:
	match = issubset(item, candidate, allow_superset=True)
	else:
	match = issubset(item, candidate) and issubset(candidate, item)
	if match:
	del remaining[index]
	break
	else:
	return False
	return True

Reply:: Sure. Swapped the greedy first-match for proper bipartite matching so as to find a valid pairing when it exists. Added a regression test as well.

4. The new docstring claim "When False (default) both directions are required, which is equivalent to equality for lists of dicts" is inaccurate: the dict branch skips None-valued keys (L68-L69), so {"a": 1, "b": None} and {"a": 1} match bidirectionally despite not being equal. Harmless for current callers (to_diff_dict uses exclude_none=True) but misleading for future direct callers of issubset.

ansible-nd/plugins/module_utils/utils.py

Lines 38 to 43 in 28f6cdc

	allow_superset: When True, list element matching is one-directional:
	an element in ``subset`` is considered matched when it is a subset
	of a candidate in ``superset``, even if the candidate has
	additional keys. When False (default) both directions are
	required, which is equivalent to equality for lists of dicts.
	"""

Reply:: As the dict comparison ignores 'None' keys it was not strict ==. Corrected the docstring now.

🤖 Generated with Claude Code

• If this code review was useful, please react with 👍. Otherwise, react with 👎.

[mikewiebe]

High: PRs #281 and #360 require semantic conflict reconciliation after #294

“Conflict reconciliation” means manually combining both PRs’ behaviors after #294 merges. Selecting Git’s entire “ours” or “theirs” version would break one side.

Currently, PR #281 already conflicts with develop. PR #360 merges with current develop, but conflicts after applying #294.

PR #360

Both PRs modify NDBaseModel.get_diff():

• Add shared module utility support #294 adds the allow_superset argument for merged-list comparison.
• Enforce storm-control percentage/pps mutual exclusivity (#351) #360 adds merge_would_change() to detect storm-control percentage/PPS transitions that have merge side effects.

The combined implementation needs both:

def get_diff(
    self,
    other,
    exclude_unset=False,
    allow_superset=False,
):
    self_data = self.to_diff_dict()
    other_data = other.to_diff_dict(exclude_unset=exclude_unset)

    is_subset = issubset(
        other_data,
        self_data,
        allow_superset=allow_superset,
    )

    if is_subset and exclude_unset and self.merge_would_change(other):
        return False

    return is_subset

Keep #360’s merge_would_change() implementation too.

Taking only #294 would lose storm-control transition detection. Taking only #360 would remove allow_superset, causing the same keyword-argument compatibility failure discussed for #286 and #312.

The combined tests should prove that:

• Existing percentage can transition to PPS.
• Existing PPS can transition to percentage.
• A controller response containing both values is remediated.
• Add shared module utility support #294’s merged-list behavior still works.

PR #281

PR #281 conflicts in three shared files and also has semantic overlaps that Git may auto-merge incorrectly.

models/base.py

• Keep Add shared module utility support #294’s recursive removal of implicit None values in from_config().
• Do not restore Vrf_lite module for Ansible ND 4.x #281’s direct validation of the uncleaned Ansible dictionary.

nd_config_collection.py

• The direct conflict is mostly documentation.
• Keep the contract that None can be normalized to an empty collection only after the caller has validated whether omitted config is legal for the requested state.

nd_state_machine.py

• Keep Add shared module utility support #294’s raw config is None validation for write states.
• Keep state-aware Pydantic validation and preflight checks.
• Keep Add shared module utility support #294’s rule that sent contains only operations actually sent to ND and remains empty in check mode.
• Preserve Vrf_lite module for Ansible ND 4.x #281’s track_deletes_in_sent behavior as a VRF Lite opt-in, with the generic default remaining False.

Two additional #281 behaviors need deliberate adjustment:

• Vrf_lite module for Ansible ND 4.x #281 converts omitted config to [] before creating the state machine. It must validate raw user input first so omitted config is not confused with an intentional empty configuration.
• Vrf_lite module for Ansible ND 4.x #281 calculates _changed_vrfs from sent. In check mode, that collection is intentionally empty under Add shared module utility support #294. Planned VRFs should instead be derived from the before/after preview while still preventing save or deploy operations.

Its keyed VRF Lite attachment merge behavior must also remain intact.

Recommended integration order

1. Finalize and merge Add shared module utility support #294.
2. Rebase Enforce storm-control percentage/pps mutual exclusivity (#351) #360 and manually combine both get_diff() behaviors.
3. Rebase Vrf_lite module for Ansible ND 4.x #281, dropping duplicated shared-framework changes where Add shared module utility support #294 now supplies them.
4. Resolve the three Vrf_lite module for Ansible ND 4.x #281 shared files according to the contracts above.
5. Run each module’s tests together with Add shared module utility support #294’s shared state-machine and diff tests.

[sivakasi-cisco]

Have tested the nd_vrf_lite module with the latest changes in #294.

[sivakasi-cisco]

Hi Mike,

This is really helpful with the integration order.

For PR294 - the current one - base merged to 'develop' branch.

Now the combined version of base.py is below with allow_superset and merge_would_change.

self_data = self.to_diff_dict()
other_data = other.to_diff_dict(exclude_unset=exclude_unset)
is_subset = issubset(other_data, self_data, allow_superset=allow_superset)
if is_subset and exclude_unset and self.merge_would_change(other):
return False
return is_subset

Both #294 and #360 touch NDBaseModel.get_diff()

#294 added allow_superset for list comparison
#360 added merge_would_change to catch storm-control percentag

[allenrobel]

Code review

Fresh pass over the current head (my June review was dismissed by subsequent pushes). One behavioral concern with check-mode sent tracking and two type-annotation notes, each as an inline comment.

🤖 Generated with Claude Code

[allenrobel]

LGTM after comments addressed.