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

@@ -134,15 +134,15 @@ class RedfishInspect(base.InspectInterface):
                         {'node': task.node.uuid})
             inspected_properties['local_gb'] = '0'
 
-        if system.simple_storage:
+        if storages := system.storage or system.simple_storage:
-            simple_storage_list = system.simple_storage.get_members()
             disks = list()
-
+            for storage in storages.get_members():
-            for simple_storage in simple_storage_list:
+                drives = storage.drives if hasattr(
-                for simple_storage_device in simple_storage.devices:
+                    storage, 'drives') else storage.devices
+                for drive in drives:
                     disk = {}
-                    disk['name'] = simple_storage_device.name
+                    disk['name'] = drive.name
-                    disk['size'] = simple_storage_device.capacity_bytes
+                    disk['size'] = drive.capacity_bytes
                     disks.append(disk)
 
             inventory['disks'] = disks

ironic/drivers/modules/redfish/management.py

@@ -441,8 +441,11 @@ class RedfishManagement(base.ManagementInterface):
         """
         sensors = {}
 
-        for storage in system.simple_storage.get_members():
+        if storages := system.storage or system.simple_storage:
-            for drive in storage.devices:
+            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(
@@ -610,11 +613,14 @@ class RedfishManagement(base.ManagementInterface):
 
         try:
             if (component in (None, components.DISK)
-                    and system.simple_storage
+                    and system.storage):
-                    and system.simple_storage.drives):
                 indicators[components.DISK] = {
-                    drive.uuid: properties
+                    # NOTE(vanou) There is no uuid property in Drive resource.
-                    for drive in system.simple_storage.drives
+                    # 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,10 +662,11 @@ class RedfishManagement(base.ManagementInterface):
                         return
 
             elif (component == components.DISK
-                  and system.simple_storage
+                  and system.storage and len(indicator.split(':')) == 2):
-                  and system.simple_storage.drives):
+                for storage in system.storage.get_members():
-                for drive in system.simple_storage.drives:
+                    if storage.identity == indicator.split(':')[0]:
-                    if drive.uuid == indicator:
+                        for drive in storage.drives:
+                            if drive.identity == indicator.split(':')[1]:
                                 drive.set_indicator_led(
                                     INDICATOR_MAP_REV[state])
                                 return
@@ -708,10 +715,11 @@ class RedfishManagement(base.ManagementInterface):
                         return INDICATOR_MAP[chassis.indicator_led]
 
             if (component == components.DISK
-                    and system.simple_storage
+                    and system.storage and len(indicator.split(':')) == 2):
-                    and system.simple_storage.drives):
+                for storage in system.storage.get_members():
-                for drive in system.simple_storage.drives:
+                    if storage.identity == indicator.split(':')[0]:
-                    if drive.uuid == indicator:
+                        for drive in storage.drives:
+                            if drive.identity == indicator.split(':')[1]:
                                 return INDICATOR_MAP[drive.indicator_led]
 
         except sushy.exceptions.SushyError as e:

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):

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 = mock.MagicMock(
-        mock_storage.devices = [mock_drive]
+            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"

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.