Detect Multi-status per-item failures in NdV1Strategy (#295)

[allenrobel]

Related Issue(s)

Fixes #295
Relates to #397 (follow-up: consolidate the remaining orchestrators' bespoke 207 detectors)

Proposed Changes

ND reports per-item outcomes for batch operations in DATA.results[] / DATA.switchIds[] items carrying
status: success|failed|failure|error. It sends these bodies on HTTP 207 (Multi-Status) and, for some
endpoints, on a plain HTTP 200 — the L3Out batch POST returns 200 with per-item failures inside.
NdV1Strategy previously classified any success-code response as success, so a per-item failure (e.g. one
switch fails while its peer succeeds, or a subinterface POST rejected because the parent is not routed) was
silently reported as success/changed — masking failures and breaking idempotency.

Central fix (NdV1Strategy):

• is_success() now scans the body of every success-code response (200, 201, 202, 204, 207 — not only
  207) and returns False when any per-item status in results[]/switchIds[] is
  failed/failure/error (case-insensitive, whitespace-tolerant).
• extract_error_message() aggregates the failing items as label: message, so the user sees which items
  failed and why. Dict-error handling factored into a helper to keep branch count in bounds.
• 12 new unit tests cover both envelope shapes, all three literals, whitespace/mixed-case, mixed
  partial-failure, a None-status control, and message aggregation.

Subinterface retirement: the managed and unmanaged subinterface orchestrators each carried their own
_raise_on_multi_status_failures guard (with TODO(4.2.1) markers pointing at #295). These are now
redundant — _request() raises via the strategy — and are removed along with their markers. Redundant
direct-call unit tests are dropped (covered centrally); the end-to-end 207 tests are kept and re-pointed to
the centralized message.

Downstream test updates: five tests in manage_l3out, manage_acl, and manage_policy_group asserted a
bespoke per-item message that the centralized path now preempts for hard-failure literals. They are updated
to the centralized message. Behavior (raise) is preserved, and the bespoke detectors remain in place —
they still cover the allowlist / soft-status cases (warning, notexecuted, != "success") and alternate
envelope shapes the baseline does not. Retiring those is tracked in #397.

Affected orchestrator owners (FYI — your bespoke 207 detectors compose with this change; consolidation tracked in #397)

• @skaszlik — manage_acl, manage_l3out
• @nikhilsrikrishna — manage_policy_group
• @AKDRG — networks / network_attachment_manager, vrfs / vrf_attachment_manager
• @allenrobel — subinterface_* (retired here), maintenance_mode, fabric_update_group

Test Notes

• Full unit suite green inside the nd-dev container: ndpytest tests/unit/ — 3152 passed.

• nd_v1_strategy.py: pylint 10.00/10, mypy clean, black/isort clean.

• Subinterface orchestrators: no new pylint/mypy findings vs develop (verified against the pristine
  baseline; pylint score improved after removing the duplicated guards).

• Verified the central fix composes with the remaining bespoke detectors: manage_policy_group test
  00830 (bare-list body) still routes through its own detector, while 00820 (dict/switchIds body) is now
  caught centrally.

• Follow-up commit "Clarify per-item failure detection docs (RestSend treats HTTP 207 Multi-Status as success even when results[].status is 'failed' or 'error' #295)" addresses the Copilot review: docstrings
  and comments had described the detection as 207-only and omitted the failure literal, i.e. narrower than
  the code actually implements. Comments/docstrings only — no behavior change; suite re-run green.

• Follow-up commit "Harden per-item failure extraction from review (RestSend treats HTTP 207 Multi-Status as success even when results[].status is 'failed' or 'error' #295)" addresses @akinross' review.
  Adds two constant-driven helpers (_get_typed_value, _first_non_empty) and fixes three defects, two of
  which are pre-existing on develop — this PR only moved them into _extract_dict_error_message, and
  they sit on the path that reports an error to the user:

  • _format_multistatus_failure gated its label on is not None, so an item carrying name: "" rendered
    as ": <message>".
  • "messages" in data_dict and len(data_dict.get("messages", [])) raised TypeError when ND sent the key
    with an explicit null value (same for errors) — despite the method's ## Raises: None contract.
  • is_success() classifies DATA.error as a failure, but the extractor had no matching branch, so the
    error ND actually sent was replaced by the generic Request failed with status <code> fallback.

  Per-item message keys are now the _MULTISTATUS_ITEM_MESSAGE_KEYS constant (message, warningMessage,
  status) — ND is not consistent across endpoints. Note the warningMessage items on fabric_update_group
  ride an attachUpdateGroups envelope that is not in _MULTISTATUS_ITEM_KEYS, so they still do not
  reach the central path; the message-key seam is in place, but the envelope-key half remains Consolidate per-item 207 Multi-Status detection: retire remaining bespoke orchestrator detectors onto NdV1Strategy #397's job.

  Six new tests (01300–01350). Full unit suite green — 3158 passed; nd_v1_strategy.py pylint
  10.00/10, mypy clean, black/isort clean.

• Follow-up commit "Scan links[] Multi-Status envelope; label by linkId (PR Detect Multi-status per-item failures in NdV1Strategy (#295) #398 review)" addresses @shrsr's
  request for nd_manage_links. Adds links to _MULTISTATUS_ITEM_KEYS and linkId to
  _MULTISTATUS_ITEM_LABEL_KEYS (ahead of the generic id) — bulk link create (POST /links) and bulk link
  delete (POST /linkActions/remove) return HTTP 207 with {"links": [{"linkId", "message", "status"}]},
  status success|failure (verified against the ND 4.2.1 OpenAPI). The GET /links list body rides the same
  envelope, but its link objects carry no top-level status key, so the literal-gated scan cannot
  false-positive on queries. Four new tests (01360–01390), including the OpenAPI's own linkId: ""
  failure example. Full unit suite green — 3182 passed; pylint 10.00/10, mypy clean, black/isort clean.

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_01NAFsW8ALcvgZBSbgKjRPxn

[Copilot]

Pull request overview

This PR fixes ND API v1 response handling so that success HTTP codes (notably 207 Multi-Status) are no longer treated as overall success when the response body reports per-item failures in DATA.results[] / DATA.switchIds[]. The change centralizes detection and error-message aggregation in NdV1Strategy, and removes redundant per-orchestrator workarounds (subinterface), with unit tests updated accordingly.

Changes:

• Update NdV1Strategy.is_success() to return False when any per-item status in DATA.results[] / DATA.switchIds[] is a failure literal, and enhance extract_error_message() to aggregate failing items.
• Add unit tests covering both envelope shapes, failure literals, casing/whitespace normalization, and aggregation behavior.
• Remove redundant _raise_on_multi_status_failures guards from subinterface orchestrators and adjust downstream orchestrator tests to expect centralized messages.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
plugins/module_utils/rest/response_strategies/nd_v1_strategy.py	Centralizes per-item failure detection and message aggregation for ND v1 responses.
tests/unit/module_utils/test_response_handler_nd.py	Adds focused unit coverage for multi-status per-item failure detection and messaging.
plugins/module_utils/orchestrators/subinterface_unmanaged_interface.py	Removes redundant per-item 207 detector now handled centrally.
plugins/module_utils/orchestrators/subinterface_managed_interface.py	Removes redundant per-item 207 detector now handled centrally.
tests/unit/module_utils/orchestrators/test_subinterface_unmanaged_interface.py	Drops direct tests of the removed guard and updates expectations to centralized failure messaging.
tests/unit/module_utils/orchestrators/test_subinterface_managed_interface.py	Drops direct tests of the removed guard and updates references to centralized detection.
tests/unit/module_utils/orchestrators/test_manage_policy_group_orchestrator.py	Updates assertions/documentation to reflect centralized handling for dict/switchIds envelope.
tests/unit/module_utils/orchestrators/test_manage_acl.py	Updates failure-message assertions now produced via the centralized strategy path.
tests/unit/module_utils/orchestrators/test_l3out.py	Updates assertions/documentation to reflect centralized per-item failure detection and surfaced messages.

---

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

[shrsr]

@allenrobel
Following up on folding the links (207) handling into NdV1Strategy. I checked the links contract against the ND 4.2 OpenAPI, so here is exactly what nd_manage_links needs from the central detector before I can delete its own body scan.

Confirmed contract: bulk create (POST /links) and bulk delete (POST /linkActions/remove) both return HTTP 207 with a body shaped {"links": [{"linkId", "message", "status"}]}, where status is either success or failure. Both scopes (manage and one_manage) use the same shape.

What is needed:

1. Add "links" to _MULTISTATUS_ITEM_KEYS so the detector scans the links array. Adding "items" alongside it is a safe superset if you want to also cover the linkActions family.

2. Add "linkId" to _MULTISTATUS_ITEM_LABEL_KEYS so a failing link is labelled by its id in the aggregated error. Today it would fall through to id or come back unlabelled.

3. Nothing else on the literals. failure is already in _MULTISTATUS_FAILURE_STATUSES and message is already in _MULTISTATUS_ITEM_MESSAGE_KEYS, so those are covered.

4. This is the one we really have to pin down. The detector pulls the failing items out of response["DATA"], but I do not think ND wraps the links body in a DATA envelope at all. By the time my orchestrator sees the bulk response it is already a plain {"links": [...]} at the top level, with no DATA key. If that is also what is_success receives, then reading response["DATA"] hands back an empty dict, the scan finds zero items, and is_success returns True even when a link failed. In other words the central check would silently let every partial failure through for links, which is the exact bug we are trying to kill. So before I switch links over, we need to confirm where the per item array actually lands in the response object is_success is given, top level response["links"] versus response["DATA"]["links"], and point the scan there. If different endpoints expose the body at different nesting levels, the detector has to handle both.

Optional but actually nice: instead of every orchestrator editing these three module level constants, consider letting an orchestrator declare its own result keys and label keys (for example a bulk_result_keys and bulk_label_keys attribute the strategy reads). Then links, l3out, acl and policy_group can each register their envelope without touching the shared file.

Once items 1 and 2 land and the nesting question in 4 is settled, I will delete _raise_on_bulk_failures in nd_manage_links and rely on the central detector. Until then I am keeping the bespoke scan as the safe superset, with a TODO(4.2.1) marker pointing here. Happy to send links response fixtures once the shape is wired in.

[allenrobel]

@shrsr Items 1 and 2 are in — commit "Scan links[] Multi-Status envelope; label by linkId (PR #398 review)". links is now in _MULTISTATUS_ITEM_KEYS and linkId is in _MULTISTATUS_ITEM_LABEL_KEYS (ahead of the generic id). I re-verified the contract against the ND 4.2.1 OpenAPI: both POST /links and POST /linkActions/remove return 207 with {"links": [{"linkId", "message", "status"}]}, status success|failure. Four new tests (01360–01390) cover partial failure, all-success, the spec's own linkId: "" failure example (empty labels are skipped rather than rendered as ": <message>"), and a GET-shaped list body.

On item 4, the nesting question: the detector's read of response["DATA"] is correct, and there is no path on which the per-item array reaches is_success() at the top level. The HttpAPI plugin wraps every parsed JSON body under DATA in _verify_response() (plugins/httpapi/nd.py, self.info["DATA"] = response_data), and RestSend.commit() hands sender.response to the response handler unmodified — so is_success() sees {"RETURN_CODE": 207, ..., "DATA": {"links": [...]}}. The bare {"links": [...]} your orchestrator sees is the post-validation view: orchestrator code consumes response["DATA"] after the handler has already classified the response. The one practical consequence for you: unit fixtures exercising the central path need the full envelope (DATA wrapper), not the bare body.

Related false-positive check: GET /links list bodies ride the same DATA.links[] envelope, but link objects carry no top-level status key, so the literal-gated scan cannot misclassify a query — test 01390 pins this.

On the items superset: holding off. Envelope keys get added one at a time against a verified endpoint shape, since a speculative key risks misreading an unrelated body that happens to carry a failure literal in status. When the linkActions import/export family is wired up, its envelope can be added the same way. That expansion — and your per-orchestrator key-registration idea, which is a good candidate design — are both #397's scope; I'll carry the registration idea into that issue.

Links response fixtures are welcome once you switch the module over — with the above landed, you should be able to drop _raise_on_bulk_failures without a TODO(4.2.1) marker.

[allenrobel]

@gmicol and @skaszlik should review this PR since it removes their 207 handlers in PRs #266, #286, and #288 in favor of this centralized handler.

[mikewiebe]

High: Terminal per-item failures replay the complete mutation for up to 300 seconds

Issue

The new item-status scan converts a success-code 200/207 response containing a
failed member into success=False, but RestSend treats every unsuccessful
result as retryable and resubmits the original mutation until its 300-second
budget expires.

Evidence

• nd_v1_strategy.py line 270
  changes a success-code response containing a failed item into an unsuccessful
  response-handler result.
• rest_send.py lines 330-381
  retries the same method, path, and payload whenever the result is
  unsuccessful; the default budget is 300 seconds.
• An exact-head reproduction using a mixed HTTP 207 response, a three-second
  timeout, and a one-second send interval submitted the identical POST three
  times before returning the deterministic application failure.

Live ND 4.2 verification

A bounded live test ran against Nexus Dashboard 4.2.1 build 4.2.1.10 using
the exact PR head. The test pre-created ACL reg_pr398_seed_205203, then sent
one batch containing that existing ACL and new ACL
reg_pr398_new_205203.

Submission	New ACL result	Existing ACL result	HTTP
1	success: created successfully	failed: already exists	207
2	failed: already exists	failed: already exists	207
3	failed: already exists	failed: already exists	207

All three requests used the identical payload, with SHA-256
3352085c889e2d619b2fc1893f6f8e86c53c3ca003483714bce7903a858b35b3.
The first request created the new ACL, but the existing-ACL failure made the
aggregate result unsuccessful. RestSend then replayed the complete payload
twice, causing the ACL that had just succeeded to fail as already existing.

The live probe was deliberately bounded to a six-second timeout, a two-second
send interval, and three submissions. The production defaults in RestSend
are 300 seconds and five seconds, so the same loop can submit approximately 60
times; that count is derived from the source defaults rather than a
five-minute live run. The final result was success=False, changed=False
despite the first request changing controller state.

The temporary ACLs and fabric were deleted successfully after the test, the
fabric deletion was confirmed by HTTP 404, and the postflight ownership audit
passed with no designated switches managed by the test controller.

Existing PR overlap

No matching existing PR comment found. Existing threads cover supported
envelopes, labels, null handling, error text, and refactoring, but not transport
retry ownership.

Existing open issue overlap

No matching open issue found. Issues
#295 and
#397 track per-item
failure detection and detector consolidation, not replay of a terminal
application failure. Issue
#396 tracks nd_rest
accepting successful HTTP 207 responses, not retry classification. Issue
#456 lists safe 207
handling as a dependency of a future interface workflow; it does not track the
shared RestSend fix.

Impact

A deterministic validation failure can resubmit a non-idempotent batch
mutation up to roughly 60 times. Members that already succeeded are sent again,
which can duplicate side effects, produce misleading already-exists failures,
replace the original diagnostic, and multiply controller load. Orchestrators
with their own outer retries can amplify the request count further.

Suggested fix

Add an explicit terminal-versus-retryable classification between response
handling and RestSend. A successful HTTP status containing a definitive
per-item application failure should stop after one request, while
orchestrator-specific eventual-consistency policies may retain bounded retries.
Add a production-mode regression test with timeout > send_interval that
asserts exactly one submission for mixed 200/207 failures, plus an outer-retry
test for L3Out.

---

[allenrobel]

Confirmed — the live ACL repro made this unambiguous. Fixed in "Classify terminal vs retryable mutation failures in ResponseHandler (#295)" and "Stop RestSend retrying terminal application failures (#295)": the response handler now marks every POST/PUT/DELETE result with retryable, where a failure whose RETURN_CODE is a success code (the application definitively rejected the request) is terminal, and RestSend._commit_normal_mode() breaks after one submission on retryable=False. Terminal scope is any embedded error on a success code, so the pre-existing DATA.error/ERROR-on-200 replay is fixed by the same seam. Non-success codes (5xx) keep today's bounded retries, GET semantics are untouched (no retryable key, default-retryable), and orchestrator-level eventual-consistency policies still work — the L3Out attach "not found" outer retry is pinned by test_l3out_00990. Regression tests: test_rest_send_01100 asserts exactly one submission with timeout > send_interval (a sentinel second response turns any replay into a visible failure), test_rest_send_01110 guards that 5xx still retries, and test_l3out_00980 covers the outer-retry amplification you raised.

[mikewiebe]

Medium: Mixed partial success is reported as changed=False

Issue

The response handler couples aggregate success to changed state. When one batch
member succeeds and another fails, the new scan correctly makes the request
unsuccessful but the failure branch reports changed=False even though Nexus
Dashboard changed.

Evidence

• nd_v1_strategy.py line 270
  makes a mixed success/failure item response unsuccessful.
• response_handler_nd.py lines 220-232
  hard-codes changed=False whenever aggregate success is false.
• The exact-head mixed-result reproduction returned
  success=False, changed=False after one item succeeded and one failed.

Live ND 4.2 verification

A bounded live test against Nexus Dashboard 4.2.1 build 4.2.1.10 used the
exact PR head to send a batch containing one new ACL and one ACL that already
existed. The first HTTP 207 response reported:

Batch member	Controller result
New ACL reg_pr398_new_205203	success: created successfully
Existing ACL reg_pr398_seed_205203	failed: already exists

The next two submissions reported that both ACLs already existed, confirming
that the successful member from the first response had changed persistent
controller state. Nevertheless, the final RestSend result was
success=False, changed=False.

Existing PR overlap

No matching existing PR comment found. The separately posted High finding
reports the observed changed=False value as evidence of complete-payload
replay, but it does not identify or propose a fix for inaccurate partial-change
reporting.

Existing open issue overlap

No matching open issue found. Issue
#295 tracks recognizing
failed members, not truthful partial-change reporting. Issue
#389 tracks
NDStateMachine collection synchronization after mid-run failures, not the
shared response handler's changed result for one mixed 207 batch. Issues
#385 and
#456 discuss final-state
or workflow behavior but do not track this shared response-handler correction.

Impact

Failed Ansible output can claim that nothing changed even though successful
members were created, deleted, or deployed. Recovery automation and operators
cannot trust the result, and a retry begins from unexpectedly modified
controller state.

Suggested fix

Track mutation independently from aggregate success. For recognized itemized
responses, report changed=True when any member succeeded and
changed=False only when every member failed, honoring the controller modified
indicator where available. Preserve both successful and failed member evidence,
and add mixed-success and all-failed tests through ResponseHandler,
RestSend, and at least one module path.

[allenrobel]

Confirmed. Fixed in "Report changed=True for mixed Multi-Status batches (#295)": the failure branch of ResponseHandler._handle_post_put_delete_response() now calls a new strategy method is_changed_on_failure() instead of hard-coding changed=False. The modified response header is authoritative in both directions when present; otherwise any per-item status: success in the recognized envelopes (results[]/switchIds[]/links[]) reports changed=True, and all-failed batches keep changed=False. Non-itemized embedded errors retain the conservative changed=False default. Covered by test_response_handler_nd_01450–01490 (mixed, all-failed, header-override both ways, DATA.error).