Ensure that image link points to the correct image

If an image is cached for a node, and the image changes, Ironic won't
notice because we only check if the link exists. In this change it's
proposed to check in the image_cache if the image is up to date.

Closes-bug: #1405448
Co-Authored-By: Vladyslav Drok <vdrok@mirantis.com>
(cherry picked from commit 2f0c8cd8d8d8b2846b641b518b742ca051425028)

Conflicts:
	ironic/drivers/modules/image_cache.py
	ironic/tests/drivers/test_image_cache.py

Change-Id: If67c7e60eb6d048fa3aa03469d7904c6c233ff89

2 files changed, 48 insertions, 8 deletions

diff --git a/ironic/drivers/modules/image_cache.py b/ironic/drivers/modules/image_cache.py
index 2765c18c0..e50c602d1 100644
--- a/ironic/drivers/modules/image_cache.py
+++ b/ironic/drivers/modules/image_cache.py
@@ -104,11 +104,18 @@ class ImageCache(object):
         # TODO(dtantsur): lock expiration time
         with lockutils.lock(img_download_lock_name, 'ironic-'):
             if os.path.exists(dest_path):
-                LOG.debug("Destination %(dest)s already exists for "
-                            "image %(uuid)s" %
-                          {'uuid': uuid,
-                           'dest': dest_path})
-                return
+                # NOTE(vdrok): After rebuild requested image can change, so we
+                # should ensure that dest_path and master_path (if exists) are
+                # pointing to the same file
+                if (os.path.exists(master_path) and
+                        (os.stat(dest_path).st_ino ==
+                         os.stat(master_path).st_ino)):
+                    LOG.debug("Destination %(dest)s already exists for "
+                              "image %(uuid)s" %
+                              {'uuid': uuid,
+                               'dest': dest_path})
+                    return
+                os.unlink(dest_path)
 
             try:
                 # NOTE(dtantsur): ensure we're not in the middle of clean up
diff --git a/ironic/tests/drivers/test_image_cache.py b/ironic/tests/drivers/test_image_cache.py
index 0849b8285..0991b48fc 100644
--- a/ironic/tests/drivers/test_image_cache.py
+++ b/ironic/tests/drivers/test_image_cache.py
@@ -56,16 +56,49 @@ class TestImageCacheFetch(base.TestCase):
             None, 'uuid', self.dest_path, None)
         self.assertFalse(mock_clean_up.called)
 
+    @mock.patch.object(os, 'unlink')
     @mock.patch.object(image_cache.ImageCache, 'clean_up')
     @mock.patch.object(image_cache.ImageCache, '_download_image')
-    def test_fetch_image_dest_exists(self, mock_download, mock_clean_up,
-                                     mock_fetch_to_raw):
+    def test_fetch_image_dest_and_master_exist_uptodate(self, mock_download,
+            mock_clean_up, mock_unlink, mock_fetch):
+        touch(self.master_path)
+        os.link(self.master_path, self.dest_path)
+        self.cache.fetch_image(self.uuid, self.dest_path)
+        self.assertFalse(mock_unlink.called)
+        self.assertFalse(mock_download.called)
+        self.assertFalse(mock_fetch.called)
+        self.assertFalse(mock_clean_up.called)
+
+    @mock.patch.object(image_cache.ImageCache, 'clean_up')
+    @mock.patch.object(image_cache.ImageCache, '_download_image')
+    def test_fetch_image_dest_and_master_exist_outdated(self, mock_download,
+            mock_clean_up, mock_fetch):
+        touch(self.master_path)
         touch(self.dest_path)
+        self.assertNotEqual(os.stat(self.dest_path).st_ino,
+                            os.stat(self.master_path).st_ino)
         self.cache.fetch_image(self.uuid, self.dest_path)
         self.assertFalse(mock_download.called)
-        self.assertFalse(mock_fetch_to_raw.called)
+        self.assertFalse(mock_fetch.called)
+        self.assertTrue(os.path.isfile(self.dest_path))
+        self.assertEqual(os.stat(self.dest_path).st_ino,
+                         os.stat(self.master_path).st_ino)
         self.assertFalse(mock_clean_up.called)
 
+    @mock.patch.object(os, 'unlink')
+    @mock.patch.object(image_cache.ImageCache, 'clean_up')
+    @mock.patch.object(image_cache.ImageCache, '_download_image')
+    def test_fetch_image_only_dest_exists(self, mock_download,
+            mock_clean_up, mock_unlink, mock_fetch):
+        touch(self.dest_path)
+        self.cache.fetch_image(self.uuid, self.dest_path)
+        mock_unlink.assert_called_once_with(self.dest_path)
+        self.assertFalse(mock_fetch.called)
+        mock_download.assert_called_once_with(
+            self.uuid, self.master_path, self.dest_path,
+            ctx=None)
+        self.assertTrue(mock_clean_up.called)
+
     @mock.patch.object(image_cache.ImageCache, 'clean_up')
     @mock.patch.object(image_cache.ImageCache, '_download_image')
     def test_fetch_image_master_exists(self, mock_download, mock_clean_up,