Fix error handling in the virtual media attach API

Currently, if the image download fails, there are no traces of the
error. This change adds logging and populates last_error.

Change-Id: I73ea2f94fb910daf21a5d4f52d6839aac3bad579

ironic/conductor/manager.py

@@ -4080,21 +4080,32 @@ def do_sync_power_state(task, count):
 
 def do_attach_virtual_media(task, device_type, image_url,
                             image_download_source):
-    assert device_type in boot_devices.VMEDIA_DEVICES
+    success_msg = "Device %(device_type)s attached to node %(node)s",
-    file_name = "%s.%s" % (
+    error_msg = ("Could not attach device %(device_type)s "
-        device_type.lower(),
+                 "to node %(node)s: %(exc)s")
-        'iso' if device_type == boot_devices.CDROM else 'img'
+    try:
-    )
+        assert device_type in boot_devices.VMEDIA_DEVICES
-    image_url = image_utils.prepare_remote_image(
+        file_name = "%s.%s" % (
-        task, image_url, file_name=file_name,
+            device_type.lower(),
-        download_source=image_download_source)
+            'iso' if device_type == boot_devices.CDROM else 'img'
-    utils.run_node_action(
+        )
-        task, task.driver.management.attach_virtual_media,
+        image_url = image_utils.prepare_remote_image(
-        success_msg="Device %(device_type)s attached to node %(node)s",
+            task, image_url, file_name=file_name,
-        error_msg="Could not attach device %(device_type)s "
+            download_source=image_download_source)
-        "to node %(node)s: %(exc)s",
+        utils.run_node_action(
-        device_type=device_type,
+            task, task.driver.management.attach_virtual_media,
-        image_url=image_url)
+            success_msg=success_msg,
+            error_msg=error_msg,
+            device_type=device_type,
+            image_url=image_url)
+    except Exception as exc:
+        error = error_msg % {'node': task.node.uuid,
+                             'device_type': device_type,
+                             'exc': exc}
+        LOG.error(
+            error, exc_info=not isinstance(exc, exception.IronicException))
+        utils.node_history_record(task.node, event=error, error=True)
+        task.node.save()
 
 
 def do_detach_virtual_media(task, device_types):

ironic/tests/unit/conductor/test_manager.py

@@ -8740,3 +8740,53 @@ class VirtualMediaTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
             image_url='https://url', image_download_source='http')
         self.node.refresh()
         self.assertIsNone(self.node.last_error)
+
+    @mock.patch.object(redfish.management.RedfishManagement,
+                       'attach_virtual_media', autospec=True)
+    @mock.patch.object(image_utils, 'prepare_remote_image', autospec=True)
+    def test_do_attach_virtual_media(self, mock_prepare_image, mock_attach):
+        with task_manager.acquire(self.context, self.node.id) as task:
+            manager.do_attach_virtual_media(task, boot_devices.CDROM,
+                                            "https://url", "local")
+            mock_prepare_image.assert_called_once_with(
+                task, "https://url", file_name="cdrom.iso",
+                download_source="local")
+            mock_attach.assert_called_once_with(
+                task.driver.management, task, device_type=boot_devices.CDROM,
+                image_url=mock_prepare_image.return_value)
+
+    @mock.patch.object(redfish.management.RedfishManagement,
+                       'attach_virtual_media', autospec=True)
+    @mock.patch.object(image_utils, 'prepare_remote_image', autospec=True)
+    def test_do_attach_virtual_media_fails_on_prepare(self, mock_prepare_image,
+                                                      mock_attach):
+        mock_prepare_image.side_effect = exception.InvalidImageRef
+        with task_manager.acquire(self.context, self.node.id) as task:
+            manager.do_attach_virtual_media(task, boot_devices.CDROM,
+                                            "https://url", "local")
+            mock_prepare_image.assert_called_once_with(
+                task, "https://url", file_name="cdrom.iso",
+                download_source="local")
+            mock_attach.assert_not_called()
+        self.node.refresh()
+        self.assertIn("Could not attach device cdrom", self.node.last_error)
+        self.assertIn("Invalid image href", self.node.last_error)
+
+    @mock.patch.object(redfish.management.RedfishManagement,
+                       'attach_virtual_media', autospec=True)
+    @mock.patch.object(image_utils, 'prepare_remote_image', autospec=True)
+    def test_do_attach_virtual_media_fails_on_attach(self, mock_prepare_image,
+                                                     mock_attach):
+        mock_attach.side_effect = exception.UnsupportedDriverExtension
+        with task_manager.acquire(self.context, self.node.id) as task:
+            manager.do_attach_virtual_media(task, boot_devices.CDROM,
+                                            "https://url", "local")
+            mock_prepare_image.assert_called_once_with(
+                task, "https://url", file_name="cdrom.iso",
+                download_source="local")
+            mock_attach.assert_called_once_with(
+                task.driver.management, task, device_type=boot_devices.CDROM,
+                image_url=mock_prepare_image.return_value)
+        self.node.refresh()
+        self.assertIn("Could not attach device cdrom", self.node.last_error)
+        self.assertIn("disabled or not implemented", self.node.last_error)

releasenotes/notes/vmedia-error-ef4eac3d08761d5c.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes error handling in the virtual media attachment API when the image
+    downloading fails. Now the ``last_error`` field is populated correctly
+    and the error is logged.