From 974a46255d44c1e3ffaaf0f7b3cc8f271855db39 Mon Sep 17 00:00:00 2001 From: etwong_amdeng Date: Fri, 30 Oct 2026 17:36:20 -0400 Subject: [PATCH 1/4] remove low priority SA, add fallback for afids without serviceableunits, add a summary.json file --- .../plugins/serviceability/analysis_window.py | 1 + .../serviceability/mi4xx/mi4xx_analyzer.py | 43 +++++++++++++ .../plugins/serviceability/se_runner.py | 63 ++++++++++++++++++- .../serviceability_recommendations_table.py | 27 +++----- test/unit/plugin/test_mi4xx_plugin.py | 33 ++++++++++ test/unit/plugin/test_se_runner.py | 58 +++++++++++++++++ ...st_serviceability_recommendations_table.py | 7 +-- 7 files changed, 207 insertions(+), 25 deletions(-) diff --git a/nodescraper/plugins/serviceability/analysis_window.py b/nodescraper/plugins/serviceability/analysis_window.py index 911246e1..e9eab36e 100644 --- a/nodescraper/plugins/serviceability/analysis_window.py +++ b/nodescraper/plugins/serviceability/analysis_window.py @@ -191,6 +191,7 @@ def analyze_serviceability_window( rf_event_count=len(data.rf_events), raise_on_error=args.hub_raise_on_error, prefer_rf_events=args.hub_prefer_rf_events and not bool(cper_data), + logger=log, ) except (HubRunError, ValueError) as exc: return ServiceabilityWindowResult( diff --git a/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py b/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py index 6676a370..61159a1c 100644 --- a/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py +++ b/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py @@ -25,10 +25,21 @@ ############################################################################### from __future__ import annotations +import json +from datetime import datetime, timezone +from typing import Any + +from nodescraper.connection.inband import TextFileArtifact +from nodescraper.plugins.serviceability.se_models import ServiceabilityBlock +from nodescraper.plugins.serviceability.serviceability_api import ( + split_recommendation_actions, +) from nodescraper.plugins.serviceability.serviceability_hub_analyzer import ( ServiceabilityHubAnalyzer, ) +SUMMARY_FILENAME = "summary.json" + class MI4XXAnalyzer(ServiceabilityHubAnalyzer): """Build AFID events from collected data and run the configured entry-point hub.""" @@ -38,4 +49,36 @@ class MI4XXAnalyzer(ServiceabilityHubAnalyzer): "Runs the configured entry-point service hub (hub_entry_point in analysis_args) to produce service recommendations.", "When analysis_args.skip_hub is true, only builds AFID events without running the hub.", "Supports offline analysis from a prior collection via --data with --collection False.", + f"Writes the top service action to {SUMMARY_FILENAME} in the analyzer log directory.", ) + + def _append_hub_artifacts(self, block: ServiceabilityBlock) -> None: + super()._append_hub_artifacts(block) + self.result.artifacts.append( + TextFileArtifact( + filename=SUMMARY_FILENAME, + contents=json.dumps(self._top_action_summary(block), indent=2), + ) + ) + + def _top_action_summary(self, block: ServiceabilityBlock) -> dict[str, Any]: + """Build the summary.json payload for the top Hub service action. + + Args: + block: Serviceability block returned by the hub. + + Returns: + System name, analysis time, hub and SAG versions, AFID event count, and the + top service action(s). Ties for the top rank are all included. + """ + top, _ = split_recommendation_actions(block) + return { + "system_name": self.system_info.name, + "analyzed_at": datetime.now(timezone.utc).isoformat(), + "hub_version": block.hub_version, + "afid_sag_file_version": block.afid_sag_file_version, + "afid_event_count": len(block.afid_events), + "top_service_actions": [ + action.model_dump(mode="json", exclude_none=True) for action in top + ], + } diff --git a/nodescraper/plugins/serviceability/se_runner.py b/nodescraper/plugins/serviceability/se_runner.py index dc7b74de..d3059260 100644 --- a/nodescraper/plugins/serviceability/se_runner.py +++ b/nodescraper/plugins/serviceability/se_runner.py @@ -31,9 +31,12 @@ import importlib import importlib.metadata import inspect +import logging from pathlib import Path from typing import Any, Callable, Optional, Protocol, Type, Union, cast +from nodescraper.constants import DEFAULT_LOGGER + from .afid_sag_paths import validate_afid_sag_path from .se_adapter import ( serviceability_block_from_entry_point_hub, @@ -292,15 +295,71 @@ def events_payload_for_hub_analyze( raise HubRunError("rf_events or afid_events is required") +def _usable_serviceable_units(units: Any) -> list[Any]: + """Return ServiceableUnits entries that carry an '@odata.id' string.""" + if not isinstance(units, list): + return [] + return [ + unit for unit in units if isinstance(unit, dict) and isinstance(unit.get("@odata.id"), str) + ] + + +def drop_afids_without_serviceable_units( + rf_events: list[Any], + logger: Optional[logging.Logger] = None, +) -> list[Any]: + """Waive AMDFieldIdentifiers entries the hub would reject for lacking ServiceableUnits. + + The entry-point hub rejects the whole event list when any Oem.AMD.AMDFieldIdentifiers + entry has no ServiceableUnits, so drop those entries (with a warning) before analysis. + + Args: + rf_events: Raw Redfish log members. + logger: Logger for the per-entry warning. Defaults to the nodescraper logger. + + Returns: + Members with unusable AFID entries removed. Members left with no AFID entries are dropped. + """ + log = logger or logging.getLogger(DEFAULT_LOGGER) + kept: list[Any] = [] + for member in rf_events: + oem = member.get("Oem") if isinstance(member, dict) else None + amd = oem.get("AMD") if isinstance(oem, dict) else None + identifiers = amd.get("AMDFieldIdentifiers") if isinstance(amd, dict) else None + if not isinstance(identifiers, list): + kept.append(member) + continue + usable = [] + for entry in identifiers: + if isinstance(entry, dict) and _usable_serviceable_units(entry.get("ServiceableUnits")): + usable.append(entry) + continue + afid = entry.get("AFID") if isinstance(entry, dict) else None + log.warning( + "Waiving AFID %s from %s: AMDFieldIdentifiers entry has no ServiceableUnits", + afid, + member.get("@odata.id") or member.get("Id") or "", + ) + if len(usable) == len(identifiers): + kept.append(member) + elif usable: + amd_copy = {**cast(dict, amd), "AMDFieldIdentifiers": usable} + kept.append({**member, "Oem": {**cast(dict, oem), "AMD": amd_copy}}) + return kept + + def resolve_hub_events_payload( *, afid_events: list[AfidEvent], rf_events: Optional[list[Any]] = None, prefer_rf_events: bool = True, + logger: Optional[logging.Logger] = None, ) -> list[Any]: """Choose rf_events or aggregated afid_events for hub analyze().""" if prefer_rf_events and rf_events: - return events_payload_for_hub_analyze(rf_events=rf_events) + usable_rf_events = drop_afids_without_serviceable_units(rf_events, logger) + if usable_rf_events: + return events_payload_for_hub_analyze(rf_events=usable_rf_events) return events_payload_for_hub_analyze( afid_events_payload=afid_events_to_entry_point_payload(afid_events) ) @@ -454,6 +513,7 @@ def run_entry_point_hub( rf_event_count: int = 0, raise_on_error: bool = False, prefer_rf_events: bool = True, + logger: Optional[logging.Logger] = None, ) -> ServiceabilityBlock: """Run a registered entry-point service hub and return a :class:`ServiceabilityBlock`.""" if not afid_events and not (prefer_rf_events and rf_events): @@ -468,6 +528,7 @@ def run_entry_point_hub( afid_events=afid_events, rf_events=rf_events, prefer_rf_events=prefer_rf_events, + logger=logger, ) try: raw_result = hub.analyze(events, afid_sag_path) diff --git a/nodescraper/plugins/serviceability/serviceability_recommendations_table.py b/nodescraper/plugins/serviceability/serviceability_recommendations_table.py index 2721e764..9816e95b 100644 --- a/nodescraper/plugins/serviceability/serviceability_recommendations_table.py +++ b/nodescraper/plugins/serviceability/serviceability_recommendations_table.py @@ -45,10 +45,6 @@ ) PRIMARY_RECOMMENDATION_TITLE = "Top Hub service action" -ADDITIONAL_RECOMMENDATIONS_TITLE = "Additional Hub service actions" -ADDITIONAL_RECOMMENDATIONS_NOTE = ( - "Lower-priority follow-on actions if the top service action does not resolve the issue." -) RECOMMENDATION_TABLE_MAX_WIDTHS: dict[str, int] = { "Rank": 4, @@ -197,25 +193,16 @@ def render_serviceability_recommendation_tables_for_plugin_results( def render_serviceability_recommendation_tables(block: ServiceabilityBlock) -> str: - """Render primary and additional Hub recommendation tables.""" - primary, additional = split_recommendation_actions(block) - sections: list[str] = [] - primary_section = render_recommendations_section( + """Render the top Hub recommendation table. + + Lower-ranked actions are not printed; they remain in the serviceability block + saved with the plugin result. + """ + primary, _ = split_recommendation_actions(block) + return render_recommendations_section( _recommendation_section_title(PRIMARY_RECOMMENDATION_TITLE, primary), primary, ) - if primary_section: - sections.append(primary_section) - additional_section = render_recommendations_section( - _recommendation_section_title(ADDITIONAL_RECOMMENDATIONS_TITLE, additional), - additional, - note=ADDITIONAL_RECOMMENDATIONS_NOTE, - ) - if additional_section: - sections.append(additional_section) - if not sections: - return "" - return "\n\n".join(sections) def emit_serviceability_recommendation_tables(block: ServiceabilityBlock) -> None: diff --git a/test/unit/plugin/test_mi4xx_plugin.py b/test/unit/plugin/test_mi4xx_plugin.py index 25964cd6..69c8e463 100644 --- a/test/unit/plugin/test_mi4xx_plugin.py +++ b/test/unit/plugin/test_mi4xx_plugin.py @@ -23,6 +23,7 @@ # SOFTWARE. # ############################################################################### +import json from types import SimpleNamespace from unittest.mock import patch @@ -348,6 +349,38 @@ def test_mi4xx_analyzer_appends_afid_sag_metadata_artifact(system_info, tmp_path assert any(isinstance(artifact, AfidSagMetadataArtifact) for artifact in task.artifacts) +def test_mi4xx_analyzer_writes_top_action_summary_json(system_info, tmp_path): + sag = tmp_path / "afid_sag.json" + sag.write_text("{}", encoding="utf-8") + data = ServiceabilityDataModel( + rf_events=[ + { + "Afid": DUMMY_AFID_A, + "ServiceableUnit": DUMMY_UNIT_A, + "Created": DUMMY_TIMESTAMP, + } + ] + ) + fake_ep = SimpleNamespace(name="afse", load=lambda: _FakeHub) + analyzer = MI4XXAnalyzer(system_info=system_info) + with patch( + "nodescraper.plugins.serviceability.se_runner._entry_points_for_group", + return_value=[fake_ep], + ): + task = analyzer.analyze_data( + data, + Mi4xxServiceabilityAnalyzerArgs(afid_sag_path=str(sag)), + ) + log_dir = tmp_path / "mi4xx_analyzer" + task.log_result(str(log_dir)) + + summary = json.loads((log_dir / "summary.json").read_text(encoding="utf-8")) + assert summary["system_name"] == system_info.name + assert summary["afid_event_count"] == 1 + assert [action["afid"] for action in summary["top_service_actions"]] == [DUMMY_AFID_A] + assert summary["top_service_actions"][0]["rank"] == 1 + + def test_mi4xx_plugin_analyzes_offline_data_without_collection(system_info, tmp_path): sag = tmp_path / "afid_sag.json" sag.write_text("{}", encoding="utf-8") diff --git a/test/unit/plugin/test_se_runner.py b/test/unit/plugin/test_se_runner.py index d355d814..2b516a91 100644 --- a/test/unit/plugin/test_se_runner.py +++ b/test/unit/plugin/test_se_runner.py @@ -24,6 +24,7 @@ # ############################################################################### import json +import logging from pathlib import Path from types import SimpleNamespace from typing import Any @@ -195,6 +196,63 @@ def test_resolve_hub_events_payload_honors_prefer_rf_events_false(): assert payload[0]["location"] == EXAMPLE_EVENTS[0].serviceable_unit +def _rf_member(odata_id, *identifiers): + return {"@odata.id": odata_id, "Oem": {"AMD": {"AMDFieldIdentifiers": list(identifiers)}}} + + +_UNIT = {"@odata.id": "/redfish/v1/Chassis/Instinct_EAM_0"} + + +def test_resolve_hub_events_payload_waives_afid_without_serviceable_units(caplog): + from nodescraper.plugins.serviceability.se_runner import ( + resolve_hub_events_payload, + ) + + good = _rf_member("/entries/1", {"AFID": 11101, "ServiceableUnits": [_UNIT]}) + bad = _rf_member("/entries/2", {"AFID": 11045, "ServiceableUnits": []}) + with caplog.at_level(logging.WARNING): + payload = resolve_hub_events_payload( + afid_events=EXAMPLE_EVENTS, rf_events=[good, bad], prefer_rf_events=True + ) + assert payload == [good] + assert "Waiving AFID 11045 from /entries/2" in caplog.text + + +def test_resolve_hub_events_payload_keeps_valid_entries_on_mixed_member(caplog): + from nodescraper.plugins.serviceability.se_runner import ( + resolve_hub_events_payload, + ) + + member = _rf_member( + "/entries/3", + {"AFID": 11101, "ServiceableUnits": [_UNIT]}, + {"AFID": 11045}, + {"AFID": 11046, "ServiceableUnits": [{"Name": "no odata id"}]}, + ) + with caplog.at_level(logging.WARNING): + payload = resolve_hub_events_payload( + afid_events=EXAMPLE_EVENTS, rf_events=[member], prefer_rf_events=True + ) + identifiers = payload[0]["Oem"]["AMD"]["AMDFieldIdentifiers"] + assert [entry["AFID"] for entry in identifiers] == [11101] + assert len(member["Oem"]["AMD"]["AMDFieldIdentifiers"]) == 3 + assert "Waiving AFID 11045" in caplog.text + assert "Waiving AFID 11046" in caplog.text + + +def test_resolve_hub_events_payload_falls_back_to_afid_events_when_all_waived(): + from nodescraper.plugins.serviceability.se_runner import ( + resolve_hub_events_payload, + ) + + payload = resolve_hub_events_payload( + afid_events=EXAMPLE_EVENTS[:1], + rf_events=[_rf_member("/entries/4", {"AFID": 11045})], + prefer_rf_events=True, + ) + assert payload[0]["afid"] == EXAMPLE_EVENTS[0].afid + + def test_resolved_hub_options_explicit_fields_override_options_bag(): args = ServiceabilityAnalyzerArgs( hub_python_module="dummy.test.module", diff --git a/test/unit/plugin/test_serviceability_recommendations_table.py b/test/unit/plugin/test_serviceability_recommendations_table.py index 12ad35b6..4616a704 100644 --- a/test/unit/plugin/test_serviceability_recommendations_table.py +++ b/test/unit/plugin/test_serviceability_recommendations_table.py @@ -110,7 +110,7 @@ def test_split_recommendation_rows_uses_hub_top(): assert additional[0].afid == DUMMY_AFID_B -def test_render_serviceability_recommendation_tables_splits_sections(): +def test_render_serviceability_recommendation_tables_prints_only_top_action(): block = ServiceabilityBlock( hub_top_results=[ _row( @@ -142,10 +142,9 @@ def test_render_serviceability_recommendation_tables_splits_sections(): output = render_serviceability_recommendation_tables(block) assert "Top Hub service action (rank 1)" in output - assert "Additional Hub service actions (rank 2)" in output assert f"{DUMMY_AFID_A}: Contact Support" in output - assert f"{DUMMY_AFID_B}: Update FW" in output - assert output.index("Top Hub service action") < output.index("Additional Hub service actions") + assert "Additional Hub service actions" not in output + assert f"{DUMMY_AFID_B}: Update FW" not in output def test_build_top_service_actions_includes_tied_entries(): From d38f6946914e743ae340aa8e7938576d9fa39032 Mon Sep 17 00:00:00 2001 From: etwong_amdeng Date: Fri, 30 Oct 2026 18:02:49 -0400 Subject: [PATCH 2/4] AFID_SAG preflight check --- .../plugins/serviceability/analysis_window.py | 13 +- .../mi4xx/serviceability_plugin_mi4xx.py | 17 +++ .../plugins/serviceability/se_runner.py | 113 +++++++++++++++++- test/unit/plugin/test_mi4xx_plugin.py | 50 ++++++++ test/unit/plugin/test_se_runner.py | 92 ++++++++++++++ 5 files changed, 281 insertions(+), 4 deletions(-) diff --git a/nodescraper/plugins/serviceability/analysis_window.py b/nodescraper/plugins/serviceability/analysis_window.py index e9eab36e..d0721f1f 100644 --- a/nodescraper/plugins/serviceability/analysis_window.py +++ b/nodescraper/plugins/serviceability/analysis_window.py @@ -35,7 +35,12 @@ from .analyzer_args import ServiceabilityAnalyzerArgs from .cper_decode import CperDecodeError, decode_cper_raw_attachments from .se_models import AfidEvent, ServiceabilityBlock -from .se_runner import HubRunError, run_entry_point_hub, run_service_hub +from .se_runner import ( + HubRunError, + resolve_runtime_afid_sag_path, + run_entry_point_hub, + run_service_hub, +) from .serviceability_data import ServiceabilityDataModel @@ -161,7 +166,11 @@ def analyze_serviceability_window( error="empty afid_events", ) - sag_path = args.resolved_afid_sag_path() + sag_path = ( + resolve_runtime_afid_sag_path(args, log) + if args.uses_entry_point_hub() + else args.resolved_afid_sag_path() + ) data.afid_sag_path = sag_path log.info( "(%s) Using AFID_SAG file: %s", diff --git a/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py b/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py index 4341f5d4..a21b5cc3 100644 --- a/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py +++ b/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py @@ -32,6 +32,7 @@ from nodescraper.enums import EventPriority, ExecutionStatus, SystemInteractionLevel from nodescraper.models import DataPluginResult, PluginResult, TaskResult from nodescraper.plugins.serviceability.afid_sag_lookup import log_afid_fru_summary +from nodescraper.plugins.serviceability.se_runner import HubRunError, preflight_afid_sag from nodescraper.plugins.serviceability.serviceability_data import ( ServiceabilityDataLoadError, ServiceabilityDataModel, @@ -198,6 +199,22 @@ def run( # type: ignore[override] analysis_args, ) + if analysis: + try: + parsed_analysis_args = ( + analysis_args + if isinstance(analysis_args, Mi4xxServiceabilityAnalyzerArgs) + else Mi4xxServiceabilityAnalyzerArgs(**(analysis_args or {})) + ) + if not parsed_analysis_args.skip_hub: + collection_args, analysis_args = self._merge_afid_sag_path( + preflight_afid_sag(parsed_analysis_args, self.logger), + collection_args, + analysis_args, + ) + except (HubRunError, ValueError) as exc: + return self._plugin_error_result(f"AFID_SAG preflight failed: {exc}") + if analysis and not collection and data is not None: try: loaded = ( diff --git a/nodescraper/plugins/serviceability/se_runner.py b/nodescraper/plugins/serviceability/se_runner.py index d3059260..86789312 100644 --- a/nodescraper/plugins/serviceability/se_runner.py +++ b/nodescraper/plugins/serviceability/se_runner.py @@ -33,17 +33,20 @@ import inspect import logging from pathlib import Path -from typing import Any, Callable, Optional, Protocol, Type, Union, cast +from typing import TYPE_CHECKING, Any, Callable, Optional, Protocol, Type, Union, cast from nodescraper.constants import DEFAULT_LOGGER -from .afid_sag_paths import validate_afid_sag_path +from .afid_sag_paths import default_afid_sag_path, validate_afid_sag_path from .se_adapter import ( serviceability_block_from_entry_point_hub, serviceability_block_from_service_result, ) from .se_models import AfidEvent, ServiceabilityBlock +if TYPE_CHECKING: + from .analyzer_args import ServiceabilityAnalyzerArgs + HUB_ENTRY_POINT_GROUP = "amd.serviceability_engines" @@ -503,6 +506,107 @@ def _hub_analyze_response_from_raw(raw_result: Any) -> dict[str, Any]: return dataclasses.asdict(raw_result) +def _response_field(result: Any, name: str) -> Any: + """Read a field from an AFSE TriageResponse dataclass or a dict payload.""" + if isinstance(result, dict): + return result.get(name) + return getattr(result, name, None) + + +def validate_afid_sag_with_hub(hub: Any, sag_path: str) -> bool: + """Ask the loaded hub to validate the AFID SAG file via its validate() API. + + Args: + hub: Loaded entry-point hub (AFSE). + sag_path: AFID SAG file to validate. + + Returns: + True when the hub validated the file, False when the hub has no validate(). + + Raises: + HubRunError: When validate() raises or reports a status other than ok. + """ + validate = getattr(hub, "validate", None) + if not callable(validate): + return False + try: + result = validate(sag_path) + except Exception as exc: # noqa: BLE001 + raise HubRunError(f"AFSE SAG validation failed: {exc}") from exc + status = str(_response_field(result, "status") or "").strip().lower() + if status == "ok": + return True + error = _response_field(result, "error") + code = _response_field(error, "code") if error is not None else None + message = _response_field(error, "message") if error is not None else None + detail = str(message or code or status or "unknown error") + raise HubRunError(f"AFSE SAG validation failed: {detail}") + + +def resolve_runtime_afid_sag_path( + args: ServiceabilityAnalyzerArgs, + logger: Optional[logging.Logger] = None, +) -> str: + """Return the AFID SAG file analysis should open. + + A configured afid_sag_path wins. Otherwise the loaded entry-point hub is asked for its + default_sag_path(), falling back to the built-in /opt/amd/afid/AFID_SAG.json. + + Args: + args: Analyzer args for this run. + logger: Logger for the "path omitted" line. Defaults to the nodescraper logger. + + Returns: + SAG path to use for analysis. + """ + if args.afid_sag_path and str(args.afid_sag_path).strip(): + return str(args.afid_sag_path).strip() + hub: Any = None + if args.uses_entry_point_hub(): + try: + hub = load_hub_from_entry_point(args.resolved_hub_entry_point()) + except (HubRunError, ValueError): + hub = None + path = default_afid_sag_path() + lookup = getattr(hub, "default_sag_path", None) + if callable(lookup): + found = lookup() + if found is not None and str(found).strip(): + path = str(found).strip() + (logger or logging.getLogger(DEFAULT_LOGGER)).info( + "AFID_SAG path omitted; using default: %s", path + ) + return path + + +def preflight_afid_sag( + args: ServiceabilityAnalyzerArgs, + logger: Optional[logging.Logger] = None, +) -> str: + """Resolve the AFID SAG file and check it with AFSE before any collection runs. + + Args: + args: Analyzer args for this run (entry-point hub). + logger: Logger for the OK lines. Defaults to the nodescraper logger. + + Returns: + Resolved absolute SAG path. + + Raises: + HubRunError: When the file is missing, the hub cannot load, or AFSE rejects the file. + """ + log = logger or logging.getLogger(DEFAULT_LOGGER) + sag_path = resolve_runtime_afid_sag_path(args, log) + validate_afid_sag_path(sag_path) + resolved = str(Path(sag_path).expanduser().resolve()) + log.info("AFID_SAG file OK: %s", resolved) + hub_name = args.resolved_hub_entry_point() + hub = load_hub_from_entry_point(hub_name) + if validate_afid_sag_with_hub(hub, resolved): + log.info("AFID_SAG validated by %s: %s", getattr(hub, "name", hub_name), resolved) + return resolved + + def run_entry_point_hub( *, hub_entry_point: str, @@ -519,9 +623,14 @@ def run_entry_point_hub( if not afid_events and not (prefer_rf_events and rf_events): raise HubRunError("No AFID events to analyze") + log = logger or logging.getLogger(DEFAULT_LOGGER) validate_afid_sag_path(afid_sag_path) + resolved_sag = Path(afid_sag_path).expanduser().resolve() + log.debug("AFID_SAG file OK: %s", resolved_sag) label = hub_display_name or normalize_hub_entry_point(hub_entry_point) hub = load_hub_from_entry_point(hub_entry_point) + if validate_afid_sag_with_hub(hub, str(resolved_sag)): + log.debug("AFID_SAG validated by %s: %s", getattr(hub, "name", label), resolved_sag) hub_label = getattr(hub, "name", label) engine_version = _hub_engine_version(hub) events = resolve_hub_events_payload( diff --git a/test/unit/plugin/test_mi4xx_plugin.py b/test/unit/plugin/test_mi4xx_plugin.py index 69c8e463..76296ff8 100644 --- a/test/unit/plugin/test_mi4xx_plugin.py +++ b/test/unit/plugin/test_mi4xx_plugin.py @@ -411,6 +411,56 @@ def test_mi4xx_plugin_analyzes_offline_data_without_collection(system_info, tmp_ assert result.result_data.analysis_result.status == ExecutionStatus.OK +def test_mi4xx_plugin_missing_sag_stops_before_collection(system_info, tmp_path): + fake_ep = SimpleNamespace(name="afse", load=lambda: _FakeHub) + plugin = Mi4xxServiceabilityPlugin(system_info=system_info) + with ( + patch( + "nodescraper.plugins.serviceability.se_runner._entry_points_for_group", + return_value=[fake_ep], + ), + patch.object(MI4XXCollector, "collect_data") as collect_data, + ): + result = plugin.run( + collection=True, + analysis=True, + analysis_args=Mi4xxServiceabilityAnalyzerArgs( + afid_sag_path=str(tmp_path / "missing_sag.json") + ), + ) + collect_data.assert_not_called() + assert result.status == ExecutionStatus.ERROR + assert "AFID_SAG preflight failed: AFID SAG file not found" in result.message + assert result.result_data.collection_result.status == ExecutionStatus.NOT_RAN + + +def test_mi4xx_plugin_sag_rejected_by_afse_stops_before_collection(system_info, tmp_path): + sag = tmp_path / "afid_sag.json" + sag.write_text("{}", encoding="utf-8") + + class _RejectingHub(_FakeHub): + def validate(self, afid_sag_path): + return {"status": "error", "error": {"message": "schema mismatch"}} + + fake_ep = SimpleNamespace(name="afse", load=lambda: _RejectingHub) + plugin = Mi4xxServiceabilityPlugin(system_info=system_info) + with ( + patch( + "nodescraper.plugins.serviceability.se_runner._entry_points_for_group", + return_value=[fake_ep], + ), + patch.object(MI4XXCollector, "collect_data") as collect_data, + ): + result = plugin.run( + collection=True, + analysis=True, + analysis_args=Mi4xxServiceabilityAnalyzerArgs(afid_sag_path=str(sag)), + ) + collect_data.assert_not_called() + assert result.status == ExecutionStatus.ERROR + assert "schema mismatch" in result.message + + def test_load_hub_from_entry_point_missing_raises(): with patch( "nodescraper.plugins.serviceability.se_runner._entry_points_for_group", diff --git a/test/unit/plugin/test_se_runner.py b/test/unit/plugin/test_se_runner.py index 2b516a91..4fd86a08 100644 --- a/test/unit/plugin/test_se_runner.py +++ b/test/unit/plugin/test_se_runner.py @@ -667,3 +667,95 @@ def test_mi3xx_analyzer_writes_serviceability_json(tmp_path, system_info): data.log_model(str(tmp_path)) payload = json.loads((tmp_path / "serviceability.json").read_text(encoding="utf-8")) assert payload["afid_events"][0]["afid"] == DUMMY_AFID_A + + +class _ValidatingHub: + name = "AFSE" + + def __init__(self, response=None, default_path=None): + self._response = response if response is not None else {"status": "ok"} + self._default_path = default_path + self.validated: list[str] = [] + + def validate(self, afid_sag_path): + self.validated.append(afid_sag_path) + return self._response + + def default_sag_path(self): + return self._default_path + + +def test_validate_afid_sag_with_hub_accepts_ok_status(tmp_path): + from nodescraper.plugins.serviceability.se_runner import validate_afid_sag_with_hub + + hub = _ValidatingHub() + assert validate_afid_sag_with_hub(hub, str(tmp_path / "sag.json")) is True + assert hub.validated == [str(tmp_path / "sag.json")] + + +def test_validate_afid_sag_with_hub_raises_on_error_status(): + from nodescraper.plugins.serviceability.se_runner import ( + HubRunError, + validate_afid_sag_with_hub, + ) + + hub = _ValidatingHub({"status": "error", "error": {"code": "E1", "message": "bad schema"}}) + with pytest.raises(HubRunError, match="AFSE SAG validation failed: bad schema"): + validate_afid_sag_with_hub(hub, "/tmp/sag.json") + + +def test_validate_afid_sag_with_hub_skips_hub_without_validate(): + from nodescraper.plugins.serviceability.se_runner import validate_afid_sag_with_hub + + assert validate_afid_sag_with_hub(SimpleNamespace(name="Other"), "/tmp/sag.json") is False + + +def test_resolve_runtime_afid_sag_path_prefers_configured_path(tmp_path): + from nodescraper.plugins.serviceability.se_runner import ( + resolve_runtime_afid_sag_path, + ) + + args = ServiceabilityAnalyzerArgs( + hub_entry_point="afse", afid_sag_path=str(tmp_path / "x.json") + ) + assert resolve_runtime_afid_sag_path(args) == str(tmp_path / "x.json") + + +def test_resolve_runtime_afid_sag_path_uses_hub_default(monkeypatch, tmp_path): + from nodescraper.plugins.serviceability import se_runner + + hub_default = str(tmp_path / "hub_default.json") + monkeypatch.setattr( + se_runner, + "load_hub_from_entry_point", + lambda name: _ValidatingHub(default_path=hub_default), + ) + args = ServiceabilityAnalyzerArgs(hub_entry_point="afse") + assert se_runner.resolve_runtime_afid_sag_path(args) == hub_default + + +def test_resolve_runtime_afid_sag_path_falls_back_to_builtin_default(monkeypatch): + from nodescraper.plugins.serviceability import se_runner + from nodescraper.plugins.serviceability.afid_sag_paths import DEFAULT_AFID_SAG_PATH + + monkeypatch.setattr( + se_runner, "load_hub_from_entry_point", lambda name: SimpleNamespace(name="AFSE") + ) + args = ServiceabilityAnalyzerArgs(hub_entry_point="afse") + assert se_runner.resolve_runtime_afid_sag_path(args) == DEFAULT_AFID_SAG_PATH + + +def test_run_entry_point_hub_stops_when_afse_rejects_sag(monkeypatch, tmp_path): + from nodescraper.plugins.serviceability import se_runner + + sag = tmp_path / "afid_sag.json" + sag.write_text("{}", encoding="utf-8") + hub = _ValidatingHub({"status": "error", "error": {"message": "schema mismatch"}}) + monkeypatch.setattr(se_runner, "load_hub_from_entry_point", lambda name: hub) + with pytest.raises(se_runner.HubRunError, match="schema mismatch"): + se_runner.run_entry_point_hub( + hub_entry_point="afse", + afid_events=EXAMPLE_EVENTS[:1], + afid_sag_path=str(sag), + ) + assert hub.validated == [str(sag.resolve())] From 2b1e4aa7d93e9b254c4aff7d74d4cfe916105b5d Mon Sep 17 00:00:00 2001 From: Alexandra Bara Date: Mon, 2 Nov 2026 08:34:42 -0600 Subject: [PATCH 3/4] fixed for diff shapes of AFIDs --- .../plugins/serviceability/afid_events.py | 249 ++++++++++++++++-- .../serviceability/mi4xx/mi4xx_analyzer.py | 36 +-- .../mi4xx/serviceability_plugin_mi4xx.py | 64 ++--- .../plugins/serviceability/se_runner.py | 227 +++++++++++++--- .../serviceability/serviceability_api.py | 29 ++ .../serviceability_plugin_base.py | 95 ++++++- .../plugin/test_afid_events_bmc_schema.py | 104 ++++++++ test/unit/plugin/test_mi4xx_plugin.py | 1 + 8 files changed, 679 insertions(+), 126 deletions(-) diff --git a/nodescraper/plugins/serviceability/afid_events.py b/nodescraper/plugins/serviceability/afid_events.py index 645d9177..6261f262 100644 --- a/nodescraper/plugins/serviceability/afid_events.py +++ b/nodescraper/plugins/serviceability/afid_events.py @@ -98,12 +98,32 @@ def _afid_event_from_cper_slot(unit: str, payload: Any) -> Optional[AfidEvent]: ) +def _oem_mappings(payload: dict[str, Any]) -> list[dict[str, Any]]: + """Return Oem objects on a log entry, including one nested under CPER. + + Args: + payload: Redfish log member or switch event. + + Returns: + Oem mappings that may contain AMDFieldIdentifiers. + """ + found: list[dict[str, Any]] = [] + oem = payload.get("Oem") + if isinstance(oem, dict): + found.append(oem) + cper = payload.get("CPER") + if isinstance(cper, dict): + nested = cper.get("Oem") + if isinstance(nested, dict): + found.append(nested) + return found + + def _extract_afid(payload: dict[str, Any]) -> Optional[int]: for key in _AFID_KEYS: if key in payload and payload[key] is not None: return int(payload[key]) - oem = payload.get("Oem") - if isinstance(oem, dict): + for oem in _oem_mappings(payload): for vendor_payload in oem.values(): found = _extract_afid_from_oem_fragment(vendor_payload) if found is not None: @@ -148,6 +168,148 @@ def _origin_dict_to_unit(value: Any) -> Optional[str]: return None +def _unit_odata_id(unit: Any) -> Optional[str]: + if not isinstance(unit, dict): + return None + for key in ("@odata.id", "odata.id"): + value = unit.get(key) + if isinstance(value, str) and value.strip(): + return value + return None + + +def usable_serviceable_units(units: Any) -> list[Any]: + """Return ServiceableUnits entries that carry an @odata.id or odata.id string. + + Args: + units: ServiceableUnits value from an AFID identifier entry. + + Returns: + Unit objects that name a location. Empty when none qualify. + """ + if not isinstance(units, list): + return [] + return [unit for unit in units if _unit_odata_id(unit) is not None] + + +def promote_serviceable_unit_odata_ids(member: dict[str, Any]) -> dict[str, Any]: + """Copy odata.id onto @odata.id so the hub can read the location. + + Args: + member: Redfish log member. + + Returns: + The same member, or a copy when a ServiceableUnits entry needed @odata.id. + """ + oem = member.get("Oem") + if not isinstance(oem, dict): + return member + amd = oem.get("AMD") + if not isinstance(amd, dict): + return member + identifiers = amd.get("AMDFieldIdentifiers") + if not isinstance(identifiers, list): + return member + new_identifiers: list[Any] = [] + changed = False + for entry in identifiers: + if not isinstance(entry, dict) or not isinstance(entry.get("ServiceableUnits"), list): + new_identifiers.append(entry) + continue + new_units: list[Any] = [] + entry_changed = False + for unit in entry["ServiceableUnits"]: + if ( + isinstance(unit, dict) + and not isinstance(unit.get("@odata.id"), str) + and isinstance(unit.get("odata.id"), str) + and unit["odata.id"].strip() + ): + copied = dict(unit) + copied["@odata.id"] = unit["odata.id"] + new_units.append(copied) + entry_changed = True + else: + new_units.append(unit) + if entry_changed: + changed = True + new_identifiers.append({**entry, "ServiceableUnits": new_units}) + else: + new_identifiers.append(entry) + if not changed: + return member + return { + **member, + "Oem": {**oem, "AMD": {**amd, "AMDFieldIdentifiers": new_identifiers}}, + } + + +def amd_field_identifiers_with_units( + oem: Any, +) -> Optional[tuple[list[dict[str, Any]], list[Any]]]: + """Split Oem.AMD.AMDFieldIdentifiers by whether ServiceableUnits is usable. + + Args: + oem: Redfish Oem object from a log member. + + Returns: + (usable entries, unusable entries) when Oem.AMD.AMDFieldIdentifiers is a list. + None when that shape is absent so another source shape can be tried. + """ + if not isinstance(oem, dict): + return None + amd = oem.get("AMD") + if not isinstance(amd, dict): + return None + identifiers = amd.get("AMDFieldIdentifiers") + if not isinstance(identifiers, list): + return None + usable: list[dict[str, Any]] = [] + unusable: list[Any] = [] + for entry in identifiers: + units = entry.get("ServiceableUnits") if isinstance(entry, dict) else None + if isinstance(entry, dict) and usable_serviceable_units(units): + usable.append(entry) + else: + unusable.append(entry) + return usable, unusable + + +_LOCATION_EXTRACTORS = (amd_field_identifiers_with_units,) + + +def split_afid_location_entries( + oem: Any, +) -> Optional[tuple[list[dict[str, Any]], list[Any]]]: + """Return usable and unusable AFID entries for the first matching source shape. + + Args: + oem: Redfish Oem object from a log member. + + Returns: + (usable, unusable) for a recognized shape, or None when no shape applies. + """ + for extractor in _LOCATION_EXTRACTORS: + split = extractor(oem) + if split is not None: + return split + return None + + +def _first_unit_from_identifiers(identifiers: Any) -> Optional[str]: + if not isinstance(identifiers, list): + return None + for item in identifiers: + if not isinstance(item, dict): + continue + units = item.get("ServiceableUnits") or item.get("serviceable_units") + for unit in usable_serviceable_units(units): + name = _origin_dict_to_unit(unit) + if name: + return name + return None + + def _extract_serviceable_unit(payload: dict[str, Any]) -> Optional[str]: for key in ("serviceable_unit", "ServiceableUnit", "OriginOfCondition", "Device"): value = payload.get(key) @@ -172,45 +334,78 @@ def _extract_serviceable_unit(payload: dict[str, Any]) -> Optional[str]: if unit: return unit - oem = payload.get("Oem") - if isinstance(oem, dict): - for vendor_payload in oem.values(): + for oem in _oem_mappings(payload): + for vendor_name, vendor_payload in oem.items(): if isinstance(vendor_payload, dict): unit = vendor_payload.get("serviceable_unit") or vendor_payload.get( "ServiceableUnit" ) if unit is not None and str(unit).strip(): return str(unit).strip() - identifiers = vendor_payload.get("AMDFieldIdentifiers") - if isinstance(identifiers, list): - for item in identifiers: - if not isinstance(item, dict): - continue - su = item.get("ServiceableUnits") or item.get("serviceable_units") - if isinstance(su, list) and su: - u = _origin_dict_to_unit(su[0]) - if u: - return u - elif isinstance(vendor_payload, list): - for item in vendor_payload: - if not isinstance(item, dict): - continue - su = item.get("ServiceableUnits") or item.get("serviceable_units") - if isinstance(su, list) and su: - u = _origin_dict_to_unit(su[0]) - if u: - return u + if vendor_name != "AMD": + unit = _first_unit_from_identifiers(vendor_payload.get("AMDFieldIdentifiers")) + if unit: + return unit + else: + unit = _first_unit_from_identifiers(vendor_payload) + if unit: + return unit + split = amd_field_identifiers_with_units(oem) + if split is not None: + unit = _first_unit_from_identifiers(split[0]) + if unit: + return unit + return _switch_serviceable_unit(payload) + + +def _switch_serviceable_unit(payload: dict[str, Any]) -> Optional[str]: + """Return the component name carried by one switch event. + + Args: + payload: Switch event object. + + Returns: + object-ref name, unit id, or node name, or None when this is not a switch event. + """ + if payload.get("kind") != "Event": + return None + object_ref = payload.get("object-ref") + if not isinstance(object_ref, dict): + object_ref = payload.get("object_ref") + if isinstance(object_ref, dict): + name = object_ref.get("name") + if isinstance(name, str) and name.strip(): + return name.strip() + source = payload.get("source") + if not isinstance(source, dict): + return None + for key in ("unit-id", "node-name"): + value = source.get(key) + if isinstance(value, str) and value.strip(): + return value.strip() return None def _extract_timestamp(payload: dict[str, Any]) -> Optional[str]: for key in _EVENT_TIMESTAMP_KEYS: - value = payload.get(key) - if value is not None and str(value).strip(): - return str(value).strip() + text = _timestamp_text(payload.get(key)) + if text: + return text + meta = payload.get("meta") + if isinstance(meta, dict): + for key in ("creation-time", "mod-time"): + text = _timestamp_text(meta.get(key)) + if text: + return text return None +def _timestamp_text(value: Any) -> str: + if value is None: + return "" + return str(value).replace("\x00", "").strip() + + def _unit_from_odata_id(odata_id: str) -> str: segment = odata_id.rstrip("/").split("/")[-1] return segment or odata_id diff --git a/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py b/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py index 61159a1c..91613bcb 100644 --- a/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py +++ b/nodescraper/plugins/serviceability/mi4xx/mi4xx_analyzer.py @@ -27,13 +27,10 @@ import json from datetime import datetime, timezone -from typing import Any from nodescraper.connection.inband import TextFileArtifact from nodescraper.plugins.serviceability.se_models import ServiceabilityBlock -from nodescraper.plugins.serviceability.serviceability_api import ( - split_recommendation_actions, -) +from nodescraper.plugins.serviceability.serviceability_api import top_action_summary from nodescraper.plugins.serviceability.serviceability_hub_analyzer import ( ServiceabilityHubAnalyzer, ) @@ -57,28 +54,13 @@ def _append_hub_artifacts(self, block: ServiceabilityBlock) -> None: self.result.artifacts.append( TextFileArtifact( filename=SUMMARY_FILENAME, - contents=json.dumps(self._top_action_summary(block), indent=2), + contents=json.dumps( + top_action_summary( + block, + system_name=self.system_info.name, + analyzed_at=datetime.now(timezone.utc).isoformat(), + ), + indent=2, + ), ) ) - - def _top_action_summary(self, block: ServiceabilityBlock) -> dict[str, Any]: - """Build the summary.json payload for the top Hub service action. - - Args: - block: Serviceability block returned by the hub. - - Returns: - System name, analysis time, hub and SAG versions, AFID event count, and the - top service action(s). Ties for the top rank are all included. - """ - top, _ = split_recommendation_actions(block) - return { - "system_name": self.system_info.name, - "analyzed_at": datetime.now(timezone.utc).isoformat(), - "hub_version": block.hub_version, - "afid_sag_file_version": block.afid_sag_file_version, - "afid_event_count": len(block.afid_events), - "top_service_actions": [ - action.model_dump(mode="json", exclude_none=True) for action in top - ], - } diff --git a/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py b/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py index 30a1d1e6..ab542dd3 100644 --- a/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py +++ b/nodescraper/plugins/serviceability/mi4xx/serviceability_plugin_mi4xx.py @@ -25,14 +25,13 @@ ############################################################################### from __future__ import annotations -from typing import Annotated, Any, Optional, Union +from typing import Annotated, Any, Optional, Union, cast from pydantic import Field -from nodescraper.enums import EventPriority, ExecutionStatus, SystemInteractionLevel -from nodescraper.models import DataPluginResult, PluginResult, TaskResult +from nodescraper.enums import EventPriority, SystemInteractionLevel +from nodescraper.models import PluginResult from nodescraper.plugins.serviceability.afid_sag_lookup import log_afid_fru_summary -from nodescraper.plugins.serviceability.se_runner import HubRunError, preflight_afid_sag from nodescraper.plugins.serviceability.serviceability_data import ( ServiceabilityDataLoadError, ServiceabilityDataModel, @@ -141,25 +140,22 @@ def _merge_mi4xx_collection_args( return collection_args.model_copy(update={"rf_event_log_uri": uri_text}) return collection_args - def _plugin_error_result(self, message: str) -> PluginResult: - self.logger.error("(%s) %s", self.__class__.__name__, message) - return PluginResult( - status=ExecutionStatus.ERROR, - source=self.__class__.__name__, - message=message, - result_data=DataPluginResult( - collection_result=TaskResult( - status=ExecutionStatus.NOT_RAN, - parent=self.__class__.__name__, - message="Data collection skipped", - ), - analysis_result=TaskResult( - status=ExecutionStatus.ERROR, - parent=self.__class__.__name__, - message=message, - ), - ), - ) + def _apply_afid_sag_path_to_collection_args( + self, + afid_sag_path: str, + collection_args: Any, + ) -> Any: + """Copy a resolved AFID_SAG path onto MI4xx collector args. + + Args: + afid_sag_path: Absolute path returned by preflight. + collection_args: MI4xx collector args for this run, or None. + + Returns: + Collector args with afid_sag_path set. + """ + merged_collection, _ = self._merge_afid_sag_path(afid_sag_path, collection_args, None) + return merged_collection def run( # type: ignore[override] self, @@ -200,20 +196,14 @@ def run( # type: ignore[override] ) if analysis: - try: - parsed_analysis_args = ( - analysis_args - if isinstance(analysis_args, Mi4xxServiceabilityAnalyzerArgs) - else Mi4xxServiceabilityAnalyzerArgs(**(analysis_args or {})) - ) - if not parsed_analysis_args.skip_hub: - collection_args, analysis_args = self._merge_afid_sag_path( - preflight_afid_sag(parsed_analysis_args, self.logger), - collection_args, - analysis_args, - ) - except (HubRunError, ValueError) as exc: - return self._plugin_error_result(f"AFID_SAG preflight failed: {exc}") + preflight = self._preflight_hub_sag(collection_args, analysis_args) + if isinstance(preflight, PluginResult): + return preflight + collection_args, updated_analysis = preflight + analysis_args = cast( + Optional[Union[Mi4xxServiceabilityAnalyzerArgs, dict[str, Any]]], + updated_analysis, + ) if analysis and not collection and data is not None: try: diff --git a/nodescraper/plugins/serviceability/se_runner.py b/nodescraper/plugins/serviceability/se_runner.py index 86789312..1d1682cb 100644 --- a/nodescraper/plugins/serviceability/se_runner.py +++ b/nodescraper/plugins/serviceability/se_runner.py @@ -32,11 +32,13 @@ import importlib.metadata import inspect import logging +import re from pathlib import Path from typing import TYPE_CHECKING, Any, Callable, Optional, Protocol, Type, Union, cast from nodescraper.constants import DEFAULT_LOGGER +from .afid_events import promote_serviceable_unit_odata_ids, split_afid_location_entries from .afid_sag_paths import default_afid_sag_path, validate_afid_sag_path from .se_adapter import ( serviceability_block_from_entry_point_hub, @@ -48,6 +50,11 @@ from .analyzer_args import ServiceabilityAnalyzerArgs HUB_ENTRY_POINT_GROUP = "amd.serviceability_engines" +_MIN_AFSE_VERSION = (0, 2) +# Leading major.minor. A patch, pre-release, or local .dev suffix is ignored. +_AFSE_VERSION_RE = re.compile(r"^v?(?P\d+)\.(?P\d+)") +# setuptools_scm no-guess-dev with no ancestor tag: 0.0.post1.devN+gHASH +_UNTAGGED_AFSE_RE = re.compile(r"^v?0\.0(?:\.post\d+)?\.dev\d+(?:\+[0-9A-Za-z.]+)?$") def normalize_hub_entry_point(hub_name: str) -> str: @@ -298,13 +305,17 @@ def events_payload_for_hub_analyze( raise HubRunError("rf_events or afid_events is required") -def _usable_serviceable_units(units: Any) -> list[Any]: - """Return ServiceableUnits entries that carry an '@odata.id' string.""" - if not isinstance(units, list): - return [] - return [ - unit for unit in units if isinstance(unit, dict) and isinstance(unit.get("@odata.id"), str) - ] +def _redfish_severity_is_ok(member: dict[str, Any]) -> bool: + """Return whether a Redfish log member is an OK event. + + Args: + member: Redfish log member. + + Returns: + True when Severity is OK. + """ + severity = member.get("Severity") + return isinstance(severity, str) and severity.strip().upper() == "OK" def drop_afids_without_serviceable_units( @@ -314,7 +325,8 @@ def drop_afids_without_serviceable_units( """Waive AMDFieldIdentifiers entries the hub would reject for lacking ServiceableUnits. The entry-point hub rejects the whole event list when any Oem.AMD.AMDFieldIdentifiers - entry has no ServiceableUnits, so drop those entries (with a warning) before analysis. + entry has no ServiceableUnits, so drop those entries before analysis. OK events are + dropped quietly. Any other severity is logged. Args: rf_events: Raw Redfish log members. @@ -327,27 +339,29 @@ def drop_afids_without_serviceable_units( kept: list[Any] = [] for member in rf_events: oem = member.get("Oem") if isinstance(member, dict) else None - amd = oem.get("AMD") if isinstance(oem, dict) else None - identifiers = amd.get("AMDFieldIdentifiers") if isinstance(amd, dict) else None - if not isinstance(identifiers, list): + split = split_afid_location_entries(oem) + if split is None: kept.append(member) continue - usable = [] - for entry in identifiers: - if isinstance(entry, dict) and _usable_serviceable_units(entry.get("ServiceableUnits")): - usable.append(entry) - continue - afid = entry.get("AFID") if isinstance(entry, dict) else None - log.warning( - "Waiving AFID %s from %s: AMDFieldIdentifiers entry has no ServiceableUnits", - afid, - member.get("@odata.id") or member.get("Id") or "", - ) - if len(usable) == len(identifiers): - kept.append(member) + usable, unusable = split + if not _redfish_severity_is_ok(member): + for entry in unusable: + afid = entry.get("AFID") if isinstance(entry, dict) else None + log.warning( + "Waiving AFID %s from %s: AMDFieldIdentifiers entry has no ServiceableUnits", + afid, + member.get("@odata.id") or member.get("Id") or "", + ) + if not unusable: + kept.append(promote_serviceable_unit_odata_ids(member)) elif usable: + amd = cast(dict, oem).get("AMD") amd_copy = {**cast(dict, amd), "AMDFieldIdentifiers": usable} - kept.append({**member, "Oem": {**cast(dict, oem), "AMD": amd_copy}}) + kept.append( + promote_serviceable_unit_odata_ids( + {**member, "Oem": {**cast(dict, oem), "AMD": amd_copy}} + ) + ) return kept @@ -579,6 +593,134 @@ def resolve_runtime_afid_sag_path( return path +def _untagged_afse_has_redfish_and_switch(version: str) -> bool: + """Return whether an untagged AFSE build can parse Redfish and switch events. + + Args: + version: Version reported by the AFSE package or module hub. + + Returns: + True when version has no release tag and both parsers import. + """ + if _UNTAGGED_AFSE_RE.match(version.strip()) is None: + return False + try: + redfish = importlib.import_module("serviceability_engine.transformers.redfish") + switch = importlib.import_module("serviceability_engine.transformers.switch") + except ImportError: + return False + return callable(getattr(redfish, "parse", None)) and callable(getattr(switch, "parse", None)) + + +def _afse_version_error( + version: str, + logger: Optional[logging.Logger] = None, +) -> Optional[str]: + """Return an error when AFSE is older than 0.2. + + Args: + version: Version reported by the AFSE package or module hub. + logger: Logger used when an untagged build is accepted. + + Returns: + Error text when the major.minor is below 0.2 or missing, otherwise None. + """ + if _untagged_afse_has_redfish_and_switch(version): + (logger or logging.getLogger(DEFAULT_LOGGER)).info( + "AFSE %s is an untagged build with Redfish and switch event support", + version, + ) + return None + match = _AFSE_VERSION_RE.match(version.strip()) + found = (int(match.group("major")), int(match.group("minor"))) if match else None + if found is None or found < _MIN_AFSE_VERSION: + return ( + f"AFSE {_MIN_AFSE_VERSION[0]}.{_MIN_AFSE_VERSION[1]} or newer is required " + f"(Redfish log entries and switch events); found {version}" + ) + return None + + +def _entry_point_distribution_version(hub_name: str) -> Optional[str]: + """Return the installed package version for a registered AFSE entry point. + + Args: + hub_name: Entry point name such as afse. + + Returns: + Distribution version string, or None when it cannot be resolved. + """ + wanted = normalize_hub_entry_point(hub_name) + for ep in _entry_points_for_group(HUB_ENTRY_POINT_GROUP): + if getattr(ep, "name", None) != wanted: + continue + dist = getattr(ep, "dist", None) + version = getattr(dist, "version", None) if dist is not None else None + if version: + return str(version) + value = str(getattr(ep, "value", "") or getattr(ep, "module", "") or "") + top = value.split(":")[0].split(".")[0].strip() + if not top: + continue + try: + return importlib.metadata.version(top) + except importlib.metadata.PackageNotFoundError: + continue + return None + + +def _probe_entry_point_hub( + args: ServiceabilityAnalyzerArgs, +) -> tuple[Any, str, str]: + """Load the configured AFSE entry point and return the hub, label, and version. + + Args: + args: Analyzer args for this run. + + Returns: + Loaded hub instance, display label, and version string. + """ + name = args.resolved_hub_entry_point() + hub = load_hub_from_entry_point(name) + label = args.hub_display_name or getattr(hub, "name", None) or name + version = _entry_point_distribution_version(name) or _hub_engine_version(hub) + return hub, str(label), version + + +def preflight_module_hub( + args: ServiceabilityAnalyzerArgs, + logger: Optional[logging.Logger] = None, +) -> None: + """Check that a Python-module hub is installed and new enough. + + Args: + args: Analyzer args that select hub_python_module. + logger: Logger for the install line. Defaults to the nodescraper logger. + + Returns: + None. + + Raises: + HubRunError: When the module cannot be imported or AFSE is older than 0.2. + """ + log = logger or logging.getLogger(DEFAULT_LOGGER) + module_name = str(args.hub_python_module) + try: + mod = importlib.import_module(module_name) + except ImportError as exc: + raise HubRunError(f"AFSE hub module {module_name!r} is not installed: {exc}") from exc + hub_cls = _resolve_hub_class(mod, args.hub_analyze_method) + label = args.hub_display_name or getattr(hub_cls, "name", None) or module_name + version = getattr(mod, "__version__", None) or _hub_engine_version(hub_cls) + if callable(version): + version = version() + version_text = version.strip() if isinstance(version, str) and version.strip() else "unknown" + log.info("AFSE install found: %s version %s", label, version_text) + version_error = _afse_version_error(version_text, log) + if version_error: + raise HubRunError(version_error) + + def preflight_afid_sag( args: ServiceabilityAnalyzerArgs, logger: Optional[logging.Logger] = None, @@ -593,17 +735,34 @@ def preflight_afid_sag( Resolved absolute SAG path. Raises: - HubRunError: When the file is missing, the hub cannot load, or AFSE rejects the file. + HubRunError: When the file is missing, the hub cannot load, AFSE is too old, or AFSE rejects the file. """ log = logger or logging.getLogger(DEFAULT_LOGGER) - sag_path = resolve_runtime_afid_sag_path(args, log) - validate_afid_sag_path(sag_path) - resolved = str(Path(sag_path).expanduser().resolve()) - log.info("AFID_SAG file OK: %s", resolved) - hub_name = args.resolved_hub_entry_point() - hub = load_hub_from_entry_point(hub_name) - if validate_afid_sag_with_hub(hub, resolved): - log.info("AFID_SAG validated by %s: %s", getattr(hub, "name", hub_name), resolved) + errors: list[str] = [] + hub: Any = None + hub_label = "" + try: + hub, hub_label, hub_version = _probe_entry_point_hub(args) + log.info("AFSE install found: %s version %s", hub_label, hub_version) + version_error = _afse_version_error(hub_version, log) + if version_error: + errors.append(version_error) + except (HubRunError, ValueError) as exc: + errors.append(str(exc)) + + resolved = "" + try: + sag_path = resolve_runtime_afid_sag_path(args, log) + validate_afid_sag_path(sag_path) + resolved = str(Path(sag_path).expanduser().resolve()) + log.info("AFID_SAG file OK: %s", resolved) + if hub is not None and validate_afid_sag_with_hub(hub, resolved): + log.info("AFID_SAG validated by %s: %s", getattr(hub, "name", hub_label), resolved) + except HubRunError as exc: + errors.append(str(exc)) + + if errors: + raise HubRunError("; ".join(errors)) return resolved diff --git a/nodescraper/plugins/serviceability/serviceability_api.py b/nodescraper/plugins/serviceability/serviceability_api.py index 30f1ea2d..616766f3 100644 --- a/nodescraper/plugins/serviceability/serviceability_api.py +++ b/nodescraper/plugins/serviceability/serviceability_api.py @@ -149,6 +149,35 @@ def split_recommendation_actions( return top, additional +def top_action_summary( + block: ServiceabilityBlock, + system_name: str, + analyzed_at: str, +) -> dict[str, Any]: + """Build a top-service-action summary payload from a hub result. + + Args: + block: Serviceability block returned by the hub. + system_name: Name of the analyzed system. + analyzed_at: ISO timestamp for when analysis ran. + + Returns: + System name, analysis time, hub and SAG versions, AFID event count, and the + top service action(s). Ties for the top rank are all included. + """ + top, _ = split_recommendation_actions(block) + return { + "system_name": system_name, + "analyzed_at": analyzed_at, + "hub_version": block.hub_version, + "afid_sag_file_version": block.afid_sag_file_version, + "afid_event_count": len(block.afid_events), + "top_service_actions": [ + action.model_dump(mode="json", exclude_none=True) for action in top + ], + } + + def prepare_serviceability_block_for_export( block: ServiceabilityBlock, ) -> ServiceabilityBlock: diff --git a/nodescraper/plugins/serviceability/serviceability_plugin_base.py b/nodescraper/plugins/serviceability/serviceability_plugin_base.py index 67ff45ca..edb76ac8 100644 --- a/nodescraper/plugins/serviceability/serviceability_plugin_base.py +++ b/nodescraper/plugins/serviceability/serviceability_plugin_base.py @@ -23,10 +23,14 @@ # SOFTWARE. # ############################################################################### +from typing import Any, Optional, Union + from nodescraper.base import OOBandDataPlugin -from nodescraper.models import CollectorArgs +from nodescraper.enums import ExecutionStatus +from nodescraper.models import CollectorArgs, DataPluginResult, PluginResult, TaskResult from .analyzer_args import ServiceabilityAnalyzerArgs +from .se_runner import HubRunError, preflight_afid_sag, preflight_module_hub from .serviceability_collector import ServiceabilityCollectorBase from .serviceability_data import ServiceabilityDataModel @@ -44,3 +48,92 @@ class ServiceabilityPluginBase( COLLECTOR = ServiceabilityCollectorBase COLLECTOR_ARGS = CollectorArgs ANALYZER_ARGS = ServiceabilityAnalyzerArgs + + def _apply_afid_sag_path_to_collection_args( + self, + afid_sag_path: str, + collection_args: Any, + ) -> Any: + """Copy a resolved AFID_SAG path onto collector args when the collector stores one. + + Args: + afid_sag_path: Absolute path returned by preflight. + collection_args: Collector args for this run, or None. + + Returns: + Collector args unchanged. Subclasses that store the path override this. + """ + return collection_args + + def _plugin_error_result(self, message: str) -> PluginResult: + """Build a plugin result that skips collection and fails analysis. + + Args: + message: Error recorded on the plugin and analysis results. + + Returns: + Plugin result with collection NOT_RAN and analysis ERROR. + """ + self.logger.error("(%s) %s", self.__class__.__name__, message) + return PluginResult( + status=ExecutionStatus.ERROR, + source=self.__class__.__name__, + message=message, + result_data=DataPluginResult( + collection_result=TaskResult( + status=ExecutionStatus.NOT_RAN, + parent=self.__class__.__name__, + message="Data collection skipped", + ), + analysis_result=TaskResult( + status=ExecutionStatus.ERROR, + parent=self.__class__.__name__, + message=message, + ), + ), + ) + + def _preflight_hub_sag( + self, + collection_args: Any, + analysis_args: Optional[Union[ServiceabilityAnalyzerArgs, dict[str, Any]]], + ) -> Union[ + tuple[Any, Optional[Union[ServiceabilityAnalyzerArgs, dict[str, Any]]]], + PluginResult, + ]: + """Resolve and validate AFID_SAG before collection when an entry-point hub will run. + + Args: + collection_args: Collector args for this run, or None. + analysis_args: Analyzer args for this run, or None. + + Returns: + Updated collector and analyzer args, or a plugin error when preflight fails. + """ + try: + parsed = ( + analysis_args + if isinstance(analysis_args, self.ANALYZER_ARGS) + else self.ANALYZER_ARGS(**(analysis_args or {})) + ) + if parsed.skip_hub: + return collection_args, analysis_args + if not parsed.uses_entry_point_hub(): + preflight_module_hub(parsed, self.logger) + return collection_args, analysis_args + sag_path = preflight_afid_sag(parsed, self.logger) + except (HubRunError, ValueError) as exc: + return self._plugin_error_result(f"AFID_SAG preflight failed: {exc}") + if isinstance(analysis_args, dict): + updated_analysis: Optional[Union[ServiceabilityAnalyzerArgs, dict[str, Any]]] = { + **analysis_args, + "afid_sag_path": sag_path, + } + elif analysis_args is None: + updated_analysis = {"afid_sag_path": sag_path} + else: + updated_analysis = parsed.model_copy(update={"afid_sag_path": sag_path}) + return ( + self._apply_afid_sag_path_to_collection_args(sag_path, collection_args), + updated_analysis, + ) diff --git a/test/unit/plugin/test_afid_events_bmc_schema.py b/test/unit/plugin/test_afid_events_bmc_schema.py index 8529577c..cdcec83b 100644 --- a/test/unit/plugin/test_afid_events_bmc_schema.py +++ b/test/unit/plugin/test_afid_events_bmc_schema.py @@ -24,6 +24,7 @@ # ############################################################################### """AFID / serviceable unit extraction for OpenBMC-style LogEntry payloads.""" + from __future__ import annotations from serviceability_dummy_data import ( @@ -41,8 +42,12 @@ from nodescraper.plugins.serviceability.afid_events import ( _afid_event_from_rf_member, + amd_field_identifiers_with_units, build_afid_events_from_data, ) +from nodescraper.plugins.serviceability.se_runner import ( + drop_afids_without_serviceable_units, +) from nodescraper.plugins.serviceability.serviceability_data import ( ServiceabilityDataModel, ) @@ -80,3 +85,102 @@ def test_build_afid_events_from_data_includes_openbmc_entries(): by_afid_oam = {(e.afid, e.serviceable_unit) for e in events} assert (DUMMY_AFID_BELOW_RF, DUMMY_UNIT_A) in by_afid_oam assert (DUMMY_AFID_FATAL_HBM, DUMMY_UNIT_C) in by_afid_oam + + +def test_amd_field_identifiers_split_and_drop_members_without_a_location(): + located = { + "Id": "located", + "Oem": { + "AMD": { + "AMDFieldIdentifiers": [ + { + "AFID": DUMMY_AFID_A, + "ServiceableUnits": [{"@odata.id": f"/redfish/v1/Chassis/{DUMMY_UNIT_B}"}], + } + ] + } + }, + } + missing = { + "@odata.id": "/redfish/v1/Systems/dummy/LogServices/EventLog/Entries/9", + "Id": "missing", + "Oem": {"AMD": {"AMDFieldIdentifiers": [{"AFID": 11045, "Description": "update"}]}}, + } + mixed = { + "Id": "mixed", + "Oem": { + "AMD": { + "AMDFieldIdentifiers": [ + {"AFID": 11045}, + { + "AFID": DUMMY_AFID_A, + "ServiceableUnits": [{"@odata.id": f"/redfish/v1/Chassis/{DUMMY_UNIT_A}"}], + }, + ] + } + }, + } + plain = {"Id": "plain", "Message": "no oem"} + usable, unusable = amd_field_identifiers_with_units(missing["Oem"]) + assert usable == [] + assert unusable == [{"AFID": 11045, "Description": "update"}] + assert amd_field_identifiers_with_units(plain) is None + kept = drop_afids_without_serviceable_units([located, missing, mixed, plain]) + kept_ids = [member["Id"] for member in kept] + assert kept_ids == ["located", "mixed", "plain"] + mixed_kept = next(member for member in kept if member["Id"] == "mixed") + assert mixed_kept["Oem"]["AMD"]["AMDFieldIdentifiers"] == [ + { + "AFID": DUMMY_AFID_A, + "ServiceableUnits": [{"@odata.id": f"/redfish/v1/Chassis/{DUMMY_UNIT_A}"}], + } + ] + + +def test_ok_severity_without_serviceable_units_is_silent(caplog): + member = { + "Id": "ok-update", + "@odata.id": "/redfish/v1/Systems/dummy/LogServices/EventLog/Entries/1", + "Severity": "OK", + "Oem": {"AMD": {"AMDFieldIdentifiers": [{"AFID": 11045}]}}, + } + with caplog.at_level("WARNING"): + kept = drop_afids_without_serviceable_units([member]) + assert kept == [] + assert "11045" not in caplog.text + + +def test_non_ok_severity_without_serviceable_units_warns(caplog): + member = { + "Id": "fault", + "@odata.id": "/redfish/v1/Systems/dummy/LogServices/EventLog/Entries/2", + "Severity": "Critical", + "Oem": {"AMD": {"AMDFieldIdentifiers": [{"AFID": 11045}]}}, + } + with caplog.at_level("WARNING"): + kept = drop_afids_without_serviceable_units([member]) + assert kept == [] + assert "Waiving AFID 11045" in caplog.text + + +def test_serviceable_unit_from_oem_amd_field_identifiers(): + ev = _afid_event_from_rf_member( + { + "Created": DUMMY_TIMESTAMP, + "Oem": { + "AMD": { + "AMDFieldIdentifiers": [ + { + "AFID": DUMMY_AFID_A, + "ServiceableUnits": [ + {"@odata.id": f"/redfish/v1/Chassis/{DUMMY_UNIT_B}"} + ], + } + ] + } + }, + } + ) + assert ev is not None + assert ev.afid == DUMMY_AFID_A + assert ev.serviceable_unit == DUMMY_UNIT_B diff --git a/test/unit/plugin/test_mi4xx_plugin.py b/test/unit/plugin/test_mi4xx_plugin.py index 76296ff8..5d5edd81 100644 --- a/test/unit/plugin/test_mi4xx_plugin.py +++ b/test/unit/plugin/test_mi4xx_plugin.py @@ -88,6 +88,7 @@ def test_mi4xx_analyzer_analyze_data_wrapped_once(): class _FakeHub: name = "ExampleHub" + version = "0.2.0" def analyze(self, events, afid_sag_path): assert afid_sag_path From f58d2edc3dfac781d0c729ea98bca2810186ccab Mon Sep 17 00:00:00 2001 From: Alexandra Bara Date: Mon, 2 Nov 2026 14:52:56 -0600 Subject: [PATCH 4/4] acommodating for fru -> action_subject rename --- .../plugins/serviceability/afid_sag_lookup.py | 93 +++++-- .../serviceability/serviceability_api.py | 230 ++++++++++++++++-- .../serviceability/serviceability_data.py | 15 +- .../serviceability_recommendations_table.py | 46 +++- nodescraper/resultcollators/tablesummary.py | 3 +- test/unit/plugin/test_se_runner.py | 10 +- test/unit/plugin/test_serviceability_api.py | 46 ++-- ...st_serviceability_recommendations_table.py | 116 +++++++++ 8 files changed, 484 insertions(+), 75 deletions(-) diff --git a/nodescraper/plugins/serviceability/afid_sag_lookup.py b/nodescraper/plugins/serviceability/afid_sag_lookup.py index 52957ce5..a13bee20 100644 --- a/nodescraper/plugins/serviceability/afid_sag_lookup.py +++ b/nodescraper/plugins/serviceability/afid_sag_lookup.py @@ -48,19 +48,46 @@ def load_afid_sag_data(path: Optional[str]) -> Optional[dict[str, Any]]: def normalize_fru_name(name: str) -> str: - """Normalize FRU labels for comparison (SAG serviceable_fru vs afid.fru).""" + """Normalize a FRU or action subject for comparison. + + Args: + name: SAG fru or action_subject label. + + Returns: + Uppercase label with hyphens replaced by underscores. + """ return str(name).strip().upper().replace("-", "_") -def known_fru_names_from_sag(sag: Optional[dict[str, Any]]) -> list[str]: - """Return normalized FRU names declared under serviceable_fru in the SAG.""" +def _serviceable_catalog(sag: Optional[dict[str, Any]]) -> list[Any]: + """Return the SAG list of serviceable groups. + + Args: + sag: Loaded AFID SAG object. + + Returns: + serviceable_fru when that list is present, otherwise serviceable_action_subject. + """ if not sag: return [] - raw = sag.get("serviceable_fru") - if not isinstance(raw, list): - return [] + for key in ("serviceable_fru", "serviceable_action_subject"): + raw = sag.get(key) + if isinstance(raw, list) and raw: + return raw + return [] + + +def known_fru_names_from_sag(sag: Optional[dict[str, Any]]) -> list[str]: + """Return normalized serviceable group names from the SAG catalog. + + Args: + sag: Loaded AFID SAG object. + + Returns: + Normalized fru or action_subject names declared in the SAG catalog. + """ names: list[str] = [] - for item in raw: + for item in _serviceable_catalog(sag): if not isinstance(item, dict): continue for key in item: @@ -71,14 +98,16 @@ def known_fru_names_from_sag(sag: Optional[dict[str, Any]]) -> list[str]: def sag_fru_display_names(sag: Optional[dict[str, Any]]) -> list[str]: - """Return sorted FRU labels from SAG serviceable_fru using the original key text.""" - if not sag: - return [] - raw = sag.get("serviceable_fru") - if not isinstance(raw, list): - return [] + """Return serviceable group labels from the SAG catalog. + + Args: + sag: Loaded AFID SAG object. + + Returns: + Original fru or action_subject key text from the SAG catalog. + """ names: list[str] = [] - for item in raw: + for item in _serviceable_catalog(sag): if not isinstance(item, dict): continue for key in item: @@ -100,14 +129,23 @@ def afid_entry_from_sag(afid: int, sag: Optional[dict[str, Any]]) -> Optional[di def afid_fru_from_sag(afid: int, sag: Optional[dict[str, Any]]) -> Optional[str]: - """Return the FRU string for an AFID from the SAG afid table.""" + """Return the FRU or action subject for an AFID from the SAG afid table. + + Args: + afid: AMD Fault ID. + sag: Loaded AFID SAG object. + + Returns: + afid.fru when set, otherwise afid.action_subject. None when neither is set. + """ entry = afid_entry_from_sag(afid, sag) if not entry: return None - fru = entry.get("fru") - if fru is None or not str(fru).strip(): - return None - return str(fru).strip() + for key in ("fru", "action_subject"): + value = entry.get(key) + if value is not None and str(value).strip(): + return str(value).strip() + return None def afid_summary_from_sag(afid: int, sag: Optional[dict[str, Any]]) -> Optional[str]: @@ -182,7 +220,15 @@ def group_afid_events_by_fru( events: list[AfidEvent], sag: Optional[dict[str, Any]], ) -> dict[str, list[AfidEvent]]: - """Group parsed AFID events by SAG FRU (unknown FRU bucket when lookup fails).""" + """Group parsed AFID events by SAG fru or action subject. + + Args: + events: Parsed AFID events. + sag: Loaded AFID SAG object. + + Returns: + Events keyed by normalized fru or action subject. Missing labels use UNKNOWN_FRU. + """ grouped: dict[str, list[AfidEvent]] = defaultdict(list) for event in events: fru = afid_fru_from_sag(event.afid, sag) or "UNKNOWN_FRU" @@ -215,13 +261,6 @@ def format_collected_afid_fru_summary_lines( unit_bits = ", ".join(f"{unit} x{count}" for unit, count in sorted(unit_counts.items())) lines.append(f" {fru}: AFIDs [{afid_bits}]") lines.append(f" units: {unit_bits}") - known = known_fru_names_from_sag(sag) - if known: - missing = sorted(set(known) - set(grouped.keys())) - if missing: - lines.append(f" FRUs in SAG with no collected AFID events: {', '.join(missing)}") - else: - lines.append(" All SAG serviceable FRUs have at least one collected AFID event.") return lines diff --git a/nodescraper/plugins/serviceability/serviceability_api.py b/nodescraper/plugins/serviceability/serviceability_api.py index 616766f3..5cfc8ea1 100644 --- a/nodescraper/plugins/serviceability/serviceability_api.py +++ b/nodescraper/plugins/serviceability/serviceability_api.py @@ -25,7 +25,7 @@ ############################################################################### from __future__ import annotations -from typing import Any +from typing import Any, Optional from .se_models import ( HubTriageResult, @@ -89,8 +89,210 @@ def _solution_to_action(rank: int, solution: ServiceabilitySolution) -> Prioriti ) +def _optional_int(value: Any) -> Optional[int]: + """Return value as an int when it is numeric. + + Args: + value: Field from an AFSE triage row. + + Returns: + The integer value, or None when value is missing or not numeric. + """ + if value is None: + return None + try: + return int(value) + except (TypeError, ValueError): + return None + + +def _afse_service_action(row: dict[str, Any]) -> dict[str, Any]: + """Return the service_action object on an AFSE triage row. + + Args: + row: One AFSE triage.top or triage.results entry. + + Returns: + The service_action mapping, or an empty mapping when it is absent. + """ + service_action = row.get("service_action") + if isinstance(service_action, dict): + return service_action + return {} + + +def _afse_action_title(row: dict[str, Any]) -> Optional[str]: + """Return the service action title from an AFSE triage row. + + Args: + row: One AFSE triage.top or triage.results entry. + + Returns: + The title text, or None when the row has no title. + """ + service_action = _afse_service_action(row) + for candidate in (service_action.get("title"), row.get("service_action_title")): + if candidate is None: + continue + text = str(candidate).strip() + if text: + return text + return None + + +def _afse_action_steps(row: dict[str, Any]) -> list[str]: + """Return service action step descriptions from an AFSE triage row. + + Args: + row: One AFSE triage.top or triage.results entry. + + Returns: + Step description strings in AFSE order. + """ + steps = _afse_service_action(row).get("steps") + if not isinstance(steps, list): + return [] + descriptions: list[str] = [] + for step in steps: + if not isinstance(step, dict): + continue + description = step.get("description") + if description is None: + continue + text = str(description).strip() + if text: + descriptions.append(text) + return descriptions + + +def _action_from_afse_row(rank: int, row: dict[str, Any]) -> Optional[PrioritizedServiceAction]: + """Map one AFSE triage row to a table action. + + Args: + rank: 1-based position of this row inside triage.top. + row: One AFSE triage.top entry. + + Returns: + The table action, or None when the row has no AFID, action number, or location. + """ + afid_raw = row.get("afid", row.get("afid_num")) + san_raw = row.get("service_action_num") + location = row.get("location") or row.get("serviceable_unit") + if afid_raw is None or san_raw is None or location is None: + return None + try: + afid = int(afid_raw) + service_action_num = int(san_raw) + except (TypeError, ValueError): + return None + location_text = str(location).strip() + if not location_text: + return None + service_action = _afse_service_action(row) + tier_label = row.get("tier_label") + if tier_label is not None: + tier_label = str(tier_label).strip() or None + category = service_action.get("category") + category_text = ( + str(category).strip() if category is not None and str(category).strip() else None + ) + return PrioritizedServiceAction( + rank=rank, + afid=afid, + location=location_text, + count=max(1, _optional_int(row.get("count")) or 1), + service_action_num=service_action_num, + service_action_title=_afse_action_title(row), + service_action_category=category_text, + priority=_optional_int(row.get("priority")), + sa_severity=_optional_int(row.get("sa_severity")) + or _optional_int(service_action.get("severity")), + tier=_optional_int(row.get("tier")), + tier_label=tier_label, + hub_sort_priority=_optional_int( + row.get("se_sort_priority") or row.get("hub_sort_priority") + ), + multi_mask=_optional_int(row.get("multi_mask")), + service_action_steps=_afse_action_steps(row), + ) + + +def actions_from_afse_triage_top(block: ServiceabilityBlock) -> list[PrioritizedServiceAction]: + """Return table rows for AFSE triage.top. + + Args: + block: Serviceability block that may hold the raw hub analyze response. + + Returns: + One action per AFSE triage.top entry. Empty when that list is absent. + """ + response = block.hub_analyze_response + if not isinstance(response, dict): + return [] + triage = response.get("triage") + if not isinstance(triage, dict): + return [] + top = triage.get("top") + if not isinstance(top, list): + return [] + actions: list[PrioritizedServiceAction] = [] + for index, row in enumerate(top, start=1): + if not isinstance(row, dict): + continue + action = _action_from_afse_row(index, row) + if action is not None: + actions.append(action) + return actions + + +def _afse_triage_section(block: ServiceabilityBlock) -> Optional[dict[str, Any]]: + """Return the AFSE triage object when the hub response includes one. + + Args: + block: Serviceability block populated by hub analysis. + + Returns: + The triage mapping, or None when the response has no triage section. + """ + response = block.hub_analyze_response + if not isinstance(response, dict): + return None + triage = response.get("triage") + if not isinstance(triage, dict): + return None + return triage + + +def afse_top_action_missing(block: ServiceabilityBlock) -> bool: + """Return whether an AFSE triage section has no top action. + + Args: + block: Serviceability block populated by hub analysis. + + Returns: + True when triage is present and triage.top has no usable action. + """ + if _afse_triage_section(block) is None: + return False + return not actions_from_afse_triage_top(block) + + def build_top_service_actions(block: ServiceabilityBlock) -> list[PrioritizedServiceAction]: - """Return Hub triage.top service actions with ranks from triage.results order.""" + """Return the AFSE triage.top service actions for display. + + Args: + block: Serviceability block populated by hub analysis. + + Returns: + Actions from AFSE triage.top when that list has entries. Empty when triage + is present but triage.top is empty. Parsed hub rows are used when the + response has no triage section. + """ + afse_top = actions_from_afse_triage_top(block) + if afse_top: + return afse_top + if _afse_triage_section(block) is not None: + return [] if block.hub_top_results: rank_by_identity = { _hub_triage_identity(row): rank @@ -131,6 +333,8 @@ def split_recommendation_actions( if not prioritized: return top, [] if not top: + if _afse_triage_section(block) is not None: + return [], prioritized return [prioritized[0]], prioritized[1:] top_sort_priorities = { @@ -178,17 +382,15 @@ def top_action_summary( } -def prepare_serviceability_block_for_export( - block: ServiceabilityBlock, -) -> ServiceabilityBlock: - """Return a JSON-safe serviceability block without duplicate hub triage rows.""" - return block.model_copy(update={"hub_triage_results": [], "hub_top_results": []}) +def export_serviceability_json(block: ServiceabilityBlock) -> Optional[dict[str, Any]]: + """Return the AFSE analyze payload written to serviceability.json. + Args: + block: Serviceability block that may hold the raw hub analyze response. -def export_serviceability_json(block: ServiceabilityBlock) -> dict[str, Any]: - """Serialize serviceability.json with raw Hub output and no duplicate triage rows.""" - exported = prepare_serviceability_block_for_export(block) - return exported.model_dump( - mode="json", - exclude={"hub_triage_results", "hub_top_results"}, - ) + Returns: + The hub analyze response, or None when AFSE did not return one. + """ + if block.hub_analyze_response is None: + return None + return dict(block.hub_analyze_response) diff --git a/nodescraper/plugins/serviceability/serviceability_data.py b/nodescraper/plugins/serviceability/serviceability_data.py index b77ad3f5..eeb2f6b2 100644 --- a/nodescraper/plugins/serviceability/serviceability_data.py +++ b/nodescraper/plugins/serviceability/serviceability_data.py @@ -37,6 +37,7 @@ from .event_log_utils import rf_events_from_json_payload from .se_models import AfidEvent, ServiceabilityBlock +from .serviceability_api import export_serviceability_json class ServiceabilityDataLoadError(ValueError): @@ -151,15 +152,11 @@ def log_model(self, log_path: str) -> None: with open(cper_path, "w", encoding="utf-8") as f: json.dump(self.cper_data, f, indent=2) if self.serviceability is not None: - serviceability_path = os.path.join(log_path, "serviceability.json") - from .serviceability_api import export_serviceability_json - - with open(serviceability_path, "w", encoding="utf-8") as f: - json.dump( - export_serviceability_json(self.serviceability), - f, - indent=2, - ) + payload = export_serviceability_json(self.serviceability) + if payload is not None: + serviceability_path = os.path.join(log_path, "serviceability.json") + with open(serviceability_path, "w", encoding="utf-8") as f: + json.dump(payload, f, indent=2) from .afid_fru_csv import write_afid_fru_summary_csv write_afid_fru_summary_csv( diff --git a/nodescraper/plugins/serviceability/serviceability_recommendations_table.py b/nodescraper/plugins/serviceability/serviceability_recommendations_table.py index 9816e95b..0383cf0a 100644 --- a/nodescraper/plugins/serviceability/serviceability_recommendations_table.py +++ b/nodescraper/plugins/serviceability/serviceability_recommendations_table.py @@ -25,14 +25,16 @@ ############################################################################### from __future__ import annotations +import logging import sys from textwrap import wrap from typing import Any, Optional +from nodescraper.constants import DEFAULT_LOGGER from nodescraper.models import PluginResult from .se_models import PrioritizedServiceAction, ServiceabilityBlock -from .serviceability_api import split_recommendation_actions +from .serviceability_api import afse_top_action_missing, split_recommendation_actions RECOMMENDATION_TABLE_HEADERS: tuple[str, ...] = ( "Rank", @@ -172,20 +174,37 @@ def _serviceability_block_from_plugin_result( if isinstance(serviceability, dict): serviceability = ServiceabilityBlock.model_validate(serviceability) - if not (serviceability.hub_triage_results or serviceability.hub_top_results): + if not ( + serviceability.hub_triage_results + or serviceability.hub_top_results + or isinstance(serviceability.hub_analyze_response, dict) + ): return None return serviceability def render_serviceability_recommendation_tables_for_plugin_results( plugin_results: list[PluginResult], + logger: Optional[logging.Logger] = None, ) -> str: - """Render Hub recommendation tables for all plugins that produced serviceability output.""" + """Render Hub recommendation tables for plugins that produced serviceability output. + + Args: + plugin_results: Plugin results from the run. + logger: Logger for the missing-top warning. Defaults to the nodescraper logger. + + Returns: + Joined top-action tables. Empty when AFSE returned no triage.top action. + """ + log = logger or logging.getLogger(DEFAULT_LOGGER) sections: list[str] = [] for plugin_result in plugin_results: block = _serviceability_block_from_plugin_result(plugin_result) if block is None: continue + if afse_top_action_missing(block): + log.warning("AFSE did not return a top action item") + continue rendered = render_serviceability_recommendation_tables(block) if rendered: sections.append(rendered) @@ -193,10 +212,13 @@ def render_serviceability_recommendation_tables_for_plugin_results( def render_serviceability_recommendation_tables(block: ServiceabilityBlock) -> str: - """Render the top Hub recommendation table. + """Render the AFSE triage.top service action table. + + Args: + block: Serviceability block populated by hub analysis. - Lower-ranked actions are not printed; they remain in the serviceability block - saved with the plugin result. + Returns: + The top service action table. Lower-ranked triage.results rows are omitted. """ primary, _ = split_recommendation_actions(block) return render_recommendations_section( @@ -206,7 +228,17 @@ def render_serviceability_recommendation_tables(block: ServiceabilityBlock) -> s def emit_serviceability_recommendation_tables(block: ServiceabilityBlock) -> None: - """Print Hub recommendation tables to stdout.""" + """Print the AFSE top service action table to stdout. + + Args: + block: Serviceability block populated by hub analysis. + + Returns: + None. Logs a warning and prints nothing when AFSE returned no top action. + """ + if afse_top_action_missing(block): + logging.getLogger(DEFAULT_LOGGER).warning("AFSE did not return a top action item") + return output = render_serviceability_recommendation_tables(block) if not output: return diff --git a/nodescraper/resultcollators/tablesummary.py b/nodescraper/resultcollators/tablesummary.py index cde7dc25..e8b5b2f3 100644 --- a/nodescraper/resultcollators/tablesummary.py +++ b/nodescraper/resultcollators/tablesummary.py @@ -143,7 +143,8 @@ def render_physical_row(parts: list[str]) -> str: table_sections.append(table) serviceability_tables = render_serviceability_recommendation_tables_for_plugin_results( - plugin_results + plugin_results, + logger=self.logger, ) if serviceability_tables: table_sections.append(serviceability_tables) diff --git a/test/unit/plugin/test_se_runner.py b/test/unit/plugin/test_se_runner.py index 4fd86a08..bb750bb5 100644 --- a/test/unit/plugin/test_se_runner.py +++ b/test/unit/plugin/test_se_runner.py @@ -657,16 +657,24 @@ def test_mi3xx_analyzer_runs_python_hub(system_info): def test_mi3xx_analyzer_writes_serviceability_json(tmp_path, system_info): + hub_response = { + "schema_version": "1.0", + "status": "ok", + "triage": {"top": [], "results": [], "multi_afid_summary": []}, + } data = ServiceabilityDataModel( afid_events=EXAMPLE_EVENTS[:1], serviceability=ServiceabilityBlock( afid_events=EXAMPLE_EVENTS[:1], solution=[], + solution_reasoning="node-scraper summary", + hub_analyze_response=hub_response, ), ) data.log_model(str(tmp_path)) payload = json.loads((tmp_path / "serviceability.json").read_text(encoding="utf-8")) - assert payload["afid_events"][0]["afid"] == DUMMY_AFID_A + assert payload == hub_response + assert "afid_events" not in payload class _ValidatingHub: diff --git a/test/unit/plugin/test_serviceability_api.py b/test/unit/plugin/test_serviceability_api.py index e11ebc5d..cf8e1d15 100644 --- a/test/unit/plugin/test_serviceability_api.py +++ b/test/unit/plugin/test_serviceability_api.py @@ -35,26 +35,27 @@ ) -def test_export_serviceability_json_keeps_raw_hub_and_drops_duplicate_rows(): +def test_export_serviceability_json_is_only_the_afse_response(): + hub_response = { + "schema_version": "1.0", + "status": "ok", + "triage": { + "top": [{"afid": DUMMY_AFID_A, "location": DUMMY_UNIT_A, "se_sort_priority": 1000}], + "results": [{"afid": DUMMY_AFID_A, "location": DUMMY_UNIT_A, "se_sort_priority": 1000}], + "multi_afid_summary": [], + }, + "pid": "SAG-00000", + "revision": "1.0.0", + } block = ServiceabilityBlock( afid_events=[ AfidEvent( afid=DUMMY_AFID_A, serviceable_unit="Instinct_EAM_0", time="2025-01-01T00:00:00Z" ) ], - hub_analyze_response={ - "schema_version": "1.0", - "status": "ok", - "triage": { - "top": [{"afid": DUMMY_AFID_A, "location": DUMMY_UNIT_A, "se_sort_priority": 1000}], - "results": [ - {"afid": DUMMY_AFID_A, "location": DUMMY_UNIT_A, "se_sort_priority": 1000} - ], - "multi_afid_summary": [], - }, - "pid": "SAG-00000", - "revision": "1.0.0", - }, + solution_reasoning="node-scraper summary", + hub_version="0.2.0", + hub_analyze_response=hub_response, hub_top_results=[ HubTriageResult( afid=DUMMY_AFID_A, @@ -75,6 +76,19 @@ def test_export_serviceability_json_keeps_raw_hub_and_drops_duplicate_rows(): payload = export_serviceability_json(block) - assert payload["hub_analyze_response"]["triage"]["top"][0]["se_sort_priority"] == 1000 - assert "hub_top_results" not in payload + assert payload == hub_response + assert "afid_events" not in payload + assert "solution" not in payload assert "hub_triage_results" not in payload + + +def test_export_serviceability_json_omits_file_without_afse_response(): + block = ServiceabilityBlock( + afid_events=[ + AfidEvent( + afid=DUMMY_AFID_A, serviceable_unit="Instinct_EAM_0", time="2025-01-01T00:00:00Z" + ) + ] + ) + + assert export_serviceability_json(block) is None diff --git a/test/unit/plugin/test_serviceability_recommendations_table.py b/test/unit/plugin/test_serviceability_recommendations_table.py index 4616a704..17971e82 100644 --- a/test/unit/plugin/test_serviceability_recommendations_table.py +++ b/test/unit/plugin/test_serviceability_recommendations_table.py @@ -23,6 +23,8 @@ # SOFTWARE. # ############################################################################### +import logging + from serviceability_dummy_data import ( DUMMY_AFID_A, DUMMY_AFID_B, @@ -147,6 +149,120 @@ def test_render_serviceability_recommendation_tables_prints_only_top_action(): assert f"{DUMMY_AFID_B}: Update FW" not in output +def test_table_prints_afse_triage_top_service_action(): + block = ServiceabilityBlock( + hub_analyze_response={ + "status": "ok", + "triage": { + "top": [ + { + "afid": DUMMY_AFID_A, + "location": DUMMY_UNIT_A, + "service_action_num": DUMMY_SERVICE_ACTION_NUM, + "priority": 20, + "sa_severity": 2, + "tier_label": "Secondary", + "service_action": { + "title": "Contact AMD", + "severity": 2, + "steps": [ + {"description": "Contact your AMD service provider."}, + ], + }, + } + ], + "results": [ + { + "afid": DUMMY_AFID_B, + "location": DUMMY_UNIT_B, + "service_action_num": 1, + } + ], + }, + }, + hub_top_results=[ + _row( + afid=DUMMY_AFID_B, + location=DUMMY_UNIT_B, + sort_priority=9000, + priority=1, + title="Update FW", + ) + ], + hub_triage_results=[ + _row( + afid=DUMMY_AFID_B, + location=DUMMY_UNIT_B, + sort_priority=9000, + priority=1, + title="Update FW", + ) + ], + ) + + output = render_serviceability_recommendation_tables(block) + + assert "Top Hub service action (rank 1)" in output + assert f"{DUMMY_AFID_A}: Contact AMD" in output + assert "Contact your AMD service provider." in output + assert f"{DUMMY_AFID_B}: Update FW" not in output + + +def test_missing_afse_top_skips_table_and_logs_warning(caplog): + block = ServiceabilityBlock( + hub_analyze_response={ + "status": "ok", + "triage": { + "top": [], + "results": [ + { + "afid": DUMMY_AFID_B, + "location": DUMMY_UNIT_B, + "service_action_num": 1, + } + ], + }, + }, + hub_top_results=[ + _row( + afid=DUMMY_AFID_B, + location=DUMMY_UNIT_B, + sort_priority=9000, + priority=1, + title="Update FW", + ) + ], + hub_triage_results=[ + _row( + afid=DUMMY_AFID_B, + location=DUMMY_UNIT_B, + sort_priority=9000, + priority=1, + title="Update FW", + ) + ], + ) + plugin_results = [ + PluginResult( + status=ExecutionStatus.OK, + source="Mi4xxServiceabilityPlugin", + message="ok", + result_data=DataPluginResult( + system_data=ServiceabilityDataModel(serviceability=block), + ), + ) + ] + + with caplog.at_level(logging.WARNING, logger="nodescraper"): + output = render_serviceability_recommendation_tables_for_plugin_results(plugin_results) + + assert output == "" + assert "Rank" not in output + assert "Update FW" not in output + assert "AFSE did not return a top action item" in caplog.text + assert render_serviceability_recommendation_tables(block) == "" + + def test_build_top_service_actions_includes_tied_entries(): block = ServiceabilityBlock( hub_top_results=[