From ffde150aa7069b1e70540a2ab6f49a62a0853454 Mon Sep 17 00:00:00 2001 From: Vanou Ishii Date: Mon, 21 Aug 2023 21:16:54 -0400 Subject: [PATCH] Transiton to Storage resource from SimpleStorage Current Redfish driver uses SimpleStorage resource to collect disk information. Storage resource is defined to provide more sophisticated functionality over SimpsleStorage resource. Some Redfish implementations may take transition from SimpleStorage to Storage. So Redfish driver's logic around disk should be provided through Storage resource with SimpleStorage compatibility. This commit does 2 things * Use Storage resource instead of SimpleStorage, if possible * Fix wrong disk indicator LED logic (SimpleStorage doesn't support indicator LED operation) Related-bug: 2032590 Change-Id: I28abd75a41059c23028717db6a9766a5164313c7 --- ironic/drivers/modules/redfish/inspect.py | 14 +- ironic/drivers/modules/redfish/management.py | 58 +++---- .../drivers/modules/redfish/test_inspect.py | 63 +++++++- .../modules/redfish/test_management.py | 141 ++++++++++++++++-- ...e-over-simplestorage-ccb3e0e38bfe8712.yaml | 6 + 5 files changed, 232 insertions(+), 50 deletions(-) create mode 100644 releasenotes/notes/redfish-use-storage-over-simplestorage-ccb3e0e38bfe8712.yaml 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.