From 46a5899543bce73e1d40f6f363031f9ac1a472e8 Mon Sep 17 00:00:00 2001
From: Iury Gregory Melo Ferreira <iurygregory@gmail.com>
Date: Thu, 8 Nov 2018 15:26:13 +0100
Subject: [PATCH] Improve logs when hard linking images fails

When hard linking images fail in ironic the logs doesn't have
enough related path information to resolve the underlying cause
of the exception.

Change-Id: I3331c4eca2deb9383f448f1efa93cbef53019f9c
Story: #2004303
Task: #27863
---
 ironic/drivers/modules/image_cache.py                | 11 +++++++++++
 .../tests/unit/drivers/modules/test_image_cache.py   | 12 ++++++++++++
 .../invalid_cross_device_link-7ecf3543a8ada09f.yaml  |  5 +++++
 3 files changed, 28 insertions(+)
 create mode 100644 releasenotes/notes/invalid_cross_device_link-7ecf3543a8ada09f.yaml

diff --git a/ironic/drivers/modules/image_cache.py b/ironic/drivers/modules/image_cache.py
index f4226c395d..358a21581e 100644
--- a/ironic/drivers/modules/image_cache.py
+++ b/ironic/drivers/modules/image_cache.py
@@ -30,6 +30,7 @@ import six
 
 from ironic.common import exception
 from ironic.common.glance_service import service_utils
+from ironic.common.i18n import _
 from ironic.common import image_service
 from ironic.common import images
 from ironic.common import utils
@@ -153,6 +154,9 @@ class ImageCache(object):
         :param ctx: context
         :param force_raw: boolean value, whether to convert the image to raw
                           format
+        :raise ImageDownloadFailed: when the image cache and the image HTTP or
+                                    TFTP location are on different file system,
+                                    causing hard link to fail.
         """
         # TODO(ghe): timeout and retry for downloads
         # TODO(ghe): logging when image cannot be created
@@ -165,6 +169,13 @@ class ImageCache(object):
             # will have link count >1 at any moment, so won't be cleaned up
             os.link(tmp_path, master_path)
             os.link(master_path, dest_path)
+        except OSError as exc:
+            msg = (_("Could not link image %(img_href)s from %(src_path)s "
+                     "to %(dst_path)s, error: %(exc)s") %
+                   {'img_href': href, 'src_path': master_path,
+                    'dst_path': dest_path, 'exc': exc})
+            LOG.error(msg)
+            raise exception.ImageDownloadFailed(msg)
         finally:
             utils.rmtree_without_raise(tmp_dir)
 
diff --git a/ironic/tests/unit/drivers/modules/test_image_cache.py b/ironic/tests/unit/drivers/modules/test_image_cache.py
index 68a1d95e01..37ae008ecf 100644
--- a/ironic/tests/unit/drivers/modules/test_image_cache.py
+++ b/ironic/tests/unit/drivers/modules/test_image_cache.py
@@ -211,6 +211,18 @@ class TestImageCacheFetch(base.TestCase):
         with open(self.dest_path) as fp:
             self.assertEqual("TEST", fp.read())
 
+    @mock.patch.object(image_cache, '_fetch', autospec=True)
+    @mock.patch.object(image_cache, 'LOG', autospec=True)
+    @mock.patch.object(os, 'link', autospec=True)
+    def test__download_image_linkfail(self, mock_link, mock_log, mock_fetch):
+        mock_link.side_effect = [None, OSError]
+        self.assertRaises(exception.ImageDownloadFailed,
+                          self.cache._download_image,
+                          self.uuid, self.master_path, self.dest_path)
+        self.assertTrue(mock_fetch.called)
+        self.assertEqual(2, mock_link.call_count)
+        self.assertTrue(mock_log.error.called)
+
 
 @mock.patch.object(os, 'unlink', autospec=True)
 class TestUpdateImages(base.TestCase):
diff --git a/releasenotes/notes/invalid_cross_device_link-7ecf3543a8ada09f.yaml b/releasenotes/notes/invalid_cross_device_link-7ecf3543a8ada09f.yaml
new file mode 100644
index 0000000000..d9e4cebbe8
--- /dev/null
+++ b/releasenotes/notes/invalid_cross_device_link-7ecf3543a8ada09f.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Properly reports an error when the image cache and the image HTTP or TFTP
+    location are on different file system, causing hard link to fail.