diff --git a/ironic/drivers/modules/drac/boot.py b/ironic/drivers/modules/drac/boot.py index dd91ad5e2e..0420e255b2 100644 --- a/ironic/drivers/modules/drac/boot.py +++ b/ironic/drivers/modules/drac/boot.py @@ -69,6 +69,59 @@ class DracRedfishVirtualMediaBoot(redfish_boot.RedfishVirtualMediaBoot): def _validate_vendor(self, task, managers): pass # assume people are doing the right thing + @staticmethod + def _get_idrac_version_from_model(model): + """Extract iDRAC version from the hardware model string. + + :param model: The hardware model string from the manager. + :returns: The iDRAC version as an integer, or None if unable to + determine. + """ + if not model: + return None + + try: + generation = int(model[:2]) + # Map hardware generation to iDRAC version + if generation > 16: + return 10 # iDRAC 10 + elif generation in (16, 15, 14): + return 9 # iDRAC 9 + elif generation in (12, 13): + return 8 # iDRAC 8 + else: + return None # Unknown or unsupported version + except (ValueError, TypeError): + LOG.debug("Unable to parse iDRAC version from model string: %s", + model) + return None + + def _get_acceptable_media_id(self, task, resource): + """Get acceptable virtual media IDs for iDRAC systems. + + For iDRAC10 systems, only virtual media ID "1" is acceptable + for virtual media insertion due to hardware limitations. + + :param task: A TaskManager instance containing the node to act on. + :param resource: A redfish resource (System or Manager) containing + virtual media. + :returns: "1" for iDRAC10 systems, None otherwise. + """ + # In case the resource is System, we need to check the managers + if resource.managers: + for manager in resource.managers: + # Check the iDRAC version based on the hardware model + if manager.model: + idrac_version = self._get_idrac_version_from_model( + manager.model) + if idrac_version == 10: + return "1" + return None + else: + # In case the resource is Manager, we don't need to check anything. + # iDRAC10 doesn't have Virtual Media in Managers, so we ignore. + return None + @classmethod def _set_boot_device(cls, task, device, persistent=False): """Set boot device for a node. diff --git a/ironic/drivers/modules/redfish/boot.py b/ironic/drivers/modules/redfish/boot.py index 2ecdba9e13..1a4a0c5372 100644 --- a/ironic/drivers/modules/redfish/boot.py +++ b/ironic/drivers/modules/redfish/boot.py @@ -301,7 +301,30 @@ def _insert_vmedia_in_resource(task, resource, boot_url, boot_device, :raises: InvalidParameterValue, if no suitable virtual CD or DVD is found on the node. """ + # Get acceptable virtual media IDs from the boot interface + # This allows vendor-specific implementations to restrict which + # virtual media devices can be used + try: + acceptable_id = task.driver.boot._get_acceptable_media_id(task, + resource) + except AttributeError: + LOG.warning("Boot interface %s does not implement " + "_get_acceptable_media_id method. This method should be " + "implemented for compatibility.", + task.driver.boot.__class__.__name__) + acceptable_id = None + for v_media in resource.virtual_media.get_members(): + # Skip virtual media if the ID is not in the acceptable set + if acceptable_id is not None and v_media.identity != acceptable_id: + LOG.debug("Boot Interface %(name)s returned %(acceptable_id)s as " + "Virtual Media Slot ID, current slot is %(slot)s, " + "skipping", + {'name': task.driver.boot.__class__.__name__, + 'acceptable_id': acceptable_id, + 'slot': v_media.identity}) + continue + if boot_device not in v_media.media_types: # NOTE(janders): this conditional allows v_media that only # support DVD MediaType and NOT CD to also be used. @@ -671,6 +694,20 @@ class RedfishVirtualMediaBoot(base.BootInterface): 'node': task.node.uuid, 'vendor': vendor, 'fwv': bmc_manager[0].firmware_version}) + def _get_acceptable_media_id(self, task, resource): + """Get acceptable virtual media IDs for the resource. + + This method can be overridden by vendor-specific implementations + to return specific virtual media IDs that should be used. + + :param task: A TaskManager instance containing the node to act on. + :param resource: A redfish resource (System or Manager) containing + virtual media. + :returns: An acceptable virtual media ID, or None if any ID + is acceptable. + """ + return None + def validate(self, task): """Validate the deployment information for the task's node. diff --git a/ironic/tests/unit/drivers/modules/drac/test_boot.py b/ironic/tests/unit/drivers/modules/drac/test_boot.py index 5f18a44b23..e3d8ddedc2 100644 --- a/ironic/tests/unit/drivers/modules/drac/test_boot.py +++ b/ironic/tests/unit/drivers/modules/drac/test_boot.py @@ -25,6 +25,7 @@ import sushy from ironic.common import boot_devices from ironic.conductor import task_manager from ironic.drivers.modules import deploy_utils +from ironic.drivers.modules.drac import boot as idrac_boot from ironic.drivers.modules.redfish import utils as redfish_utils from ironic.tests.unit.db import utils as db_utils from ironic.tests.unit.drivers.modules.drac import utils as test_utils @@ -112,3 +113,83 @@ class DracBootTestCase(test_utils.BaseDracTest): task.driver.boot._set_boot_device(task, boot_devices.DISK) self.assertFalse(mock_system.called) + + @mock.patch.object(idrac_boot.LOG, 'debug', autospec=True) + def test__get_idrac_version_from_model(self, mock_log_debug, + mock_get_system): + # Use shorter alias for readability and line length + idrac_redfish_boot = idrac_boot.DracRedfishVirtualMediaBoot + + # Test cases that should return None without logging + none_models = [None, "", "10G", "10ABC", "10G1XPT"] + for model in none_models: + version = idrac_redfish_boot._get_idrac_version_from_model(model) + self.assertIsNone(version) + + # Test cases causing TypeError/ValueError and should log debug message + error_models = [(5, 6, 7), [1, 2], "ABC", "X1Z", "9GTP"] + + for model in error_models: + version = idrac_redfish_boot._get_idrac_version_from_model(model) + self.assertIsNone(version) + # Verify that debug logging was called for each error model + self.assertEqual(mock_log_debug.call_count, len(error_models)) + # Verify the debug log calls have the correct format + expected_calls = [ + mock.call( + "Unable to parse iDRAC version from model string: %s", model) + for model in error_models + ] + mock_log_debug.assert_has_calls(expected_calls) + + idrac8 = ["12G Modular", "13G Modular"] + idrac9 = ["14G Monolithic", "15G Monolithic", "16G Monolithic", + "16G DCS"] + idrac10 = ["17G Monolithic", "18G Monolithic"] + for model in idrac8: + version = idrac_redfish_boot._get_idrac_version_from_model(model) + self.assertEqual(version, 8) + for model in idrac9: + version = idrac_redfish_boot._get_idrac_version_from_model(model) + self.assertEqual(version, 9) + for model in idrac10: + version = idrac_redfish_boot._get_idrac_version_from_model(model) + self.assertEqual(version, 10) + + def test__get_acceptable_media_id(self, mock_get_system): + + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + + # Case 1: System resource with manager model "17G Monolithic" + # (iDRAC 10) returns "1" + mock_manager_idrac10 = mock.Mock() + mock_manager_idrac10.model = "17G Monolithic" + mock_system_resource = mock.Mock() + mock_system_resource.managers = [mock_manager_idrac10] + + result = task.driver.boot._get_acceptable_media_id( + task, mock_system_resource) + self.assertEqual("1", result) + + # Case 2: System resource with manager model "16G Monolithic" + # (iDRAC 9) returns None + mock_manager_idrac9 = mock.Mock() + mock_manager_idrac9.model = "16G Monolithic" + mock_system_resource_idrac9 = mock.Mock() + mock_system_resource_idrac9.managers = [mock_manager_idrac9] + + result = task.driver.boot._get_acceptable_media_id( + task, mock_system_resource_idrac9) + self.assertIsNone(result) + + # Case 3: Manager resource with model "15G Monolithic" + # Should return None (Manager resources) + mock_manager_resource = mock.Mock() + mock_manager_resource.model = "15G Monolithic" + # Simulate Manager resource by setting managers to None + mock_manager_resource.managers = None + + result = task.driver.boot._get_acceptable_media_id( + task, mock_manager_resource) + self.assertIsNone(result) diff --git a/ironic/tests/unit/drivers/modules/redfish/test_boot.py b/ironic/tests/unit/drivers/modules/redfish/test_boot.py index cef06aa13f..c32d7ccbf1 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_boot.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_boot.py @@ -3007,3 +3007,96 @@ class RedfishVirtualMediaBootViaSystemTestCase(db_base.DbTestCase): redfish_boot._has_vmedia_device( [mock_manager], sushy.VIRTUAL_MEDIA_FLOPPY, inserted=True, system=mock_system)) + + def test__insert_vmedia_in_resource_skips_unacceptable_ids(self): + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + + # Mock the boot interface to return acceptable_id "1" + task.driver.boot._get_acceptable_media_id = mock.MagicMock( + return_value="1") + + # Create two virtual media devices with IDs "2" and "3" + mock_vmedia_2 = mock.MagicMock() + mock_vmedia_2.identity = "2" + mock_vmedia_2.media_types = [sushy.VIRTUAL_MEDIA_CD] + mock_vmedia_2.inserted = False + + mock_vmedia_3 = mock.MagicMock() + mock_vmedia_3.identity = "3" + mock_vmedia_3.media_types = [sushy.VIRTUAL_MEDIA_CD] + mock_vmedia_3.inserted = False + + # Mock resource with virtual media collection + mock_resource = mock.MagicMock() + mock_resource.virtual_media.get_members.return_value = [ + mock_vmedia_2, mock_vmedia_3 + ] + + # Create error messages list + err_msgs = [] + + # Call _insert_vmedia_in_resource + result = redfish_boot._insert_vmedia_in_resource( + task, mock_resource, 'http://example.com/boot.iso', + sushy.VIRTUAL_MEDIA_CD, err_msgs) + + # Should return False since no acceptable media found + self.assertFalse(result) + + # Verify that _get_acceptable_media_id was called + task.driver.boot._get_acceptable_media_id.assert_called_once_with( + task, mock_resource) + + # Verify that no insert_media was called on either device + # since they were skipped due to unacceptable IDs + mock_vmedia_2.insert_media.assert_not_called() + mock_vmedia_3.insert_media.assert_not_called() + + def test__insert_vmedia_in_resource_uses_acceptable_id(self): + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + + # Mock the boot interface to return acceptable_id "1" + task.driver.boot._get_acceptable_media_id = mock.MagicMock( + return_value="1") + + # Create two virtual media devices with IDs "2" and "1" + mock_vmedia_2 = mock.MagicMock() + mock_vmedia_2.identity = "2" + mock_vmedia_2.media_types = [sushy.VIRTUAL_MEDIA_CD] + mock_vmedia_2.inserted = False + + mock_vmedia_1 = mock.MagicMock() + mock_vmedia_1.identity = "1" + mock_vmedia_1.media_types = [sushy.VIRTUAL_MEDIA_CD] + mock_vmedia_1.inserted = False + + # Mock resource with virtual media collection + mock_resource = mock.MagicMock() + mock_resource.virtual_media.get_members.return_value = [ + mock_vmedia_2, mock_vmedia_1 + ] + + # Create error messages list + err_msgs = [] + + # Call _insert_vmedia_in_resource + result = redfish_boot._insert_vmedia_in_resource( + task, mock_resource, 'http://example.com/boot.iso', + sushy.VIRTUAL_MEDIA_CD, err_msgs) + + # Should return True since acceptable media was found and used + self.assertTrue(result) + + # Verify that _get_acceptable_media_id was called + task.driver.boot._get_acceptable_media_id.assert_called_once_with( + task, mock_resource) + + # Verify that first device was skipped (ID "2" != acceptable "1") + mock_vmedia_2.insert_media.assert_not_called() + + # Verify that second device was used (ID "1" == acceptable "1") + mock_vmedia_1.insert_media.assert_called_once_with( + 'http://example.com/boot.iso', inserted=True, + write_protected=True) diff --git a/releasenotes/notes/idrac10-vmedia-slot1-5d15433b84843a10.yaml b/releasenotes/notes/idrac10-vmedia-slot1-5d15433b84843a10.yaml new file mode 100644 index 0000000000..354238112e --- /dev/null +++ b/releasenotes/notes/idrac10-vmedia-slot1-5d15433b84843a10.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes an issue where some iDRAC10 machines requires a specific Virtual + Media Slot. Ironic will attempt to automatically identify the version, + and ensures the right slot is used to insert the iso. + See `bug 2125571 `_ + for details.