From a9a4fff71c15e6192e06652d64a1048bd5c2633d Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 3 Apr 2024 16:16:31 +0200 Subject: [PATCH] Fix generating local paths when connecting virtual media The generate path does not contain the node UUID, causing conflicts. Also make sure to always clean up any existing files first. Change-Id: I30f948d64e7b87f33841dc22828db60338a62dd8 --- ironic/conductor/manager.py | 19 ++++++++++++------- ironic/drivers/modules/image_utils.py | 2 ++ ironic/tests/unit/conductor/test_manager.py | 19 +++++++++++++++---- .../notes/vmedia-path-648cfa258708e0bb.yaml | 6 ++++++ 4 files changed, 35 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/vmedia-path-648cfa258708e0bb.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 57c97a30a5..506454173c 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -4081,6 +4081,14 @@ def do_sync_power_state(task, count): return count +def _virtual_media_file_name(task, device_type): + return "%s-%s.%s" % ( + device_type.lower(), + task.node.uuid, + 'iso' if device_type == boot_devices.CDROM else 'img' + ) + + def do_attach_virtual_media(task, device_type, image_url, image_download_source): success_msg = "Device %(device_type)s attached to node %(node)s", @@ -4088,10 +4096,8 @@ def do_attach_virtual_media(task, device_type, image_url, "to node %(node)s: %(exc)s") try: assert device_type in boot_devices.VMEDIA_DEVICES - file_name = "%s.%s" % ( - device_type.lower(), - 'iso' if device_type == boot_devices.CDROM else 'img' - ) + file_name = _virtual_media_file_name(task, device_type) + image_utils.cleanup_remote_image(task, file_name) image_url = image_utils.prepare_remote_image( task, image_url, file_name=file_name, download_source=image_download_source) @@ -4119,6 +4125,5 @@ def do_detach_virtual_media(task, device_types): "%(device_types)s from node %(node)s: %(exc)s", device_types=device_types) for device_type in device_types: - suffix = '.iso' if device_type == boot_devices.CDROM else '.img' - image_utils.ImageHandler.unpublish_image_for_node( - task.node, prefix=device_type.lower(), suffix=suffix) + file_name = _virtual_media_file_name(task, device_type) + image_utils.cleanup_remote_image(task, file_name) diff --git a/ironic/drivers/modules/image_utils.py b/ironic/drivers/modules/image_utils.py index 5ce4303514..021cad8508 100644 --- a/ironic/drivers/modules/image_utils.py +++ b/ironic/drivers/modules/image_utils.py @@ -326,6 +326,8 @@ def cleanup_disk_image(task, prefix=None): ImageHandler.unpublish_image_for_node(task.node, prefix=prefix) +# FIXME(dtantsur): file_name is not node-specific, we should probably replace +# it with a prefix/suffix pair and pass to _get_name def prepare_remote_image(task, image_url, file_name='boot.iso', download_source='local', cache=None): """Generic function for publishing remote images. diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index db2dbc9d30..2917ef8e0d 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -8743,30 +8743,38 @@ class VirtualMediaTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): @mock.patch.object(redfish.management.RedfishManagement, 'attach_virtual_media', autospec=True) + @mock.patch.object(image_utils, 'cleanup_remote_image', autospec=True) @mock.patch.object(image_utils, 'prepare_remote_image', autospec=True) - def test_do_attach_virtual_media(self, mock_prepare_image, mock_attach): + def test_do_attach_virtual_media(self, mock_prepare_image, + mock_cleanup_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") + file_name = f"cdrom-{self.node.uuid}.iso" mock_prepare_image.assert_called_once_with( - task, "https://url", file_name="cdrom.iso", + task, "https://url", file_name=file_name, download_source="local") + mock_cleanup_image.assert_called_once_with(task, file_name) 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, 'cleanup_remote_image', 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_cleanup_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") + file_name = f"cdrom-{self.node.uuid}.iso" mock_prepare_image.assert_called_once_with( - task, "https://url", file_name="cdrom.iso", + task, "https://url", file_name=file_name, download_source="local") + mock_cleanup_image.assert_called_once_with(task, file_name) mock_attach.assert_not_called() self.node.refresh() self.assertIn("Could not attach device cdrom", self.node.last_error) @@ -8774,15 +8782,18 @@ class VirtualMediaTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): @mock.patch.object(redfish.management.RedfishManagement, 'attach_virtual_media', autospec=True) + @mock.patch.object(image_utils, 'cleanup_remote_image', 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_cleanup_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") + file_name = f"cdrom-{self.node.uuid}.iso" mock_prepare_image.assert_called_once_with( - task, "https://url", file_name="cdrom.iso", + task, "https://url", file_name=file_name, download_source="local") mock_attach.assert_called_once_with( task.driver.management, task, device_type=boot_devices.CDROM, diff --git a/releasenotes/notes/vmedia-path-648cfa258708e0bb.yaml b/releasenotes/notes/vmedia-path-648cfa258708e0bb.yaml new file mode 100644 index 0000000000..f4a6124f1e --- /dev/null +++ b/releasenotes/notes/vmedia-path-648cfa258708e0bb.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes generated URL when using the virtual media attachment API. + Previously, it missed the node UUID, causing conflicts between different + nodes.