Merge "Fixes rbd _delete_image snapshot with missing image"

glance/store/rbd.py

@@ -38,7 +38,8 @@ try:
     import rados
     import rbd
 except ImportError:
-    pass
+    rados = None
+    rbd = None
 
 DEFAULT_POOL = 'rbd'
 DEFAULT_CONFFILE = ''  # librados will locate the default conf file
@@ -249,9 +250,9 @@ class Store(glance.store.base.Store):
             librbd.create(ioctx, image_name, size, order, old_format=True)
             return StoreLocation({'image': image_name})
 
-    def _delete_image(self, image_name, snapshot_name):
+    def _delete_image(self, image_name, snapshot_name=None):
         """
-        Find the image file to delete.
+        Delete RBD image and snapshot.
 
         :param image_name Image's name
         :param snapshot_name Image snapshot's name
@@ -261,17 +262,23 @@ class Store(glance.store.base.Store):
         """
         with rados.Rados(conffile=self.conf_file, rados_id=self.user) as conn:
             with conn.open_ioctx(self.pool) as ioctx:
-                if snapshot_name:
-                    with rbd.Image(ioctx, image_name) as image:
-                        try:
-                            image.unprotect_snap(snapshot_name)
-                        except rbd.ImageBusy:
-                            log_msg = _("snapshot %s@%s could not be "
-                                        "unprotected because it is in use")
-                            LOG.debug(log_msg % (image_name, snapshot_name))
-                            raise exception.InUseByStore()
-                        image.remove_snap(snapshot_name)
                 try:
+                    # First remove snapshot.
+                    if snapshot_name is not None:
+                        with rbd.Image(ioctx, image_name) as image:
+                            try:
+                                image.unprotect_snap(snapshot_name)
+                            except rbd.ImageBusy:
+                                log_msg = _("snapshot %(image)s@%(snap)s "
+                                            "could not be unprotected because "
+                                            "it is in use")
+                                LOG.debug(log_msg %
+                                          {'image': image_name,
+                                           'snap': snapshot_name})
+                                raise exception.InUseByStore()
+                            image.remove_snap(snapshot_name)
+
+                    # Then delete image.
                     rbd.RBD().remove(ioctx, image_name)
                 except rbd.ImageNotFound:
                     raise exception.NotFound(
@@ -340,11 +347,14 @@ class Store(glance.store.base.Store):
                         if loc.snapshot:
                             image.create_snap(loc.snapshot)
                             image.protect_snap(loc.snapshot)
-                except:
+                except Exception as exc:
-                    # Note(zhiyan): clean up already received data when
+                    # Delete image if one was created
-                    # error occurs such as ImageSizeLimitExceeded exception.
+                    try:
-                    with excutils.save_and_reraise_exception():
                         self._delete_image(loc.image, loc.snapshot)
+                    except exception.NotFound:
+                        pass
+
+                    raise exc
 
         return (loc.get_uri(), image_size, checksum.hexdigest(), {})
 

glance/tests/unit/fake_rados.py

@@ -0,0 +1,132 @@
+# Copyright 2013 Canonical Ltd.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+
+class mock_rados(object):
+
+    class ioctx(object):
+        def __init__(self, *args, **kwargs):
+            pass
+
+        def __enter__(self, *args, **kwargs):
+            return self
+
+        def __exit__(self, *args, **kwargs):
+            return False
+
+        def close(self, *args, **kwargs):
+            pass
+
+    class Rados(object):
+
+        def __init__(self, *args, **kwargs):
+            pass
+
+        def __enter__(self, *args, **kwargs):
+            return self
+
+        def __exit__(self, *args, **kwargs):
+            return False
+
+        def connect(self, *args, **kwargs):
+            pass
+
+        def open_ioctx(self, *args, **kwargs):
+            return mock_rados.ioctx()
+
+        def shutdown(self, *args, **kwargs):
+            pass
+
+
+class mock_rbd(object):
+
+    class ImageExists(Exception):
+        pass
+
+    class ImageBusy(Exception):
+        pass
+
+    class ImageNotFound(Exception):
+        pass
+
+    class Image(object):
+
+        def __init__(self, *args, **kwargs):
+            pass
+
+        def __enter__(self, *args, **kwargs):
+            return self
+
+        def __exit__(self, *args, **kwargs):
+            pass
+
+        def create_snap(self, *args, **kwargs):
+            pass
+
+        def remove_snap(self, *args, **kwargs):
+            pass
+
+        def protect_snap(self, *args, **kwargs):
+            pass
+
+        def unprotect_snap(self, *args, **kwargs):
+            pass
+
+        def read(self, *args, **kwargs):
+            raise NotImplementedError()
+
+        def write(self, *args, **kwargs):
+            raise NotImplementedError()
+
+        def resize(self, *args, **kwargs):
+            raise NotImplementedError()
+
+        def discard(self, offset, length):
+            raise NotImplementedError()
+
+        def close(self):
+            pass
+
+        def list_snaps(self):
+            raise NotImplementedError()
+
+        def parent_info(self):
+            raise NotImplementedError()
+
+        def size(self):
+            raise NotImplementedError()
+
+    class RBD(object):
+
+        def __init__(self, *args, **kwargs):
+            pass
+
+        def __enter__(self, *args, **kwargs):
+            return self
+
+        def __exit__(self, *args, **kwargs):
+            return False
+
+        def create(self, *args, **kwargs):
+            pass
+
+        def remove(self, *args, **kwargs):
+            pass
+
+        def list(self, *args, **kwargs):
+            raise NotImplementedError()
+
+        def clone(self, *args, **kwargs):
+            raise NotImplementedError()

glance/tests/unit/test_rbd_store.py

@@ -13,80 +13,136 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import contextlib
 import StringIO
-
-import stubout
-
 from glance.common import exception
 from glance.common import utils
-from glance.store.rbd import Store
+import glance.store.rbd as rbd_store
+from glance.store.location import Location
 from glance.store.rbd import StoreLocation
 from glance.tests.unit import base
-try:
+from glance.tests.unit.fake_rados import mock_rados
-    import rados
+from glance.tests.unit.fake_rados import mock_rbd
-    import rbd
-except ImportError:
-    rbd = None
-
-
-RBD_CONF = {'verbose': True,
-            'debug': True,
-            'default_store': 'rbd'}
-FAKE_CHUNKSIZE = 1
 
 
 class TestStore(base.StoreClearingUnitTest):
     def setUp(self):
         """Establish a clean test environment"""
-        self.config(**RBD_CONF)
         super(TestStore, self).setUp()
-        self.stubs = stubout.StubOutForTesting()
+        self.stubs.Set(rbd_store, 'rados', mock_rados)
-        self.store = Store()
+        self.stubs.Set(rbd_store, 'rbd', mock_rbd)
-        self.store.chunk_size = FAKE_CHUNKSIZE
+        self.store = rbd_store.Store()
-        self.addCleanup(self.stubs.UnsetAll)
+        self.store.chunk_size = 2
-
+        self.called_commands_actual = []
-    def test_cleanup_when_add_image_exception(self):
+        self.called_commands_expected = []
-        if rbd is None:
+        self.store_specs = {'image': 'fake_image',
-            msg = 'RBD store can not add images, skip test.'
+                            'snapshot': 'fake_snapshot'}
-            self.skipTest(msg)
+        self.location = StoreLocation(self.store_specs)
-
-        called_commands = []
-
-        class FakeConnection(object):
-            @contextlib.contextmanager
-            def open_ioctx(self, *args, **kwargs):
-                yield None
-
-        class FakeImage(object):
-            def write(self, *args, **kwargs):
-                called_commands.append('write')
-                return FAKE_CHUNKSIZE
-
-        @contextlib.contextmanager
-        def _fake_rados(*args, **kwargs):
-            yield FakeConnection()
-
-        @contextlib.contextmanager
-        def _fake_image(*args, **kwargs):
-            yield FakeImage()
 
+    def test_add_w_rbd_image_exception(self):
         def _fake_create_image(*args, **kwargs):
-            called_commands.append('create')
+            self.called_commands_actual.append('create')
-            return StoreLocation({'image': 'fake_image',
+            return self.location
-                                  'snapshot': 'fake_snapshot'})
 
         def _fake_delete_image(*args, **kwargs):
-            called_commands.append('delete')
+            self.called_commands_actual.append('delete')
+
+        def _fake_enter(*args, **kwargs):
+            raise exception.NotFound("")
 
-        self.stubs.Set(rados, 'Rados', _fake_rados)
-        self.stubs.Set(rbd, 'Image', _fake_image)
         self.stubs.Set(self.store, '_create_image', _fake_create_image)
         self.stubs.Set(self.store, '_delete_image', _fake_delete_image)
+        self.stubs.Set(mock_rbd.Image, '__enter__', _fake_enter)
 
+        self.assertRaises(exception.NotFound, self.store.add,
+                          'fake_image_id', StringIO.StringIO('xx'), 2)
+
+        self.called_commands_expected = ['create', 'delete']
+
+    def test_add_duplicate_image(self):
+        def _fake_create_image(*args, **kwargs):
+            self.called_commands_actual.append('create')
+            raise mock_rbd.ImageExists()
+
+        self.stubs.Set(self.store, '_create_image', _fake_create_image)
+        self.assertRaises(exception.Duplicate, self.store.add,
+                          'fake_image_id', StringIO.StringIO('xx'), 2)
+        self.called_commands_expected = ['create']
+
+    def test_delete(self):
+        def _fake_remove(*args, **kwargs):
+            self.called_commands_actual.append('remove')
+
+        self.stubs.Set(mock_rbd.RBD, 'remove', _fake_remove)
+        self.store.delete(Location('test_rbd_store', StoreLocation,
+                                   self.location.get_uri()))
+        self.called_commands_expected = ['remove']
+
+    def test__delete_image(self):
+        def _fake_remove(*args, **kwargs):
+            self.called_commands_actual.append('remove')
+
+        self.stubs.Set(mock_rbd.RBD, 'remove', _fake_remove)
+        self.store._delete_image(self.location)
+        self.called_commands_expected = ['remove']
+
+    def test__delete_image_w_snap(self):
+        def _fake_unprotect_snap(*args, **kwargs):
+            self.called_commands_actual.append('unprotect_snap')
+
+        def _fake_remove_snap(*args, **kwargs):
+            self.called_commands_actual.append('remove_snap')
+
+        def _fake_remove(*args, **kwargs):
+            self.called_commands_actual.append('remove')
+
+        self.stubs.Set(mock_rbd.RBD, 'remove', _fake_remove)
+        self.stubs.Set(mock_rbd.Image, 'unprotect_snap', _fake_unprotect_snap)
+        self.stubs.Set(mock_rbd.Image, 'remove_snap', _fake_remove_snap)
+        self.store._delete_image(self.location, snapshot_name='snap')
+
+        self.called_commands_expected = ['unprotect_snap', 'remove_snap',
+                                         'remove']
+
+    def test__delete_image_w_snap_exc_image_not_found(self):
+        def _fake_unprotect_snap(*args, **kwargs):
+            self.called_commands_actual.append('unprotect_snap')
+            raise mock_rbd.ImageNotFound()
+
+        self.stubs.Set(mock_rbd.Image, 'unprotect_snap', _fake_unprotect_snap)
+        self.assertRaises(exception.NotFound, self.store._delete_image,
+                          self.location, snapshot_name='snap')
+
+        self.called_commands_expected = ['unprotect_snap']
+
+    def test__delete_image_exc_image_not_found(self):
+        def _fake_remove(*args, **kwargs):
+            self.called_commands_actual.append('remove')
+            raise mock_rbd.ImageNotFound()
+
+        self.stubs.Set(mock_rbd.RBD, 'remove', _fake_remove)
+        self.assertRaises(exception.NotFound, self.store._delete_image,
+                          self.location, snapshot_name='snap')
+
+        self.called_commands_expected = ['remove']
+
+    def test_image_size_exceeded_exception(self):
+        def _fake_write(*args, **kwargs):
+            if 'write' not in self.called_commands_actual:
+                self.called_commands_actual.append('write')
+            raise exception.ImageSizeLimitExceeded
+
+        def _fake_delete_image(*args, **kwargs):
+            self.called_commands_actual.append('delete')
+
+        self.stubs.Set(mock_rbd.Image, 'write', _fake_write)
+        self.stubs.Set(self.store, '_delete_image', _fake_delete_image)
+        data = utils.LimitingReader(StringIO.StringIO('abcd'), 4)
         self.assertRaises(exception.ImageSizeLimitExceeded,
-                          self.store.add,
+                          self.store.add, 'fake_image_id', data, 5)
-                          'fake_image_id',
+
-                          utils.LimitingReader(StringIO.StringIO('xx'), 1),
+        self.called_commands_expected = ['write', 'delete']
-                          2)
+
-        self.assertEqual(called_commands, ['create', 'write', 'delete'])
+    def tearDown(self):
+        self.assertEqual(self.called_commands_actual,
+                         self.called_commands_expected)
+        super(TestStore, self).tearDown()