From 5aa6ca2e80631a32e429bcacbd031ddaa6e5bfb1 Mon Sep 17 00:00:00 2001 From: Zhiteng Huang Date: Mon, 9 Sep 2013 14:53:37 +0800 Subject: [PATCH] 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 --- cinder/api/contrib/qos_specs_manage.py | 11 ++++++++-- cinder/db/sqlalchemy/api.py | 20 +++++++++++------- cinder/tests/test_qos_specs.py | 29 ++++++++++++++++++++++++++ cinder/tests/test_volume_types.py | 13 +++++++----- cinder/volume/qos_specs.py | 27 ++++++++++++++++++++++-- 5 files changed, 83 insertions(+), 17 deletions(-) diff --git a/cinder/api/contrib/qos_specs_manage.py b/cinder/api/contrib/qos_specs_manage.py index 3a45d39c0..b60a9ca6c 100644 --- 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 b0724cae6..be7028913 100644 --- 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 30453a4eb..3246edb42 100644 --- 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 25cea5776..73c932775 100644 --- 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 436c48205..19fa3e0b4 100644 --- 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) -- 2.45.2