Merge "Validate volume type during volume create"

4 files changed, 104 insertions, 46 deletions

diff --git a/glance_store/_drivers/cinder.py b/glance_store/_drivers/cinder.py
index 3160a75..c04a073 100644
--- a/glance_store/_drivers/cinder.py
+++ b/glance_store/_drivers/cinder.py
@@ -33,7 +33,7 @@ from glance_store import capabilities
 from glance_store.common import utils
 import glance_store.driver
 from glance_store import exceptions
-from glance_store.i18n import _, _LE, _LI
+from glance_store.i18n import _, _LE, _LI, _LW
 import glance_store.location
 
 try:
@@ -405,38 +405,26 @@ class Store(glance_store.driver.Store):
 
     def configure_add(self):
         """
-        Configure the Store to use the stored configuration options
-        Any store that needs special configuration should implement
-        this method. If the store was not able to successfully configure
-        itself, it should raise `exceptions.BadStoreConfiguration`
-        :raises: `exceptions.BadStoreConfiguration` if multiple stores are
-                 defined and particular store wasn't able to configure
-                 successfully
-        :raises: `exceptions.BackendException` if single store is defined and
-                 it wasn't able to configure successfully
+        Check to verify if the volume types configured for the cinder store
+        exist in deployment and if not, log a warning.
         """
-        if self.backend_group:
-            cinder_volume_type = self.store_conf.cinder_volume_type
-            if cinder_volume_type:
-                # NOTE: `cinder_volume_type` is configured, check
-                # configured volume_type is available in cinder or not
-                cinder_client = self.get_cinderclient()
-                try:
-                    # We don't even need the volume type object, as long
-                    # as this returns clean, we know the name is good.
-                    cinder_client.volume_types.find(name=cinder_volume_type)
-                    # No need to worry NoUniqueMatch as volume type name is
-                    # unique
-                except cinder_exception.NotFound:
-                    reason = _("Invalid `cinder_volume_type %s`"
-                               % cinder_volume_type)
-                    if len(self.conf.enabled_backends) > 1:
-                        LOG.error(reason)
-                        raise exceptions.BadStoreConfiguration(
-                            store_name=self.backend_group, reason=reason)
-                    else:
-                        LOG.critical(reason)
-                        raise exceptions.BackendException(reason)
+        cinder_volume_type = self.store_conf.cinder_volume_type
+        if cinder_volume_type:
+            # NOTE: `cinder_volume_type` is configured, check
+            # configured volume_type is available in cinder or not
+            cinder_client = self.get_cinderclient()
+            try:
+                # We don't even need the volume type object, as long
+                # as this returns clean, we know the name is good.
+                cinder_client.volume_types.find(name=cinder_volume_type)
+                # No need to worry about a NoUniqueMatch as volume type name
+                # is unique
+            except cinder_exception.NotFound:
+                reason = (_LW("Invalid `cinder_volume_type %s`"
+                              % cinder_volume_type))
+                LOG.warning(reason)
+            except cinder_exception.ClientException:
+                pass
 
     def is_image_associated_with_store(self, context, volume_id):
         """
@@ -849,8 +837,18 @@ class Store(glance_store.driver.Store):
             LOG.info(_LI("Since image size is zero, we will be doing "
                          "resize-before-write for each GB which "
                          "will be considerably slower than normal."))
-        volume = client.volumes.create(size_gb, name=name, metadata=metadata,
-                                       volume_type=volume_type)
+        try:
+            volume = client.volumes.create(size_gb, name=name,
+                                           metadata=metadata,
+                                           volume_type=volume_type)
+        except cinder_exception.NotFound:
+            LOG.error(_LE("Invalid volume type %s configured. Please check "
+                          "the `cinder_volume_type` configuration parameter."
+                          % volume_type))
+            msg = (_("Failed to create image-volume due to invalid "
+                     "`cinder_volume_type` configured."))
+            raise exceptions.BackendException(msg)
+
         volume = self._wait_volume_status(volume, 'creating', 'available')
         size_gb = volume.size
 
diff --git a/glance_store/tests/unit/test_cinder_store.py b/glance_store/tests/unit/test_cinder_store.py
index 858bcbd..badbb5e 100644
--- a/glance_store/tests/unit/test_cinder_store.py
+++ b/glance_store/tests/unit/test_cinder_store.py
@@ -413,3 +413,23 @@ class TestCinderStore(base.StoreBaseTest,
 
     def test_set_url_prefix(self):
         self.assertEqual('cinder://', self.store._url_prefix)
+
+    def test_configure_add(self):
+
+        def fake_volume_type(name):
+            if name != 'some_type':
+                raise cinder.cinder_exception.NotFound(code=404)
+
+        with mock.patch.object(self.store, 'get_cinderclient') as mocked_cc:
+            mocked_cc.return_value = FakeObject(volume_types=FakeObject(
+                find=fake_volume_type))
+            self.config(cinder_volume_type='some_type')
+            # If volume type exists, no exception is raised
+            self.store.configure_add()
+            # setting cinder_volume_type to non-existent value will log a
+            # warning
+            self.config(cinder_volume_type='some_random_type')
+            with mock.patch.object(cinder, 'LOG') as mock_log:
+                self.store.configure_add()
+                mock_log.warning.assert_called_with(
+                    "Invalid `cinder_volume_type some_random_type`")
diff --git a/glance_store/tests/unit/test_multistore_cinder.py b/glance_store/tests/unit/test_multistore_cinder.py
index 4a9a76f..71933ae 100644
--- a/glance_store/tests/unit/test_multistore_cinder.py
+++ b/glance_store/tests/unit/test_multistore_cinder.py
@@ -271,27 +271,55 @@ class TestMultiCinderStore(base.MultiStoreBaseTest,
 
         self.store._check_context(FakeObject(service_catalog='fake'))
 
-    def test_cinder_configure_add(self):
+    def test_configure_add(self):
+
+        def fake_volume_type_check(name):
+            if name != 'some_type':
+                raise cinder.cinder_exception.NotFound(code=404)
+
         with mock.patch.object(self.store, 'get_cinderclient') as mocked_cc:
-            def raise_(ex):
-                raise ex
             mocked_cc.return_value = FakeObject(volume_types=FakeObject(
-                find=lambda name: 'some_type' if name == 'some_type'
-                else raise_(cinder.cinder_exception.NotFound(code=404))))
+                find=fake_volume_type_check))
             self.config(cinder_volume_type='some_type',
                         group=self.store.backend_group)
             # If volume type exists, no exception is raised
             self.store.configure_add()
-            # setting cinder_volume_type to non-existent value will raise
-            # BadStoreConfiguration exception
+            # setting cinder_volume_type to non-existent value will log a
+            # warning
             self.config(cinder_volume_type='some_random_type',
                         group=self.store.backend_group)
+            with mock.patch.object(cinder, 'LOG') as mock_log:
+                self.store.configure_add()
+                mock_log.warning.assert_called_with(
+                    "Invalid `cinder_volume_type some_random_type`")
 
-            self.assertRaises(exceptions.BadStoreConfiguration,
-                              self.store.configure_add)
-            # when only 1 store is configured, BackendException is raised
-            self.config(enabled_backends={'cinder1': 'cinder'})
-            self.assertRaises(exceptions.BackendException,
+    def test_configure_add_cinder_service_down(self):
+
+        def fake_volume_type_check(name):
+            raise cinder.cinder_exception.ClientException(code=503)
+
+        self.config(cinder_volume_type='some_type',
+                    group=self.store.backend_group)
+        with mock.patch.object(self.store, 'get_cinderclient') as mocked_cc:
+            mocked_cc.return_value = FakeObject(volume_types=FakeObject(
+                find=fake_volume_type_check))
+            # We handle the ClientException to pass so no exception is raised
+            # in this case
+            self.store.configure_add()
+
+    def test_configure_add_authorization_failed(self):
+
+        def fake_volume_type_check(name):
+            raise cinder.exceptions.AuthorizationFailure(code=401)
+
+        self.config(cinder_volume_type='some_type',
+                    group=self.store.backend_group)
+        with mock.patch.object(self.store, 'get_cinderclient') as mocked_cc:
+            mocked_cc.return_value = FakeObject(volume_types=FakeObject(
+                find=fake_volume_type_check))
+            # Anything apart from invalid volume type or cinder service
+            # down will raise an exception
+            self.assertRaises(cinder.exceptions.AuthorizationFailure,
                               self.store.configure_add)
 
     def test_is_image_associated_with_store(self):
diff --git a/releasenotes/notes/volume-type-validation-check-011a400d7fb3b307.yaml b/releasenotes/notes/volume-type-validation-check-011a400d7fb3b307.yaml
new file mode 100644
index 0000000..7ff169b
--- /dev/null
+++ b/releasenotes/notes/volume-type-validation-check-011a400d7fb3b307.yaml
@@ -0,0 +1,12 @@
+---
+upgrade:
+  - |
+    Previously, during service startup, the check to validate volume types
+    used to raise ``BackendException`` or ``BadStoreConfiguration`` exceptions
+    when an invalid volume type was configured hence failing the service
+    startup. It now logs a warning and the glance service starts normally.
+fixes:
+  - |
+    `Bug #1915163 <https://bugs.launchpad.net/glance-store/+bug/1915163>`_:
+    Added handling to log and raise proper exception during image create when
+    an invalid volume type is configured.