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

plugins/module_utils/orchestrators/manage_acl.py

@@ -6,7 +6,7 @@
 
 from __future__ import annotations
 
-from typing import Any, ClassVar
+from typing import ClassVar
 
 from ansible_collections.cisco.nd.plugins.module_utils.endpoints.base import NDEndpointBaseModel
 from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_acl import (
@@ -22,11 +22,6 @@
 from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.base import NDBaseOrchestrator
 from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.types import ResponseType
 
-# Per-item ``status`` values in a 207 Multi-Status body that count as a failure. Anything else --
-# ``success``, missing, empty, or future progress tokens -- is tolerated so informational rows do
-# not surface as spurious errors. Mirrors the prefix-list orchestrator's denylist approach.
-_FAILURE_STATUSES = frozenset({"failed", "failure", "error"})
-
 # camelCase wrapper keys used in ACL request/response bodies.
 _LIST_KEY = "accessControlLists"
 _NAMES_KEY = "accessControlListNames"
@@ -47,11 +42,10 @@ class ManageAclOrchestrator(NDBaseOrchestrator[AclModel]):
     - bulk delete: ``POST /fabrics/{fabricName}/accessControlListActions/remove``
       with ``{"accessControlListNames": [...]}``.
 
-    Because the controller answers these bulk calls with 207 (which
-    ``ResponseHandler`` treats as transport success), every bulk response body is
-    inspected per item; any entry whose ``status`` is in ``_FAILURE_STATUSES``
-    raises with the offending ACL names, so partial failures are not silently
-    reported as success.
+    The controller answers these bulk calls with 207 Multi-Status even when some
+    items fail; ``NdV1Strategy`` inspects the per-item ``results`` array and marks
+    the request failed on any failing item, so partial failures surface as errors
+    from ``_request`` rather than being silently reported as success.
 
     The ``fabric_name`` field is read from ``rest_send.params`` (populated by
     ``NDStateMachine`` from the validated module params).
@@ -85,31 +79,6 @@ def fabric_name(self) -> str:
         """
         return self.rest_send.params.get("fabric_name")
 
-    @staticmethod
-    def _raise_on_207_failures(result: Any, operation: str) -> None:
-        """
-        Inspect a 207 Multi-Status bulk response body. If any per-item ``status``
-        is in ``_FAILURE_STATUSES``, raise with the offending ACL names and
-        messages so partial failures are not silently swallowed.
-        """
-        if not isinstance(result, dict):
-            return
-        items = result.get("results")
-        if not isinstance(items, list) or not items:
-            return
-        failures: list[str] = []
-        for item in items:
-            if not isinstance(item, dict):
-                continue
-            status = str(item.get("status") or "").lower()
-            if status not in _FAILURE_STATUSES:
-                continue
-            name = item.get("name") or "?"
-            message = item.get("message") or "unknown error"
-            failures.append(f"{name}: {message}")
-        if failures:
-            raise Exception(f"ACL {operation} reported per-item failures: {'; '.join(failures)}")
-
     def create(self, model_instance: AclModel, **kwargs) -> ResponseType:
         """Create a single ACL via the bulk endpoint."""
         try:
@@ -191,25 +160,21 @@ def query_all(self, model_instance: AclModel = None, **kwargs) -> ResponseType:
             raise Exception(f"Query all failed: {e}") from e
 
     def create_bulk(self, model_instances: list[AclModel], **kwargs) -> ResponseType:
-        """Bulk-create ACLs in a single request and check the 207 body."""
+        """Bulk-create ACLs in a single request."""
         try:
             api_endpoint = self.create_bulk_endpoint()
             api_endpoint.fabric_name = self.fabric_name
             payload = {_LIST_KEY: [item.to_payload() for item in model_instances]}
-            result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload)
-            self._raise_on_207_failures(result, "create")
-            return result
+            return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload)
         except Exception as e:
             raise Exception(f"Bulk create failed: {e}") from e
 
     def delete_bulk(self, model_instances: list[AclModel], **kwargs) -> ResponseType:
-        """Bulk-delete ACLs in a single request and check the 207 body."""
+        """Bulk-delete ACLs in a single request."""
         try:
             api_endpoint = self.delete_bulk_endpoint()
             api_endpoint.fabric_name = self.fabric_name
             payload = {_NAMES_KEY: [item.name for item in model_instances]}
-            result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload)
-            self._raise_on_207_failures(result, "delete")
-            return result
+            return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload)
         except Exception as e:
             raise Exception(f"Bulk delete failed: {e}") from e

plugins/module_utils/orchestrators/manage_prefix_list.py

@@ -32,11 +32,6 @@
 _QUERY_PAGE_SIZE = 100
 _SCOPED_QUERY_MAX_IDENTIFIERS = 8
 
-# Per-item ``status`` values in a 207 Multi-Status body that count as a failure. Anything else --
-# ``success``, missing, empty, or future progress tokens -- is tolerated so informational rows do
-# not surface as spurious errors. Mirrors the maintenance_mode orchestrator's denylist approach.
-_FAILURE_STATUSES = frozenset({"failed", "failure", "error"})
-
 # Single source of truth for everything that differs between the two address families: the endpoint
 # classes plus the camelCase wrapper keys used in request/response bodies. Centralising this here
 # keeps every CRUD/bulk method address-family agnostic (no scattered ``"ipv4..." if v == "ipv4"``).
@@ -83,10 +78,10 @@ class ManagePrefixListOrchestrator(NDBaseOrchestrator[PrefixListModel]):
     - IPv6 bulk delete: ``POST /fabrics/{fabricName}/ipv6PrefixListActions/remove``
       with ``{"ipv6PrefixListNames": [...]}``.
 
-    Because the controller answers these bulk calls with 207 (which ``ResponseHandler``
-    treats as transport success), every bulk response body is inspected per item; any
-    entry whose ``status`` is in ``_FAILURE_STATUSES`` raises with the offending prefix
-    list names, so partial failures are not silently reported as success.
+    The controller answers these bulk calls with 207 Multi-Status even when some items
+    fail; ``NdV1Strategy`` inspects the per-item ``results`` array and marks the request
+    failed on any failing item, so partial failures surface as errors from ``_request``
+    rather than being silently reported as success.
 
     ``query_all`` fetches both IPv4 and IPv6 prefix lists and injects the
     ``ipVersion`` key into each raw API response dict so ``PrefixListModel``
@@ -278,48 +273,19 @@ def _query_all_for_version(self, version: str) -> list[dict[str, Any]]:
             offset += len(page)
         return results
 
-    @staticmethod
-    def _raise_on_207_failures(result: Any, operation: str) -> None:
-        """
-        Inspect a 207 Multi-Status bulk response body. If any per-item ``status`` is in
-        ``_FAILURE_STATUSES``, raise with the offending prefix list names and messages so partial
-        failures are not silently swallowed (the controller returns 207 even when some items fail).
-        """
-        if not isinstance(result, dict):
-            return
-        items = result.get("results")
-        if not isinstance(items, list) or not items:
-            return
-        failures: list[str] = []
-        for item in items:
-            if not isinstance(item, dict):
-                continue
-            status = str(item.get("status") or "").lower()
-            if status not in _FAILURE_STATUSES:
-                continue
-            name = item.get("name") or "?"
-            message = item.get("message") or "unknown error"
-            failures.append(f"{name}: {message}")
-        if failures:
-            raise Exception(f"prefix list {operation} reported per-item failures: {'; '.join(failures)}")
-
     def _bulk_create_for_version(self, version: str, items: list[PrefixListModel]) -> ResponseType:
-        """Send a single bulk-create request for all items of the given ip_version and check the 207 body."""
+        """Send a single bulk-create request for all items of the given ip_version."""
         config = self._config_for_version(version)
         api_endpoint = self._configure_endpoint(config["post"]())
         payload = {config["list_key"]: [item.to_payload() for item in items]}
-        result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE)
-        self._raise_on_207_failures(result, "create")
-        return result
+        return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE)
 
     def _bulk_delete_for_version(self, version: str, names: list[str]) -> ResponseType:
-        """Send a single bulk-delete request for the given prefix list names and check the 207 body."""
+        """Send a single bulk-delete request for the given prefix list names."""
         config = self._config_for_version(version)
         api_endpoint = self._configure_endpoint(config["bulk_delete"]())
         payload = {config["names_key"]: names}
-        result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE)
-        self._raise_on_207_failures(result, "delete")
-        return result
+        return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE)
 
     def create(self, model_instance: PrefixListModel, **kwargs) -> ResponseType:
         """Create a single prefix list via the bulk endpoint."""

plugins/module_utils/orchestrators/manage_route_map.py

@@ -10,8 +10,8 @@
 
 from ansible_collections.cisco.nd.plugins.module_utils.endpoints.base import NDEndpointBaseModel
 from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_route_maps import (
-    EpManageRouteMapsDelete,
     EpManageRouteMapsBulkDelete,
+    EpManageRouteMapsDelete,
     EpManageRouteMapsGet,
     EpManageRouteMapsListGet,
     EpManageRouteMapsPost,
@@ -24,11 +24,6 @@
 from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.base import NDBaseOrchestrator
 from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.types import ResponseType
 
-# Per-item ``status`` values in a 207 Multi-Status body that count as a failure. Anything else --
-# ``success``, missing, empty, or future progress tokens -- is tolerated so informational rows do
-# not surface as spurious errors. Mirrors the ACL orchestrator's denylist approach.
-_FAILURE_STATUSES = frozenset({"failed", "failure", "error"})
-
 # camelCase wrapper key used in route-map list responses and bulk-create request bodies.
 _LIST_KEY = "routeMaps"
 
@@ -99,28 +94,6 @@ def preflight(self, model_instances: list[RouteMapModel]) -> None:
         if model_instances:
             self.fabric_context.validate_for_mutation()
 
-    @staticmethod
-    def _raise_on_bulk_errors(result: ResponseType, action: str) -> None:
-        """Raise when a 207 bulk response contains failed per-item results."""
-        if not isinstance(result, dict):
-            return
-        failures = []
-        for item in result.get("results", []):
-            if not isinstance(item, dict):
-                continue
-            status = str(item.get("status") or "").lower()
-            if status in _FAILURE_STATUSES:
-                failures.append(item)
-        if not failures:
-            return
-        details = []
-        for item in failures:
-            name = item.get("name") or "<unknown>"
-            status = item.get("status") or "<unknown>"
-            message = item.get("message") or "no message"
-            details.append(f"{name}: {status}: {message}")
-        raise RuntimeError(f"Route map bulk {action} failed for {', '.join(details)}")
-
     # -------------------------------------------------------------------------
     # Query helpers
     # -------------------------------------------------------------------------
@@ -226,9 +199,7 @@ def create_bulk(self, model_instances: list[RouteMapModel], **kwargs) -> Respons
         try:
             api_endpoint = self._configure_endpoint(self.create_bulk_endpoint())
             payload = {_LIST_KEY: [item.to_payload() for item in model_instances]}
-            result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE)
-            self._raise_on_bulk_errors(result, "create")
-            return result
+            return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.CREATE)
         except Exception as e:
             raise Exception(f"Bulk create failed: {e}") from e
 
@@ -243,8 +214,6 @@ def delete_bulk(self, model_instances: list[RouteMapModel], **kwargs) -> Respons
             api_endpoint = self._configure_endpoint(self.delete_bulk_endpoint())
             route_map_names = [item.get_identifier_value() for item in model_instances]
             payload = {"routeMapNames": route_map_names}
-            result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE)
-            self._raise_on_bulk_errors(result, "delete")
-            return result
+            return self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=payload, operation_type=OperationType.DELETE)
         except Exception as e:
             raise Exception(f"Bulk delete failed: {e}") from e

plugins/module_utils/orchestrators/subinterface_managed_interface.py

@@ -72,32 +72,6 @@ class SubinterfaceManagedInterfaceOrchestrator(NDBaseInterfaceOrchestrator[Subin
     supports_bulk_create: ClassVar[bool] = True
     supports_bulk_delete: ClassVar[bool] = True
 
-    # TODO(4.2.1) ND returns HTTP 207 Multi-Status on subinterface POST with per-item `status: "failed"` when the parent
-    # interface is not in routed mode (or other policy validation fails). Our RestSend response_handler treats 207 as
-    # success and returns the body without raising, so without this check the orchestrator would silently report
-    # "changed" when nothing was actually created. Remove this workaround once CiscoDevNet/ansible-nd#295 lands the
-    # 207-aware response handling at the RestSend layer.
-    @staticmethod
-    def _raise_on_multi_status_failures(response: ResponseType) -> None:
-        """
-        # Summary
-
-        Inspect a 207 Multi-Status body and raise if any item carries `status: "failed"` or `status: "error"`.
-
-        ## Raises
-
-        ### RuntimeError
-
-        - If `response["results"]` contains any item with `status` in `("failed", "error")`.
-        """
-        if not isinstance(response, dict):
-            return
-        results = response.get("results") or []
-        failed = [r for r in results if isinstance(r, dict) and r.get("status") in ("failed", "error")]
-        if failed:
-            summary = "; ".join(f"{r.get('name')}: {r.get('message')}" for r in failed)
-            raise RuntimeError(f"ND rejected {len(failed)} interface(s): {summary}")
-
     create_endpoint: type[NDEndpointBaseModel] = EpManageInterfacesPost
     update_endpoint: type[NDEndpointBaseModel] = EpManageInterfacesPut
     delete_endpoint: type[NDEndpointBaseModel] = NDEndpointBaseModel  # unused; delete() uses bulk remove
@@ -126,7 +100,6 @@ def create(self, model_instance: SubinterfaceManagedInterfaceModel, **kwargs) ->
             payload["switchId"] = switch_id
             request_body = {"interfaces": [payload]}
             result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body)
-            self._raise_on_multi_status_failures(result)
             self._queue_deploy(model_instance.interface_name, switch_id)
             return result
         except Exception as e:
@@ -202,7 +175,6 @@ def create_bulk(self, model_instances: list[SubinterfaceManagedInterfaceModel],
                 api_endpoint = self._configure_endpoint(self.create_bulk_endpoint(), switch_sn=switch_id)  # pyright: ignore[reportOptionalCall]
                 request_body = {"interfaces": [payload for interface_name, payload in items]}
                 result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body)
-                self._raise_on_multi_status_failures(result)
                 results.append(result)
                 for interface_name, payload in items:
                     self._queue_deploy(interface_name, switch_id)

plugins/module_utils/orchestrators/subinterface_unmanaged_interface.py

@@ -60,35 +60,6 @@ class SubinterfaceUnmanagedInterfaceOrchestrator(NDBaseInterfaceOrchestrator[Sub
     create_bulk_endpoint: type[NDEndpointBaseModel] | None = EpManageInterfacesPost
     delete_bulk_endpoint: type[NDEndpointBaseModel] | None = EpManageInterfacesRemove
 
-    # TODO(4.2.1) multi-status-207-status-field-inconsistent
-    # ND returns HTTP 207 Multi-Status on subinterface POST with per-item `status: "failed"` when the parent
-    # interface is not in routed mode (or other policy validation fails). Our RestSend response_handler treats 207 as
-    # success and returns the body without raising, so without this check the orchestrator would silently report
-    # "changed" when nothing was actually created. Remove this workaround once CiscoDevNet/ansible-nd#295 lands the
-    # 207-aware response handling at the RestSend layer.
-    @staticmethod
-    def _raise_on_multi_status_failures(response: ResponseType) -> None:
-        """
-        # Summary
-
-        Inspect a 207 Multi-Status body and raise if any item carries `status: "failed"` or `status: "error"`. The
-        comparison is case-insensitive and whitespace-tolerant because ND is inconsistent about the casing of the
-        per-item `status` value across endpoints (see the `multi-status-207-status-field-inconsistent` vault note).
-
-        ## Raises
-
-        ### RuntimeError
-
-        - If `response["results"]` contains any item whose `status` is `"failed"` or `"error"` (any casing).
-        """
-        if not isinstance(response, dict):
-            return
-        results = response.get("results") or []
-        failed = [r for r in results if isinstance(r, dict) and str(r.get("status") or "").strip().lower() in ("failed", "error")]
-        if failed:
-            summary = "; ".join(f"{r.get('name')}: {r.get('message')}" for r in failed)
-            raise RuntimeError(f"ND rejected {len(failed)} interface(s): {summary}")
-
     def create(self, model_instance: SubinterfaceUnmanagedInterfaceModel, **kwargs) -> ResponseType:
         """
         # Summary
@@ -110,7 +81,6 @@ def create(self, model_instance: SubinterfaceUnmanagedInterfaceModel, **kwargs)
             payload["switchId"] = switch_id
             request_body = {"interfaces": [payload]}
             result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body)
-            self._raise_on_multi_status_failures(result)
             self._queue_deploy(model_instance.interface_name, switch_id)
             return result
         except Exception as e:
@@ -186,7 +156,6 @@ def create_bulk(self, model_instances: list[SubinterfaceUnmanagedInterfaceModel]
                 api_endpoint = self._configure_endpoint(self.create_bulk_endpoint(), switch_sn=switch_id)  # pyright: ignore[reportOptionalCall]
                 request_body = {"interfaces": [payload for interface_name, payload in items]}
                 result = self._request(path=api_endpoint.path, verb=api_endpoint.verb, data=request_body)
-                self._raise_on_multi_status_failures(result)
                 results.append(result)
                 for interface_name, payload in items:
                     self._queue_deploy(interface_name, switch_id)