Merge "Transiton to Storage resource from SimpleStorage"

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

@@ -516,8 +516,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(
@@ -685,11 +688,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
                 }
 
@@ -731,10 +737,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
@@ -783,10 +790,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

@@ -59,7 +59,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):
 
@@ -670,7 +669,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',
@@ -682,14 +681,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:
@@ -757,13 +800,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
 
@@ -795,7 +842,7 @@ class RedfishManagementTestCase(db_base.DbTestCase):
                     }
                 },
                 components.DISK: {
-                    'ZZZ-YYY-XXX': {
+                    'storage1:drive1': {
                         "readonly": False,
                         "states": [
                             indicator_states.BLINKING,
@@ -814,13 +861,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
 
@@ -834,19 +885,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
 
@@ -860,6 +945,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.