From f6f97237390fba46cc38ec29b452ea3494ed2775 Mon Sep 17 00:00:00 2001 From: haseeb Date: Mon, 26 Oct 2026 15:48:30 +0530 Subject: [PATCH] refactor: trim enroll to post_inspection_runbooks + post-enroll hook Strip BMC init/password, out-of-band and agent inspection, Dell BIOS, RAID, and firmware from the default enroll() flow. The trimmed flow runs chassis_info (read-only, for node naming) -> create_or_update -> the run_post_inspection_runbook loop -> transition(provide/available), and main() still writes --node-uuid-output on success for the post-enroll hook. All CLI args and parse_bool/parse_runbooks are preserved; old_password/firmware_update/raid_configure stay in the signature but are inert. Update tests: remove the two full-sequence tests that asserted removed paths, rewrite test_post_inspection_runbook_order and test_post_inspection_runbooks_stop_on_failure for the trimmed flow, preserve all in-scope coverage. enroll-server.yaml: doc note only; contract/params/output/args/mounts unchanged. Gates: ruff + uv build + pytest (822 passed) green; helm lint/template, kustomize renders green; deploy-repo argo-lint/unittest unchanged vs baseline. --- .../tests/test_enroll_server.py | 334 ++---------------- .../main/enroll_server.py | 148 +------- 2 files changed, 45 insertions(+), 437 deletions(-) diff --git a/python/understack-workflows/tests/test_enroll_server.py b/python/understack-workflows/tests/test_enroll_server.py index f2cec7911..0c5eedbc4 100644 --- a/python/understack-workflows/tests/test_enroll_server.py +++ b/python/understack-workflows/tests/test_enroll_server.py @@ -104,17 +104,6 @@ def make_bmc(mocker, fake_sushy=None): return bmc -def make_raid_hardware(): - controller = SimpleNamespace( - identity="RAID.Integrated.1-1", - drives=[SimpleNamespace(identity="Disk1"), SimpleNamespace(identity="Disk2")], - ) - system = SimpleNamespace(storage=SimpleNamespace(get_members=lambda: [controller])) - return SimpleNamespace( - get_system_collection=lambda: SimpleNamespace(get_members=lambda: [system]) - ) - - def make_ironic_client( *, node_name: str, @@ -178,263 +167,6 @@ def make_port(*, address: str, pxe_enabled: bool, bios_name: str | None): ) -def test_enrol_happy_path_uses_virtual_media_inspect_and_flips_back(mocker): - device_info = make_device_info() - fake_bmc = make_bmc(mocker, fake_sushy=make_raid_hardware()) - inventory = make_node_inventory( - connected_interface_names=["NIC.Integrated.1-1", "NIC.Integrated.1-2"], - storage_controllers=[ - { - "id": "RAID.Integrated.1-1", - "drives": [ - {"id": "Disk1", "size": 479559942144}, - {"id": "Disk2", "size": 479559942144}, - ], - } - ], - ) - # port-enroll-config hook has run during agent inspection and flagged - # pxe_enabled on LLDP-connected ports. port-bios-name (OOB) stamped - # extra.bios_name earlier. - ports = [ - make_port( - address="aa:aa:aa:aa:aa:01", - pxe_enabled=True, - bios_name="NIC.Integrated.1-1", - ), - make_port( - address="aa:aa:aa:aa:aa:02", - pxe_enabled=True, - bios_name="NIC.Integrated.1-2", - ), - make_port( - address="aa:aa:aa:aa:aa:03", - pxe_enabled=False, - bios_name="NIC.Slot.1-1", - ), - ] - fake_ironic, created_node = make_ironic_client( - node_name="Dell-ABC123", - # OOB inspect, agent inspect, OOB inspect (post-RAID). - inspect_interfaces=["idrac-redfish", "idrac-redfish", "idrac-redfish"], - inventory=inventory, - ports=ports, - ) - - mocker.patch.object(enroll_server, "bmc_for_ip_address", return_value=fake_bmc) - mocker.patch.object(enroll_server, "chassis_info", return_value=device_info) - set_bmc_password = mocker.patch.object(enroll_server, "set_bmc_password") - update_dell_drac_settings = mocker.patch.object( - enroll_server, "update_dell_drac_settings" - ) - bmc_set_hostname = mocker.patch.object(enroll_server, "bmc_set_hostname") - update_dell_bios_settings = mocker.patch.object( - enroll_server, "update_dell_bios_settings" - ) - mocker.patch( - "understack_workflows.ironic.client.get_ironic_client", - return_value=fake_ironic, - ) - - enroll_server.enroll( - ip_address="10.0.0.10", - firmware_update=False, - raid_configure=True, - old_password="old-password", - external_cmdb_id="cmdb-1", - ) - - set_bmc_password.assert_called_once_with( - ip_address="10.0.0.10", - new_password="calvin", - old_password="old-password", - ) - update_dell_drac_settings.assert_called_once_with(fake_bmc) - bmc_set_hostname.assert_called_once_with(fake_bmc, "None", "Dell-ABC123") - - # BIOS settings configured exactly once, using pxe_enabled port BIOS names. - update_dell_bios_settings.assert_any_call( - fake_bmc, - pxe_interface="NIC.Integrated.1-1", - ) - - fake_ironic.node.create.assert_called_once_with( - name="Dell-ABC123", - driver="idrac", - driver_info={ - "redfish_address": "https://10.0.0.10", - "redfish_verify_ca": False, - "redfish_username": "root", - "redfish_password": "calvin", - }, - boot_interface="http-ipxe", - deploy_interface="direct", - inspect_interface="idrac-redfish", - extra={"external_cmdb_id": "cmdb-1"}, - ) - - fake_ironic.node.set_target_raid_config.assert_called_once_with( - created_node.uuid, - { - "logical_disks": [ - { - "controller": "RAID.Integrated.1-1", - "is_root_volume": True, - "physical_disks": ["Disk1", "Disk2"], - "raid_level": "1", - "size_gb": "MAX", - } - ] - }, - ) - - fake_ironic.node.set_provision_state.assert_has_calls( - [ - call( - created_node.uuid, - "manage", - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - call( - created_node.uuid, - "inspect", # OOB redfish inspect for bios_name / basic info - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - call( - created_node.uuid, - "inspect", # agent inspect via virtual media - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - call( - created_node.uuid, - "clean", - cleansteps=[ - {"interface": "raid", "step": "delete_configuration"}, - {"interface": "raid", "step": "create_configuration"}, - ], - runbook=None, - disable_ramdisk=False, - ), - call( - created_node.uuid, - "inspect", # OOB redfish inspect to refresh disks post-RAID - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - call( - created_node.uuid, - "provide", - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - ] - ) - - expected_reset = [{"op": "remove", "path": "/inspect_interface"}] - expected_ipxe_boot = [ - {"op": "add", "path": "/boot_interface", "value": "http-ipxe"} - ] - expected_agent = [{"op": "add", "path": "/inspect_interface", "value": "agent"}] - assert fake_ironic.node.update.call_args_list == [ - call(created_node.uuid, expected_reset), # OOB inspect prep - call(created_node.uuid, expected_ipxe_boot), - call(created_node.uuid, expected_agent), - call(created_node.uuid, expected_ipxe_boot), - call(created_node.uuid, expected_reset), # Post-RAID OOB inspect prep - ] - - -def test_enrol_existing_failed_node_recovers_and_updates(mocker): - device_info = make_device_info() - existing_node = SimpleNamespace( - uuid="node-999", - provision_state="inspect failed", - driver="idrac", - inspect_interface="idrac-redfish", - ) - inventory = make_node_inventory( - connected_interface_names=["NIC.Integrated.1-1"], - ) - ports = [ - make_port( - address="aa:aa:aa:aa:aa:01", - pxe_enabled=True, - bios_name="NIC.Integrated.1-1", - ), - ] - fake_bmc = make_bmc(mocker) - fake_ironic, _ = make_ironic_client( - node_name="Dell-ABC123", - node_uuid="node-999", - existing_node=existing_node, - inspect_interfaces=["idrac-redfish", "idrac-redfish"], - inventory=inventory, - ports=ports, - ) - - mocker.patch.object(enroll_server, "bmc_for_ip_address", return_value=fake_bmc) - mocker.patch.object(enroll_server, "chassis_info", return_value=device_info) - mocker.patch.object(enroll_server, "set_bmc_password") - mocker.patch.object(enroll_server, "update_dell_drac_settings") - mocker.patch.object(enroll_server, "bmc_set_hostname") - mocker.patch.object(enroll_server, "update_dell_bios_settings") - mocker.patch( - "understack_workflows.ironic.client.get_ironic_client", - return_value=fake_ironic, - ) - - enroll_server.enroll( - ip_address="10.0.0.10", - firmware_update=False, - raid_configure=False, - old_password=None, - external_cmdb_id="cmdb-1", - ) - - fake_ironic.node.create.assert_not_called() - fake_ironic.node.set_target_raid_config.assert_not_called() - fake_ironic.node.set_provision_state.assert_has_calls( - [ - call( - existing_node.uuid, - "manage", - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - call( - existing_node.uuid, - "inspect", # OOB inspect - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - call( - existing_node.uuid, - "inspect", # Agent inspect via virtual media - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - call( - existing_node.uuid, - "provide", - cleansteps=None, - runbook=None, - disable_ramdisk=None, - ), - ] - ) - - def test_apply_firmware_updates_runs_traits_in_numeric_order(mocker): fake_ironic, _ = make_ironic_client( node_name="unused", @@ -605,23 +337,22 @@ def test_post_inspection_runbook_order(mocker, runbooks): node = SimpleNamespace(uuid="node-123") bmc = make_bmc(mocker) mocker.patch.object(enroll_server, "bmc_for_ip_address", return_value=bmc) - mocker.patch.object( - enroll_server, "initialize_bmc", return_value=make_device_info() - ) - mocker.patch.object( - enroll_server.ironic_node, "create_or_update", return_value=node - ) - mocker.patch.object(enroll_server.ironic_node, "inspect_out_of_band") - mocker.patch.object( - enroll_server.ironic_node, "get_node_inventory", return_value={} - ) - mocker.patch.object( - enroll_server.ironic_node, "pxe_enabled_bios_name", return_value="NIC.1" + # The trimmed enroll() derives the node name from a read-only chassis read. + mocker.patch.object(enroll_server, "chassis_info", return_value=make_device_info()) + # RAID/firmware steps were removed from enroll(); keep apply_firmware_updates + # patched so we can positively assert it is never called (flags are inert). + apply_firmware_updates = mocker.patch.object( + enroll_server.ironic_node, "apply_firmware_updates" ) + events = MagicMock() - for name in ("agent_inspection", "update_dell_bios_settings", "configure_raid"): - events.attach_mock(mocker.patch.object(enroll_server, name), name) - for name in ("run_post_inspection_runbook", "apply_firmware_updates", "transition"): + events.attach_mock( + mocker.patch.object( + enroll_server.ironic_node, "create_or_update", return_value=node + ), + "create_or_update", + ) + for name in ("run_post_inspection_runbook", "transition"): events.attach_mock(mocker.patch.object(enroll_server.ironic_node, name), name) result = enroll_server.enroll( @@ -633,22 +364,23 @@ def test_post_inspection_runbook_order(mocker, runbooks): ) expected = [ - call.update_dell_bios_settings(bmc, pxe_interface="NIC.1"), - call.agent_inspection(node, virtual_media=False), - call.update_dell_bios_settings(bmc, pxe_interface="NIC.1"), + call.create_or_update( + bmc=bmc, + name="Dell-ABC123", + manufacturer="Dell", + external_cmdb_id=None, + ) ] expected.extend( call.run_post_inspection_runbook(node, runbook) for runbook in runbooks or [] ) - expected.extend( - [ - call.configure_raid(node, {}), - call.apply_firmware_updates(node), - call.transition(node, target_state="provide", expected_state="available"), - ] + expected.append( + call.transition(node, target_state="provide", expected_state="available") ) assert events.mock_calls == expected assert result == node.uuid + # firmware_update=True / raid_configure=True are inert in the trimmed flow. + apply_firmware_updates.assert_not_called() def test_post_inspection_runbook_uses_uuid_and_waits(mocker): @@ -733,28 +465,15 @@ def test_post_inspection_runbooks_reject_invalid_cli(value): def test_post_inspection_runbooks_stop_on_failure(mocker): node = SimpleNamespace(uuid="node-123") mocker.patch.object(enroll_server, "bmc_for_ip_address") - mocker.patch.object( - enroll_server, "initialize_bmc", return_value=make_device_info() - ) + mocker.patch.object(enroll_server, "chassis_info", return_value=make_device_info()) mocker.patch.object( enroll_server.ironic_node, "create_or_update", return_value=node ) - mocker.patch.object(enroll_server.ironic_node, "inspect_out_of_band") - mocker.patch.object( - enroll_server.ironic_node, "get_node_inventory", return_value={} - ) - mocker.patch.object( - enroll_server.ironic_node, "pxe_enabled_bios_name", return_value="NIC.1" - ) - mocker.patch.object(enroll_server, "agent_inspection") - mocker.patch.object(enroll_server, "update_dell_bios_settings") runbook = mocker.patch.object( enroll_server.ironic_node, "run_post_inspection_runbook", side_effect=RuntimeError("clean failed"), ) - raid = mocker.patch.object(enroll_server, "configure_raid") - firmware = mocker.patch.object(enroll_server.ironic_node, "apply_firmware_updates") transition = mocker.patch.object(enroll_server.ironic_node, "transition") with pytest.raises(RuntimeError, match="clean failed"): @@ -766,9 +485,8 @@ def test_post_inspection_runbooks_stop_on_failure(mocker): post_inspection_runbooks=["first", "second"], ) + # The loop stops at the first failing runbook; transition never runs. runbook.assert_called_once_with(node, "first") - raid.assert_not_called() - firmware.assert_not_called() transition.assert_not_called() diff --git a/python/understack-workflows/understack_workflows/main/enroll_server.py b/python/understack-workflows/understack_workflows/main/enroll_server.py index b58ad4209..8dce37157 100644 --- a/python/understack-workflows/understack_workflows/main/enroll_server.py +++ b/python/understack-workflows/understack_workflows/main/enroll_server.py @@ -6,60 +6,42 @@ import os from pathlib import Path -from ironicclient.v1.node import Node - from understack_workflows import helpers from understack_workflows import ironic_node -from understack_workflows.bmc import Bmc from understack_workflows.bmc import bmc_for_ip_address -from understack_workflows.bmc_bios import update_dell_bios_settings from understack_workflows.bmc_chassis_info import ChassisInfo from understack_workflows.bmc_chassis_info import chassis_info -from understack_workflows.bmc_credentials import set_bmc_password -from understack_workflows.bmc_hostname import bmc_set_hostname -from understack_workflows.bmc_settings import update_dell_drac_settings -from understack_workflows.raid import configure_raid logger = logging.getLogger(__name__) -MAX_PXE_INTERFACES = 8 - def main() -> None: """On-board a new baremetal node, or refresh an existing one. - - Connect to the BMC using standard password, if that fails then use the - password supplied in --old-bmc-password option, or factory default. + TRIMMED flow (branch `trim-enroll-post-inspection`): this build exercises + ONLY the post_inspection_runbooks loop and the post-enroll hook. BMC + initialization/password, out-of-band and agent inspection, Dell BIOS + configuration, RAID, and firmware steps have been removed from the default + enroll() flow. The remaining steps are: - - Ensure standard BMC password is set, and other basic BMC settings. - - - From BMC, discover basic chassis info: manufacturer, model, serial. + - From the BMC, read basic chassis info (manufacturer, serial) with a + read-only Redfish query to derive the node name. - Find or create this baremetal node in Ironic: - - Set the name to "{manufacturer}-{servicetag}". + - Set the name to "{manufacturer}-{serial_number}". - Set the driver as appropriate for the manufacturer/model. - Set the node's external_cmdb_id, if one was provided. - - Perform agent inspection via virtual-media boot to capture the full - hardware inventory and LLDP-confirmed switch topology. - - Using virtual-media avoids having to configure PXE during the initial - phase, however the IPA agent still requires DHCP autoconfiguration, so it - needs to be connected to the provisioning VLAN, and it needs at least one - port in Neutron to be configured for "enroll". + - Execute post-inspection runbooks in the supplied order. - - Configure the Dell HTTP-boot BIOS entries using the LLDP-confirmed PXE - interfaces, then flip the node back to its final production http-ipxe boot - mode for cleaning and provisioning (we prefer PXE-based booting over - virtual media, for performance reasons). - - - Optionally execute post-inspection runbooks in the supplied order. - - - Optionally configure RAID. + - Transition the node to the 'available' state (implies cleaning). - - Optionally apply firmware updates. + - On success, write the node UUID to --node-uuid-output (consumed by the + optional post-enroll hook). - - Transition the node to the 'available' state (implies cleaning). + The --old-password, --firmware-update, and --raid-configure arguments are + still parsed (the Argo template passes them) but are inert in the trimmed + flow. """ helpers.setup_logger() args = argument_parser().parse_args() @@ -85,13 +67,14 @@ def enroll( external_cmdb_id: int | str | None = None, post_inspection_runbooks: list[str] | None = None, ) -> str: - logger.info("Starting enroll workflow for bmc_ip_address=%s", ip_address) + logger.info("Starting (trimmed) enroll workflow for bmc_ip_address=%s", ip_address) if external_cmdb_id: logger.info(" external_cmdb_id=%s", external_cmdb_id) bmc = bmc_for_ip_address(ip_address) - device_info = initialize_bmc(bmc, old_password) + # Read-only Redfish query to derive the node name. No BMC mutation. + device_info = chassis_info(bmc) node = ironic_node.create_or_update( bmc=bmc, @@ -100,107 +83,14 @@ def enroll( external_cmdb_id=external_cmdb_id, ) - # Out-of-band redfish inspection populates data including baremetal ports. - # - # Our hooks augment the ironic baremetal port with the BMC-reported - # interface name (e.g. NIC.Integrated.1-1) as well as some placeholder - # "enrol" dummy data that is required by Ironic/Neutron to perform agent - # inspection. Neutron needs to assign a port to the provisioning network - # and it bails out unless we have ports with pxe_enabled, local_link_info, - # etc. - ironic_node.inspect_out_of_band(node) - inventory = ironic_node.get_node_inventory(node) - - # Agent inspection gathers LLDP and full hardware inventory. - # - # Virtual media boot makes Neutron behave differently to normal HTTP agent - # boot - in our normal configuration it fails to set up ports on the - # provisioning network. - # - # Therefore, we only use virtual media during our "enroll" phase, when the - # port data is set up in a manner that suits the Neutron algorithm. If a - # normal PXE/HTTP port is available then we use it instead: - - pxe_interface = ironic_node.pxe_enabled_bios_name(node) - update_dell_bios_settings(bmc, pxe_interface=pxe_interface) - agent_inspection(node, virtual_media=not pxe_interface) - - pxe_interface = ironic_node.pxe_enabled_bios_name(node) - if not pxe_interface: - raise RuntimeError( - f"[node:{node.uuid}] Agent inspection produced no pxe_enabled ports " - "cannot configure HTTP boot." - ) - logger.info("[node:%s] Selected PXE interface %s", node.uuid, pxe_interface) - - update_dell_bios_settings(bmc, pxe_interface=pxe_interface) - for runbook in post_inspection_runbooks or []: ironic_node.run_post_inspection_runbook(node, runbook) - if raid_configure: - configure_raid(node, inventory) - # RAID reconfiguration changes the disk layout; refresh inventory. - ironic_node.inspect_out_of_band(node) - else: - logger.info("%s RAID configuration was not requested", node.uuid) - - if firmware_update: - ironic_node.apply_firmware_updates(node) - ironic_node.transition(node, target_state="provide", expected_state="available") - logger.info("Completed enroll workflow for bmc_ip_address=%s", ip_address) + logger.info("Completed (trimmed) enroll workflow for bmc_ip_address=%s", ip_address) return str(node.uuid) -def agent_inspection(node: Node, virtual_media=False) -> None: - """Run agent inspection booted, optionally via virtual media. - - If requested, the node is temporarily flipped to a virtual-media - boot_interface so the agent ramdisk boots from a BMC-attached ISO rather - than via PXE. After initial inspection, the boot_interface is set back to - the steady-state value used for cleaning and provisioning. - - If cleaning fails we don't reset the boot_interface (as Ironic won't let us - change that attribute in the inspect_failed state). - """ - if virtual_media: - refreshed = ironic_node.refresh(node, fields=["driver"]) - boot_interface = ironic_node.virtual_media_boot_interface_for(refreshed.driver) - else: - boot_interface = ironic_node.STEADY_STATE_BOOT_INTERFACE - - logger.info( - "[node:%s] Switching boot_interface to %s for agent inspection", - node.uuid, - boot_interface, - ) - ironic_node.patch(node, [f"boot_interface={boot_interface}"]) - ironic_node.inspect(node) - ironic_node.patch( - node, [f"boot_interface={ironic_node.STEADY_STATE_BOOT_INTERFACE}"] - ) - - -def initialize_bmc(bmc: Bmc, old_password: str | None) -> ChassisInfo: - """Discover and configure BMC with Undercloud standard settings.""" - set_bmc_password( - ip_address=bmc.ip_address, - new_password=bmc.password, - old_password=old_password, - ) - - device_info = chassis_info(bmc) - for line in device_info.dump: - logger.info("Discovered %s", line) - - if device_info.manufacturer == "Dell": - update_dell_drac_settings(bmc) - - bmc_set_hostname(bmc, device_info.bmc_hostname, device_name(device_info)) - return device_info - - def device_name(device_info: ChassisInfo) -> str: return f"{device_info.manufacturer}-{device_info.serial_number}"