From e1097b6a0a8a438271fac7040da67eecdffcebb6 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 12 Nov 2026 14:13:35 +0100 Subject: [PATCH 1/2] baremetal list: fix "manageable" state choice Change the --provision-state choice "managable" to "manageable" in osism baremetal list. Ironic's provision state is spelled "manageable". The misspelled choice was passed to Ironic as a filter that no node can match, so "--provision-state managable" always listed nothing, while the correct spelling was rejected by argparse. Listing the manageable nodes was therefore impossible with this option. Add a test that "manageable" is accepted and forwarded to the node query. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Roger Luethi --- osism/commands/baremetal.py | 2 +- tests/unit/commands/test_baremetal.py | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/osism/commands/baremetal.py b/osism/commands/baremetal.py index 474df1e70..76436450d 100644 --- a/osism/commands/baremetal.py +++ b/osism/commands/baremetal.py @@ -138,7 +138,7 @@ def get_parser(self, prog_name): parser.add_argument( "--provision-state", default=None, - choices=["enroll", "managable", "available", "active", "error"], + choices=["enroll", "manageable", "available", "active", "error"], type=str, help="Only list nodes with the given provision state", ) diff --git a/tests/unit/commands/test_baremetal.py b/tests/unit/commands/test_baremetal.py index 4fec10ec4..6eedfa3c4 100644 --- a/tests/unit/commands/test_baremetal.py +++ b/tests/unit/commands/test_baremetal.py @@ -470,6 +470,15 @@ def test_list_query_flags_forwarded(): ) +def test_list_accepts_manageable_provision_state(): + conn = MagicMock() + conn.baremetal.nodes.return_value = [] + + _run_list(["--provision-state", "manageable"], conn) + + conn.baremetal.nodes.assert_called_once_with(provision_state="manageable") + + def test_list_without_flags_queries_without_kwargs(): conn = MagicMock() conn.baremetal.nodes.return_value = [] From d9e2ceea88b3a88b3aa523f364ef5956fe29569e Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 12 Nov 2026 14:17:24 +0100 Subject: [PATCH 2/2] baremetal list: add --details Add a --details option to osism baremetal list. It appends five columns to the table; without it the output is unchanged: - State Since (UTC): when the node entered its current provision state, from Ironic's provision_updated_at. After a series of redeploys it shows which nodes have been redeployed and which still run their old installation. - Target State: target_provision_state, i.e. where a node in a transitional state such as "wait call-back" is heading. - Automated Clean: the node's automated_clean. Ironic's null means the node follows the conductor's [conductor]automated_clean, so it is shown as "default" rather than as False. - Maintenance Reason: maintenance_reason, or Ironic's fault (e.g. "power failure") when Ironic put the node into maintenance itself. - Last Error: last_error on a single line, cut to 80 characters. Maintenance Reason and Last Error are free text; last_error in particular can carry messages that originate from a BMC or the deploy agent. Control characters in them (C0 and C1, which include ESC, BEL and CSI as well as line breaks) are replaced with spaces before printing, so an escape sequence prints as plain text instead of reaching the operator's terminal, and the table stays one line per node. The plain node list returns only a few fields, so --details lists the nodes with details=True. openstacksdk's Node resource does not map provision_updated_at: Ironic sends the field, but the SDK drops it while parsing the response. The new osism.utils.ironic module therefore lists through a Node subclass, DetailedNode, that declares the field. It goes through Node.list, the public method behind conn.baremetal.nodes(), which sends details=True to /nodes/detail, so the request and microversion negotiation are the same as the SDK's own. The subclass can be removed once openstacksdk maps the field. The subclass also repeats Node's "uuid" alternate-id declaration: openstacksdk looks the alternate id up only in the class's own __dict__, so a Node subclass without it reports None as every node's id. The module is imported only when --details is given, keeping openstacksdk out of the command module's import. Tests cover the columns and their formatting, the exact 80-character Last Error cell, the control-character handling, and, by running openstacksdk's real list code against a mocked baremetal proxy, that the request goes to /nodes/detail with the filter and the negotiated microversion and that uuid and provision_updated_at survive parsing. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Roger Luethi --- osism/commands/baremetal.py | 89 +++++++++++- osism/utils/ironic.py | 38 +++++ tests/unit/commands/test_baremetal.py | 194 ++++++++++++++++++++++++++ 3 files changed, 320 insertions(+), 1 deletion(-) create mode 100644 osism/utils/ironic.py diff --git a/osism/commands/baremetal.py b/osism/commands/baremetal.py index 76436450d..1ae8e4c8e 100644 --- a/osism/commands/baremetal.py +++ b/osism/commands/baremetal.py @@ -6,10 +6,12 @@ BooleanOptionalAction, RawDescriptionHelpFormatter, ) +from datetime import datetime, timezone from textwrap import dedent import tempfile import os +import re import subprocess import threading from loguru import logger @@ -126,6 +128,59 @@ def _apply_metalbox_vars(play_vars, device): play_vars["docker_insecure_registries"] = ["metalbox:5001"] +LIST_LAST_ERROR_WIDTH = 80 + +# NOTE: C0 and C1 control characters, DEL included. ESC (0x1b) and CSI +# (0x9b) start terminal escape sequences, BEL (0x07) ends OSC ones. +_CONTROL_CHARACTERS = re.compile(r"[\x00-\x1f\x7f-\x9f]") + + +def _single_line_text(value): + """Make a free-text Ironic field safe to print as one table cell. + + Fields such as last_error can carry text from BMCs or the deploy agent. + Control characters become spaces, so an escape sequence prints as + plain text instead of reaching the terminal, and line breaks do not + break the table. + """ + if not value: + return "" + text = _CONTROL_CHARACTERS.sub(" ", value) + return re.sub(r"\s+", " ", text).strip() + + +def _format_state_since(value): + """Render Ironic's provision_updated_at as "YYYY-MM-DD HH:MM" in UTC.""" + if not value: + return "" + try: + timestamp = datetime.fromisoformat(value) + except ValueError: + return value + if timestamp.tzinfo is None: + timestamp = timestamp.replace(tzinfo=timezone.utc) + return timestamp.astimezone(timezone.utc).strftime("%Y-%m-%d %H:%M") + + +def _format_automated_clean(value): + # NOTE: None is not False: the node follows the conductor's + # [conductor]automated_clean setting. + return "default" if value is None else value + + +def _format_maintenance_reason(node): + # NOTE: Ironic sets fault (e.g. "power failure") when it put the node + # into maintenance itself; an operator's reason takes precedence. + return _single_line_text(node["maintenance_reason"] or node["fault"]) + + +def _shorten_last_error(value, width=LIST_LAST_ERROR_WIDTH): + text = _single_line_text(value) + if len(text) <= width: + return text + return text[: width - 3] + "..." + + class BaremetalList(Command): def get_parser(self, prog_name): parser = super(BaremetalList, self).get_parser(prog_name) @@ -154,6 +209,16 @@ def get_parser(self, prog_name): action="store_true", help="Include device role from NetBox (slower)", ) + parser.add_argument( + "--details", + default=False, + action="store_true", + help=( + "Also show when each node entered its provision state (UTC), " + "the target state, automated clean, the maintenance reason " + "and the last error" + ), + ) return parser def take_action(self, parsed_args): @@ -161,6 +226,7 @@ def take_action(self, parsed_args): provision_state = parsed_args.provision_state maintenance = parsed_args.maintenance include_netbox = parsed_args.netbox + details = parsed_args.details from osism.tasks.openstack import get_cloud_helpers @@ -181,7 +247,12 @@ def take_action(self, parsed_args): if maintenance: query.update(dict(maintenance=maintenance)) - baremetal = conn.baremetal.nodes(**query) + if details: + from osism.utils.ironic import list_detailed_nodes + + baremetal = list_detailed_nodes(conn, **query) + else: + baremetal = conn.baremetal.nodes(**query) result = [] for b in baremetal: @@ -192,6 +263,14 @@ def take_action(self, parsed_args): b["provision_state"], b["maintenance"], ] + if details: + row += [ + _format_state_since(b["provision_updated_at"]), + b["target_provision_state"] or "", + _format_automated_clean(b["automated_clean"]), + _format_maintenance_reason(b), + _shorten_last_error(b["last_error"]), + ] result.append(row) result.sort(key=lambda x: x[0]) @@ -203,6 +282,14 @@ def take_action(self, parsed_args): "Provision State", "Maintenance", ] + if details: + headers += [ + "State Since (UTC)", + "Target State", + "Automated Clean", + "Maintenance Reason", + "Last Error", + ] if include_netbox and utils.nb: from osism.tasks.openstack import get_baremetal_node_netbox_info diff --git a/osism/utils/ironic.py b/osism/utils/ironic.py new file mode 100644 index 000000000..33a6a62bf --- /dev/null +++ b/osism/utils/ironic.py @@ -0,0 +1,38 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Ironic helpers that need more than openstacksdk exposes. + +Importing this module imports openstacksdk; import it where it is used, +not at the top of a command module. +""" + +from openstack import resource +from openstack.baremetal.v1.node import Node + + +class DetailedNode(Node): + """An Ironic node that also keeps ``provision_updated_at``. + + Ironic returns ``provision_updated_at`` (when the node entered its + current provision state) with every detailed node, but openstacksdk's + ``Node`` has no attribute for it, so the SDK drops the value while + parsing the response. Declaring the field here keeps it. Remove the + declaration once openstacksdk maps the field itself. + """ + + # openstacksdk looks up the alternate id only in the class's own + # __dict__, not in its bases. Without repeating the declaration a + # subclass of Node loses the "uuid" -> id mapping and every node's id + # is None. + id = resource.Body("uuid", alternate_id=True) + + provision_updated_at = resource.Body("provision_updated_at") + + +def list_detailed_nodes(conn, **query): + """List nodes with all fields, as ``conn.baremetal.nodes(details=True)``. + + This goes through the same request and microversion negotiation as the + SDK's own proxy method; only the resource class differs. + """ + return DetailedNode.list(conn.baremetal, details=True, **query) diff --git a/tests/unit/commands/test_baremetal.py b/tests/unit/commands/test_baremetal.py index 6eedfa3c4..587ee76fb 100644 --- a/tests/unit/commands/test_baremetal.py +++ b/tests/unit/commands/test_baremetal.py @@ -518,6 +518,200 @@ def test_list_netbox_without_connection_skips_extra_column(capsys): assert "Device Role" not in capsys.readouterr().out +def _detailed_node(name, **fields): + from osism.utils.ironic import DetailedNode + + body = { + "uuid": f"uuid-{name}", + "name": name, + "power_state": "power on", + "provision_state": "active", + "maintenance": False, + } + body.update(fields) + return DetailedNode.existing(**body) + + +def _run_list_details(args, nodes): + conn = MagicMock() + with patch( + "osism.utils.ironic.list_detailed_nodes", return_value=nodes + ) as list_detailed: + rc, _ = _run_list(["--details"] + args, conn) + return rc, conn, list_detailed + + +def test_list_without_details_keeps_standard_columns(capsys): + conn = MagicMock() + conn.baremetal.nodes.return_value = [_list_node("node-a")] + + _run_list([], conn) + + out = capsys.readouterr().out + for header in ( + "State Since", + "Target State", + "Automated Clean", + "Maintenance Reason", + "Last Error", + ): + assert header not in out + + +def test_list_details_queries_detailed_nodes_with_filters(): + rc, conn, list_detailed = _run_list_details( + ["--provision-state", "active", "--maintenance"], [] + ) + + list_detailed.assert_called_once_with( + conn, provision_state="active", maintenance=True + ) + conn.baremetal.nodes.assert_not_called() + + +def test_list_details_shows_extra_columns(capsys): + node = _detailed_node( + "node-a", + provision_state="clean failed", + target_provision_state="available", + provision_updated_at="2026-11-10T08:15:42.123456+00:00", + automated_clean=True, + maintenance=True, + maintenance_reason="disk swap", + last_error="Agent did not come back", + ) + + _run_list_details([], [node]) + + out = capsys.readouterr().out + for header in ( + "State Since (UTC)", + "Target State", + "Automated Clean", + "Maintenance Reason", + "Last Error", + ): + assert header in out + assert "uuid-node-a" in out + assert "2026-11-10 08:15" in out + assert "clean failed" in out + assert "available" in out + assert "disk swap" in out + assert "Agent did not come back" in out + + +def test_list_details_state_since_is_converted_to_utc(capsys): + node = _detailed_node("node-a", provision_updated_at="2026-11-10T10:15:00+02:00") + + _run_list_details([], [node]) + + assert "2026-11-10 08:15" in capsys.readouterr().out + + +def test_list_details_unset_automated_clean_shows_default(capsys): + # Ironic's null means "follow the conductor's [conductor]automated_clean". + node = _detailed_node("node-a", automated_clean=None) + + _run_list_details([], [node]) + + assert "default" in capsys.readouterr().out + + +def test_list_details_maintenance_reason_falls_back_to_fault(capsys): + node = _detailed_node("node-a", maintenance=True, fault="power failure") + + _run_list_details([], [node]) + + assert "power failure" in capsys.readouterr().out + + +def test_list_details_last_error_flattened_and_shortened(capsys): + error = "first line\nsecond line " + "x" * 200 + node = _detailed_node("node-a", last_error=error) + + _run_list_details([], [node]) + + shortened = "first line second line " + "x" * 54 + "..." + assert len(shortened) == 80 + out = capsys.readouterr().out + assert f" {shortened} " in out + assert "x" * 55 not in out + + +def test_list_details_free_text_control_characters_neutralised(capsys): + # Free-text node fields must not carry terminal escape sequences (ESC, + # OSC's BEL, C1 CSI) through to the operator's terminal. + node = _detailed_node( + "node-a", + maintenance=True, + maintenance_reason="swap\x1b]0;owned\x07disk", + last_error="boom\x1b[2J\x9b31mred", + ) + + _run_list_details([], [node]) + + out = capsys.readouterr().out + for char in ("\x1b", "\x07", "\x9b"): + assert char not in out + assert "swap ]0;owned disk" in out + assert "boom [2J 31mred" in out + + +def test_detailed_node_keeps_provision_updated_at_and_uuid(): + from osism.utils.ironic import DetailedNode + + node = DetailedNode.existing( + uuid="11111111-2222-3333-4444-555555555555", + provision_updated_at="2026-11-10T08:15:42+00:00", + ) + + assert node["id"] == "11111111-2222-3333-4444-555555555555" + assert node["provision_updated_at"] == "2026-11-10T08:15:42+00:00" + + +def test_list_detailed_nodes_requests_node_detail_endpoint(): + # Runs openstacksdk's real list code against a mocked baremetal proxy: + # the request must go to /nodes/detail (not the summary /nodes), keep + # the filter, negotiate the microversion, and keep both the uuid and + # provision_updated_at of the response. + from openstack.baremetal.v1 import _proxy + + from osism.utils.ironic import list_detailed_nodes + + response = MagicMock(status_code=200, headers={}, links={}) + response.json.return_value = { + "nodes": [ + { + "uuid": "11111111-2222-3333-4444-555555555555", + "name": "node-a", + "provision_updated_at": "2026-11-10T08:15:42+00:00", + } + ] + } + proxy = MagicMock(spec=_proxy.Proxy) + proxy.default_microversion = None + proxy.get_endpoint_data.return_value = MagicMock( + min_microversion="1.1", max_microversion="1.109" + ) + proxy.get.return_value = response + conn = MagicMock() + conn.baremetal = proxy + + nodes = list(list_detailed_nodes(conn, provision_state="active")) + + proxy.get.assert_called_once() + args, kwargs = proxy.get.call_args + assert args == ("/nodes/detail",) + assert kwargs["params"] == {"provision_state": "active"} + assert kwargs["microversion"] == "1.109" + assert [(n["id"], n["provision_updated_at"]) for n in nodes] == [ + ( + "11111111-2222-3333-4444-555555555555", + "2026-11-10T08:15:42+00:00", + ) + ] + + def test_list_cleanup_called_when_listing_fails(): cmd = baremetal.BaremetalList(MagicMock(), MagicMock()) parsed_args = cmd.get_parser("test").parse_args([])