From a74f7f0cd00619a775483f66f134ca8952eca7d3 Mon Sep 17 00:00:00 2001
From: Vishakha Agarwal <agarwalvishakha18@gmail.com>
Date: Thu, 10 May 2018 11:14:40 +0530
Subject: [PATCH] Cloning image fails results duplicate cache entry

When creating a volume with image whose cache
entry is already present,results in duplicate
cache entry when create_clone_image() fails.

Closes-Bug: #1552734
Change-Id: I0708c333fa94bb44e66f64b2b7cc3bc0dbe8a409
---
 cinder/tests/unit/volume/test_image.py | 28 ++++++++++++++++++++++++--
 cinder/volume/manager.py               | 11 +++++++++-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/cinder/tests/unit/volume/test_image.py b/cinder/tests/unit/volume/test_image.py
index 539fec0fbf6..47938572296 100644
--- a/cinder/tests/unit/volume/test_image.py
+++ b/cinder/tests/unit/volume/test_image.py
@@ -37,7 +37,6 @@ from cinder.tests.unit import volume as base
 import cinder.volume
 from cinder.volume import manager as vol_manager
 
-
 QUOTAS = quota.QUOTAS
 NON_EXISTENT_IMAGE_ID = '003f540f-ec6b-4293-a3f9-7c68646b0f5c'
 
@@ -61,10 +60,10 @@ class CopyVolumeToImageTestCase(base.BaseVolumeTestCase):
         super(CopyVolumeToImageTestCase, self).setUp()
         self.dst_fd, self.dst_path = tempfile.mkstemp()
         self.addCleanup(os.unlink, self.dst_path)
-
         os.close(self.dst_fd)
         self.mock_object(self.volume.driver, 'local_path',
                          self.fake_local_path)
+        self.mock_cache = mock.MagicMock()
         self.image_id = '70a599e0-31e7-49b7-b260-868f441e862b'
         self.image_meta = {
             'id': self.image_id,
@@ -330,6 +329,31 @@ class CopyVolumeToImageTestCase(base.BaseVolumeTestCase):
         self.assertNotIn('locations', image)
         self.assertTrue(mock_delete.called)
 
+    @mock.patch('cinder.volume.manager.'
+                'VolumeManager._clone_image_volume')
+    @mock.patch('cinder.volume.manager.'
+                'VolumeManager._create_image_cache_volume_entry')
+    def test_create_image_cache_volume_entry(self,
+                                             mock_cache_entry,
+                                             mock_clone_image_volume):
+        image_id = self.image_id
+        image_meta = self.image_meta
+
+        self.mock_cache.get_entry.return_value = mock_cache_entry
+
+        if mock_cache_entry:
+            # Entry is in cache, so basically don't do anything.
+            # Make sure we didn't try and create a cache entry
+            self.assertFalse(self.mock_cache.ensure_space.called)
+            self.assertFalse(self.mock_cache.create_cache_entry.called)
+        else:
+            result = self.volume._create_image_cache_volume_entry(
+                self.context, mock_clone_image_volume, image_id, image_meta)
+            self.assertNotEqual(False, result)
+            cache_entry = self.image_volume_cache.get_entry(
+                self.context, mock_clone_image_volume, image_id, image_meta)
+            self.assertIsNotNone(cache_entry)
+
 
 class ImageVolumeCacheTestCase(base.BaseVolumeTestCase):
 
diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 91ab0d2c58d..53e6f09f0b2 100644
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -1370,6 +1370,16 @@ class VolumeManager(manager.CleanableManager,
         This assumes that the image has already been downloaded and stored
         in the volume described by the volume_ref.
         """
+        cache_entry = self.image_volume_cache.get_entry(ctx,
+                                                        volume_ref,
+                                                        image_id,
+                                                        image_meta)
+        if cache_entry:
+            LOG.debug('Cache entry already exists with image ID %'
+                      '(image_id)s',
+                      {'image_id': image_id})
+            return
+
         image_volume = None
         try:
             if not self.image_volume_cache.ensure_space(ctx, volume_ref):
@@ -1388,7 +1398,6 @@ class VolumeManager(manager.CleanableManager,
                             '%(image_id)s will not create cache entry.',
                             {'image_id': image_id})
                 return
-
             self.image_volume_cache.create_cache_entry(
                 ctx,
                 image_volume,