diff --git a/ironic/drivers/modules/redfish/inspect.py b/ironic/drivers/modules/redfish/inspect.py index 7ca9624af8..507a90bff8 100644 --- a/ironic/drivers/modules/redfish/inspect.py +++ b/ironic/drivers/modules/redfish/inspect.py @@ -134,15 +134,15 @@ class RedfishInspect(base.InspectInterface): {'node': task.node.uuid}) inspected_properties['local_gb'] = '0' - if system.simple_storage: - simple_storage_list = system.simple_storage.get_members() + if storages := system.storage or system.simple_storage: disks = list() - - for simple_storage in simple_storage_list: - for simple_storage_device in simple_storage.devices: + for storage in storages.get_members(): + drives = storage.drives if hasattr( + storage, 'drives') else storage.devices + for drive in drives: disk = {} - disk['name'] = simple_storage_device.name - disk['size'] = simple_storage_device.capacity_bytes + disk['name'] = drive.name + disk['size'] = drive.capacity_bytes disks.append(disk) inventory['disks'] = disks diff --git a/ironic/drivers/modules/redfish/management.py b/ironic/drivers/modules/redfish/management.py index 8bd7058a55..fcd87487f5 100644 --- a/ironic/drivers/modules/redfish/management.py +++ b/ironic/drivers/modules/redfish/management.py @@ -441,15 +441,18 @@ class RedfishManagement(base.ManagementInterface): """ sensors = {} - for storage in system.simple_storage.get_members(): - for drive in storage.devices: - sensor = cls._sensor2dict( - drive, 'name', 'model', 'capacity_bytes') - sensor.update( - cls._sensor2dict(drive.status, 'state', 'health')) - unique_name = '%s:%s@%s' % ( - drive.name, storage.identity, system.identity) - sensors[unique_name] = sensor + if storages := system.storage or system.simple_storage: + for storage in storages.get_members(): + drives = storage.drives if hasattr( + storage, 'drives') else storage.devices + for drive in drives: + sensor = cls._sensor2dict( + drive, 'name', 'model', 'capacity_bytes') + sensor.update( + cls._sensor2dict(drive.status, 'state', 'health')) + unique_name = '%s:%s@%s' % ( + drive.name, storage.identity, system.identity) + sensors[unique_name] = sensor return sensors @@ -610,11 +613,14 @@ class RedfishManagement(base.ManagementInterface): try: if (component in (None, components.DISK) - and system.simple_storage - and system.simple_storage.drives): + and system.storage): indicators[components.DISK] = { - drive.uuid: properties - for drive in system.simple_storage.drives + # NOTE(vanou) There is no uuid property in Drive resource. + # There is no guarantee Id property of Drive is unique + # across all drives attached to server. + ':'.join([storage.identity, drive.identity]): properties + for storage in system.storage.get_members() + for drive in storage.drives if drive.indicator_led } @@ -656,13 +662,14 @@ class RedfishManagement(base.ManagementInterface): return elif (component == components.DISK - and system.simple_storage - and system.simple_storage.drives): - for drive in system.simple_storage.drives: - if drive.uuid == indicator: - drive.set_indicator_led( - INDICATOR_MAP_REV[state]) - return + and system.storage and len(indicator.split(':')) == 2): + for storage in system.storage.get_members(): + if storage.identity == indicator.split(':')[0]: + for drive in storage.drives: + if drive.identity == indicator.split(':')[1]: + drive.set_indicator_led( + INDICATOR_MAP_REV[state]) + return except sushy.exceptions.SushyError as e: error_msg = (_('Redfish set %(component)s indicator %(indicator)s ' @@ -708,11 +715,12 @@ class RedfishManagement(base.ManagementInterface): return INDICATOR_MAP[chassis.indicator_led] if (component == components.DISK - and system.simple_storage - and system.simple_storage.drives): - for drive in system.simple_storage.drives: - if drive.uuid == indicator: - return INDICATOR_MAP[drive.indicator_led] + and system.storage and len(indicator.split(':')) == 2): + for storage in system.storage.get_members(): + if storage.identity == indicator.split(':')[0]: + for drive in storage.drives: + if drive.identity == indicator.split(':')[1]: + return INDICATOR_MAP[drive.indicator_led] except sushy.exceptions.SushyError as e: error_msg = (_('Redfish get %(component)s indicator %(indicator)s ' diff --git a/ironic/tests/unit/drivers/modules/redfish/test_inspect.py b/ironic/tests/unit/drivers/modules/redfish/test_inspect.py index fe7d9e8d71..e663a0991a 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_inspect.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_inspect.py @@ -66,11 +66,24 @@ class RedfishInspectTestCase(db_base.DbTestCase): system_mock.processors.summary = '8', sushy.PROCESSOR_ARCH_x86 - mock_simple_storage_device = mock.Mock() + mock_storage_drive = mock.Mock( + spec=sushy.resources.system.storage.drive.Drive) + mock_storage_drive.name = 'storage-drive' + mock_storage_drive.capacity_bytes = '128' + + mock_storage = mock.Mock( + spec=sushy.resources.system.storage.storage.Storage) + mock_storage.drives = [mock_storage_drive] + system_mock.storage.get_members.return_value = [ + mock_storage] + + mock_simple_storage_device = mock.Mock( + spec=sushy.resources.system.simple_storage.DeviceListField) mock_simple_storage_device.name = 'test-name' mock_simple_storage_device.capacity_bytes = '123' - mock_simple_storage = mock.Mock() + mock_simple_storage = mock.Mock( + spec=sushy.resources.system.simple_storage.SimpleStorage) mock_simple_storage.devices = [mock_simple_storage_device] system_mock.simple_storage.get_members.return_value = [ mock_simple_storage] @@ -150,7 +163,7 @@ class RedfishInspectTestCase(db_base.DbTestCase): self.assertEqual(expected_cpu, inventory['inventory']['cpu']) - expected_disks = [{'name': 'test-name', 'size': '123'}] + expected_disks = [{'name': 'storage-drive', 'size': '128'}] self.assertEqual(expected_disks, inventory["inventory"]['disks']) @@ -276,11 +289,32 @@ class RedfishInspectTestCase(db_base.DbTestCase): task.driver.inspect.inspect_hardware(task) self.assertEqual(expected_properties, task.node.properties) + @mock.patch.object(redfish_utils, 'get_system', autospec=True) + def test_inspect_hardware_ignore_missing_simple_storage_and_storage( + self, mock_get_system): + system_mock = self.init_system_mock(mock_get_system.return_value) + system_mock.simple_storage = {} + system_mock.storage = {} + + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + expected_properties = { + 'capabilities': 'boot_mode:uefi', + 'cpu_arch': 'x86_64', 'cpus': '8', + 'local_gb': '0', 'memory_mb': 2048 + } + task.driver.inspect.inspect_hardware(task) + self.assertEqual(expected_properties, task.node.properties) + + inventory = inspect_utils.get_inspection_data(task.node, + self.context) + self.assertNotIn('disks', inventory['inventory']) + @mock.patch.object(redfish_utils, 'get_system', autospec=True) def test_inspect_hardware_ignore_missing_simple_storage(self, mock_get_system): system_mock = self.init_system_mock(mock_get_system.return_value) - system_mock.simple_storage = None + system_mock.simple_storage = {} with task_manager.acquire(self.context, self.node.uuid, shared=True) as task: @@ -294,7 +328,26 @@ class RedfishInspectTestCase(db_base.DbTestCase): inventory = inspect_utils.get_inspection_data(task.node, self.context) - self.assertNotIn('disks', inventory['inventory']) + self.assertIn('disks', inventory['inventory']) + + @mock.patch.object(redfish_utils, 'get_system', autospec=True) + def test_inspect_hardware_ignore_missing_storage(self, mock_get_system): + system_mock = self.init_system_mock(mock_get_system.return_value) + system_mock.storage = {} + + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + expected_properties = { + 'capabilities': 'boot_mode:uefi', + 'cpu_arch': 'x86_64', 'cpus': '8', + 'local_gb': '4', 'memory_mb': 2048 + } + task.driver.inspect.inspect_hardware(task) + self.assertEqual(expected_properties, task.node.properties) + + inventory = inspect_utils.get_inspection_data(task.node, + self.context) + self.assertIn('disks', inventory['inventory']) @mock.patch.object(redfish_utils, 'get_system', autospec=True) def test_inspect_hardware_fail_missing_memory_mb(self, mock_get_system): diff --git a/ironic/tests/unit/drivers/modules/redfish/test_management.py b/ironic/tests/unit/drivers/modules/redfish/test_management.py index 1d752d909a..ca098ac804 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_management.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_management.py @@ -57,7 +57,6 @@ class RedfishManagementTestCase(db_base.DbTestCase): self.system_uuid = 'ZZZ--XXX-YYY' self.chassis_uuid = 'XXX-YYY-ZZZ' - self.drive_uuid = 'ZZZ-YYY-XXX' def init_system_mock(self, system_mock, **properties): @@ -618,7 +617,7 @@ class RedfishManagementTestCase(db_base.DbTestCase): self.assertEqual(expected, sensors) - def test__get_sensors_data_drive(self): + def test__get_sensors_data_drive_simple_storage(self): attributes = { 'name': '32ADF365C6C1B7BD', 'manufacturer': 'IBM', @@ -630,14 +629,58 @@ class RedfishManagementTestCase(db_base.DbTestCase): } } - mock_system = mock.MagicMock(identity='ZZZ-YYY-XXX') + mock_system = mock.MagicMock(spec=sushy.resources.system.system.System) + mock_system.identity = 'ZZZ-YYY-XXX' mock_drive = mock.MagicMock(**attributes) mock_drive.name = attributes['name'] mock_drive.status = mock.MagicMock(**attributes['status']) - mock_storage = mock.MagicMock() - mock_storage.devices = [mock_drive] + mock_storage = mock.MagicMock( + spec=sushy.resources.system.storage.storage.Storage) + mock_storage.drives = [mock_drive] mock_storage.identity = 'XXX-YYY-ZZZ' - mock_system.simple_storage.get_members.return_value = [mock_storage] + mock_system.storage.get_members.return_value = [mock_storage] + mock_system.simple_storage = {} + + with task_manager.acquire(self.context, self.node.uuid, + shared=True) as task: + sensors = task.driver.management._get_sensors_drive(mock_system) + + expected = { + '32ADF365C6C1B7BD:XXX-YYY-ZZZ@ZZZ-YYY-XXX': { + 'capacity_bytes': 3750000000, + 'health': 'OK', + 'name': '32ADF365C6C1B7BD', + 'model': 'IBM 350A', + 'state': 'enabled' + } + } + + self.assertEqual(expected, sensors) + + def test__get_sensors_data_drive_storage(self): + attributes = { + 'name': '32ADF365C6C1B7BD', + 'manufacturer': 'IBM', + 'model': 'IBM 350A', + 'capacity_bytes': 3750000000, + 'status': { + 'health': 'OK', + 'state': 'enabled' + } + } + + mock_system = mock.MagicMock(spec=sushy.resources.system.system.System) + mock_system.identity = 'ZZZ-YYY-XXX' + mock_drive = mock.MagicMock(**attributes) + mock_drive.name = attributes['name'] + mock_drive.status = mock.MagicMock(**attributes['status']) + mock_simple_storage = mock.MagicMock( + spec=sushy.resources.system.simple_storage.SimpleStorage) + mock_simple_storage.devices = [mock_drive] + mock_simple_storage.identity = 'XXX-YYY-ZZZ' + mock_system.simple_storage.get_members.return_value = [ + mock_simple_storage] + mock_system.storage = {} with task_manager.acquire(self.context, self.node.uuid, shared=True) as task: @@ -705,13 +748,17 @@ class RedfishManagementTestCase(db_base.DbTestCase): uuid=self.chassis_uuid, indicator_led=sushy.INDICATOR_LED_LIT) fake_drive = mock.Mock( - uuid=self.drive_uuid, + identity='drive1', indicator_led=sushy.INDICATOR_LED_LIT) + fake_storage = mock.Mock( + identity='storage1', + drives=[fake_drive]) fake_system = mock.Mock( uuid=self.system_uuid, chassis=[fake_chassis], - simple_storage=mock.MagicMock(drives=[fake_drive]), + storage=mock.MagicMock(), indicator_led=sushy.INDICATOR_LED_LIT) + fake_system.storage.get_members.return_value = [fake_storage] mock_get_system.return_value = fake_system @@ -743,7 +790,7 @@ class RedfishManagementTestCase(db_base.DbTestCase): } }, components.DISK: { - 'ZZZ-YYY-XXX': { + 'storage1:drive1': { "readonly": False, "states": [ indicator_states.BLINKING, @@ -762,13 +809,17 @@ class RedfishManagementTestCase(db_base.DbTestCase): uuid=self.chassis_uuid, indicator_led=sushy.INDICATOR_LED_LIT) fake_drive = mock.Mock( - uuid=self.drive_uuid, + identity='drive1', indicator_led=sushy.INDICATOR_LED_LIT) + fake_storage = mock.Mock( + identity='storage1', + drives=[fake_drive]) fake_system = mock.Mock( uuid=self.system_uuid, chassis=[fake_chassis], - simple_storage=mock.MagicMock(drives=[fake_drive]), + storage=mock.MagicMock(), indicator_led=sushy.INDICATOR_LED_LIT) + fake_system.storage.get_members.return_value = [fake_storage] mock_get_system.return_value = fake_system @@ -782,19 +833,53 @@ class RedfishManagementTestCase(db_base.DbTestCase): mock_get_system.assert_called_once_with(task.node) + @mock.patch.object(redfish_utils, 'get_system', autospec=True) + def test_set_indicator_state_disk(self, mock_get_system): + fake_chassis = mock.Mock( + uuid=self.chassis_uuid, + indicator_led=sushy.INDICATOR_LED_LIT) + fake_drive = mock.Mock( + identity='drive1', + indicator_led=sushy.INDICATOR_LED_LIT) + fake_storage = mock.Mock( + identity='storage1', + drives=[fake_drive]) + fake_system = mock.Mock( + uuid=self.system_uuid, + chassis=[fake_chassis], + storage=mock.MagicMock(), + indicator_led=sushy.INDICATOR_LED_LIT) + fake_system.storage.get_members.return_value = [fake_storage] + + mock_get_system.return_value = fake_system + + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + task.driver.management.set_indicator_state( + task, components.DISK, 'storage1:drive1', indicator_states.ON) + + fake_drive.set_indicator_led.assert_called_once_with( + sushy.INDICATOR_LED_LIT) + + mock_get_system.assert_called_once_with(task.node) + @mock.patch.object(redfish_utils, 'get_system', autospec=True) def test_get_indicator_state(self, mock_get_system): fake_chassis = mock.Mock( uuid=self.chassis_uuid, indicator_led=sushy.INDICATOR_LED_LIT) fake_drive = mock.Mock( - uuid=self.drive_uuid, + identity='drive1', indicator_led=sushy.INDICATOR_LED_LIT) + fake_storage = mock.Mock( + identity='storage1', + drives=[fake_drive]) fake_system = mock.Mock( uuid=self.system_uuid, chassis=[fake_chassis], - simple_storage=mock.MagicMock(drives=[fake_drive]), + storage=mock.MagicMock(), indicator_led=sushy.INDICATOR_LED_LIT) + fake_system.storage.get_members.return_value = [fake_storage] mock_get_system.return_value = fake_system @@ -808,6 +893,36 @@ class RedfishManagementTestCase(db_base.DbTestCase): self.assertEqual(indicator_states.ON, state) + @mock.patch.object(redfish_utils, 'get_system', autospec=True) + def test_get_indicator_state_disk(self, mock_get_system): + fake_chassis = mock.Mock( + uuid=self.chassis_uuid, + indicator_led=sushy.INDICATOR_LED_LIT) + fake_drive = mock.Mock( + identity='drive1', + indicator_led=sushy.INDICATOR_LED_LIT) + fake_storage = mock.Mock( + identity='storage1', + drives=[fake_drive]) + fake_system = mock.Mock( + uuid=self.system_uuid, + chassis=[fake_chassis], + storage=mock.MagicMock(), + indicator_led=sushy.INDICATOR_LED_LIT) + fake_system.storage.get_members.return_value = [fake_storage] + + mock_get_system.return_value = fake_system + + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + + state = task.driver.management.get_indicator_state( + task, components.DISK, 'storage1:drive1') + + mock_get_system.assert_called_once_with(task.node) + + self.assertEqual(indicator_states.ON, state) + @mock.patch.object(redfish_utils, 'get_system', autospec=True) def test_detect_vendor(self, mock_get_system): mock_get_system.return_value.manufacturer = "Fake GmbH" diff --git a/releasenotes/notes/redfish-use-storage-over-simplestorage-ccb3e0e38bfe8712.yaml b/releasenotes/notes/redfish-use-storage-over-simplestorage-ccb3e0e38bfe8712.yaml new file mode 100644 index 0000000000..b23f3789d0 --- /dev/null +++ b/releasenotes/notes/redfish-use-storage-over-simplestorage-ccb3e0e38bfe8712.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes issue of changing or getting state of indicator LED of attached disk + caused by misunderstanding SimpleStorage provides this functionality but + actually Storage resource does.