Fix Qos Specs association corner case

Fix a corner case in associating QoS Specs with Volumt types that was
not handled well: Raise HTTPBadRequest() to client if associate volume
type, which is already associated one qos specs other than specified
qos specs.  Previously such operation will proceed without error which
results overwriting original association.

Also refactor DB API volume_type_qos_specs_get() to return qos specs
in consistent dict format as other qos specs related APIs.

Fix bug: # 1222692

Change-Id: I9db66d1e3a7469620ba542f5387422685a2d828f

diff --git a/cinder/api/contrib/qos_specs_manage.py b/cinder/api/contrib/qos_specs_manage.py
index 3a45d39c0a8bc8c5738132058270e306713b0cc0..b60a9ca6c4541598da64e1dec7ddd649f8acbd0d 100644 (file)
--- a/cinder/api/contrib/qos_specs_manage.py
+++ b/cinder/api/contrib/qos_specs_manage.py
@@ -255,7 +255,6 @@ class QoSSpecsController(wsgi.Controller):
                   {'id': id, 'type_id': type_id})
 
         try:
-            qos_specs.get_qos_specs(context, id)
             qos_specs.associate_qos_with_type(context, id, type_id)
             notifier_info = dict(id=id, type_id=type_id)
             notifier_api.notify(context, 'QoSSpecs',
@@ -273,6 +272,15 @@ class QoSSpecsController(wsgi.Controller):
                                          'qos_specs.associate',
                                          notifier_err)
             raise webob.exc.HTTPNotFound(explanation=str(err))
+        except exception.InvalidVolumeType as err:
+            notifier_err = dict(id=id, error_message=str(err))
+            self._notify_qos_specs_error(context,
+                                         'qos_specs.associate',
+                                         notifier_err)
+            self._notify_qos_specs_error(context,
+                                         'qos_specs.associate',
+                                         notifier_err)
+            raise webob.exc.HTTPBadRequest(explanation=str(err))
         except exception.QoSSpecsAssociateFailed as err:
             notifier_err = dict(id=id, error_message=str(err))
             self._notify_qos_specs_error(context,
@@ -300,7 +308,6 @@ class QoSSpecsController(wsgi.Controller):
                   {'id': id, 'type_id': type_id})
 
         try:
-            qos_specs.get_qos_specs(context, id)
             qos_specs.disassociate_qos_specs(context, id, type_id)
             notifier_info = dict(id=id, type_id=type_id)
             notifier_api.notify(context, 'QoSSpecs',

diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py
index b0724cae66191ff11de545aec24f7f647d5a168f..be7028913c6012a4017420de86d6fa91d6bdb82f 100644 (file)
--- a/cinder/db/sqlalchemy/api.py
+++ b/cinder/db/sqlalchemy/api.py
@@ -1853,9 +1853,11 @@ def volume_type_qos_specs_get(context, type_id):
                         'id': 'qos-specs-id',
                         'name': 'qos_specs_name',
                         'consumer': 'Consumer',
-                        'key1': 'value1',
-                        'key2': 'value2',
-                        'key3': 'value3'
+                        'specs': {
+                            'key1': 'value1',
+                            'key2': 'value2',
+                            'key3': 'value3'
+                        }
                      }
         }
 
@@ -1870,11 +1872,13 @@ def volume_type_qos_specs_get(context, type_id):
             first()
 
         # row.qos_specs is a list of QualityOfServiceSpecs ref
-        specs = {}
-        for item in row.qos_specs:
-            if item.key == 'QoS_Specs_Name':
-                if item.specs:
-                    specs = _dict_with_children_specs(item.specs)
+        specs = _dict_with_qos_specs(row.qos_specs)
+
+        if not specs:
+            # turn empty list to None
+            specs = None
+        else:
+            specs = specs[0]
 
         return {'qos_specs': specs}
 

diff --git a/cinder/tests/test_qos_specs.py b/cinder/tests/test_qos_specs.py
index 30453a4eb94dcf888abb63296b13e9885352dc77..3246edb42f7f2553ee8b4c1a7fe10d5ff9119ac1 100644 (file)
--- a/cinder/tests/test_qos_specs.py
+++ b/cinder/tests/test_qos_specs.py
@@ -169,6 +169,12 @@ class QoSSpecsTestCase(test.TestCase):
                           'Trouble')
 
     def test_associate_qos_with_type(self):
+        def fake_qos_specs_get(context, id):
+            if id == 'NotFound':
+                raise exception.QoSSpecsNotFound(specs_id=id)
+            else:
+                pass
+
         def fake_db_associate(context, id, type_id):
             if id == 'Trouble':
                 raise db_exc.DBError()
@@ -176,6 +182,12 @@ class QoSSpecsTestCase(test.TestCase):
                 raise exception.VolumeTypeNotFound(volume_type_id=type_id)
             pass
 
+        def fake_vol_type_qos_get(type_id):
+            if type_id == 'Invalid':
+                return {'qos_specs': {'id': 'Invalid'}}
+            else:
+                return {'qos_specs': None}
+
         type_ref = volume_types.create(self.ctxt, 'TypeName')
         specs_id = self._create_qos_specs('QoSName')
 
@@ -188,14 +200,29 @@ class QoSSpecsTestCase(test.TestCase):
 
         self.stubs.Set(db, 'qos_specs_associate',
                        fake_db_associate)
+        self.stubs.Set(qos_specs, 'get_qos_specs', fake_qos_specs_get)
+        self.stubs.Set(volume_types, 'get_volume_type_qos_specs',
+                       fake_vol_type_qos_get)
         self.assertRaises(exception.VolumeTypeNotFound,
                           qos_specs.associate_qos_with_type,
                           self.ctxt, 'specs-id', 'NotFound')
         self.assertRaises(exception.QoSSpecsAssociateFailed,
                           qos_specs.associate_qos_with_type,
                           self.ctxt, 'Trouble', 'id')
+        self.assertRaises(exception.QoSSpecsNotFound,
+                          qos_specs.associate_qos_with_type,
+                          self.ctxt, 'NotFound', 'id')
+        self.assertRaises(exception.InvalidVolumeType,
+                          qos_specs.associate_qos_with_type,
+                          self.ctxt, 'specs-id', 'Invalid')
 
     def test_disassociate_qos_specs(self):
+        def fake_qos_specs_get(context, id):
+            if id == 'NotFound':
+                raise exception.QoSSpecsNotFound(specs_id=id)
+            else:
+                pass
+
         def fake_db_disassociate(context, id, type_id):
             if id == 'Trouble':
                 raise db_exc.DBError()
@@ -217,6 +244,8 @@ class QoSSpecsTestCase(test.TestCase):
 
         self.stubs.Set(db, 'qos_specs_disassociate',
                        fake_db_disassociate)
+        self.stubs.Set(qos_specs, 'get_qos_specs',
+                       fake_qos_specs_get)
         self.assertRaises(exception.VolumeTypeNotFound,
                           qos_specs.disassociate_qos_specs,
                           self.ctxt, 'specs-id', 'NotFound')

diff --git a/cinder/tests/test_volume_types.py b/cinder/tests/test_volume_types.py
index 25cea5776c8947b9dd5fefa64d642e0e56ed87cf..73c9327750062a27328077139cede119f1433ece 100644 (file)
--- a/cinder/tests/test_volume_types.py
+++ b/cinder/tests/test_volume_types.py
@@ -225,14 +225,17 @@ class VolumeTypeTestCase(test.TestCase):
         type_ref = volume_types.create(self.ctxt, "type1", {"key2": "val2",
                                                   "key3": "val3"})
         res = volume_types.get_volume_type_qos_specs(type_ref['id'])
-        self.assertEqual(res['qos_specs'], {})
+        self.assertEqual(res['qos_specs'], None)
         qos_specs.associate_qos_with_type(self.ctxt,
                                           qos_ref['id'],
                                           type_ref['id'])
 
-        expected = {'qos_specs': {'consumer': 'back-end',
-                                  'k1': 'v1',
-                                  'k2': 'v2',
-                                  'k3': 'v3'}}
+        expected = {'qos_specs': {'id': qos_ref['id'],
+                                  'name': 'qos-specs-1',
+                                  'consumer': 'back-end',
+                                  'specs': {
+                                      'k1': 'v1',
+                                      'k2': 'v2',
+                                      'k3': 'v3'}}}
         res = volume_types.get_volume_type_qos_specs(type_ref['id'])
         self.assertDictMatch(expected, res)

diff --git a/cinder/volume/qos_specs.py b/cinder/volume/qos_specs.py
index 436c482058fbd68ddcb99ad8e5ef5fd5db751224..19fa3e0b45702b800c071d45289e7e1804fe185b 100644 (file)
--- a/cinder/volume/qos_specs.py
+++ b/cinder/volume/qos_specs.py
@@ -25,6 +25,7 @@ from cinder import db
 from cinder import exception
 from cinder.openstack.common.db import exception as db_exc
 from cinder.openstack.common import log as logging
+from cinder.volume import volume_types
 
 
 CONF = cfg.CONF
@@ -158,9 +159,30 @@ def get_associations(context, specs_id):
 
 
 def associate_qos_with_type(context, specs_id, type_id):
-    """Associate qos_specs from volume type."""
+    """Associate qos_specs with volume type.
+
+    Associate target qos specs with specific volume type. Would raise
+    following exceptions:
+        VolumeTypeNotFound  - if volume type doesn't exist;
+        QoSSpecsNotFound  - if qos specs doesn't exist;
+        InvalidVolumeType  - if volume type is already associated with
+                             qos specs other than given one.
+        QoSSpecsAssociateFailed -  if there was general DB error
+    :param specs_id: qos specs ID to associate with
+    :param type_id: volume type ID to associate with
+    """
     try:
-        db.qos_specs_associate(context, specs_id, type_id)
+        get_qos_specs(context, specs_id)
+        res = volume_types.get_volume_type_qos_specs(type_id)
+        if res.get('qos_specs', None):
+            if res['qos_specs'].get('id') != specs_id:
+                msg = (_("Type %(type_id)s is already associated with another "
+                         "qos specs: %(qos_specs_id)s") %
+                       {'type_id': type_id,
+                        'qos_specs_id': res['qos_specs']['id']})
+                raise exception.InvalidVolumeType(reason=msg)
+        else:
+            db.qos_specs_associate(context, specs_id, type_id)
     except db_exc.DBError as e:
         LOG.exception(_('DB error: %s') % e)
         LOG.warn(_('Failed to associate qos specs '
@@ -173,6 +195,7 @@ def associate_qos_with_type(context, specs_id, type_id):
 def disassociate_qos_specs(context, specs_id, type_id):
     """Disassociate qos_specs from volume type."""
     try:
+        get_qos_specs(context, specs_id)
         db.qos_specs_disassociate(context, specs_id, type_id)
     except db_exc.DBError as e:
         LOG.exception(_('DB error: %s') % e)