Skip to content

Commit

Permalink
Fix Qos Specs association corner case
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Zhiteng Huang committed Sep 10, 2013
1 parent 83cb5b1 commit 5aa6ca2
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 17 deletions.
11 changes: 9 additions & 2 deletions cinder/api/contrib/qos_specs_manage.py
View file
Expand Up @@ -255,7 +255,6 @@ def associate(self, req, id):
{'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',
Expand All @@ -273,6 +272,15 @@ def associate(self, req, id):
'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,
Expand Down Expand Up @@ -300,7 +308,6 @@ def disassociate(self, req, id):
{'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',
Expand Down
20 changes: 12 additions & 8 deletions cinder/db/sqlalchemy/api.py
View file
Expand Up @@ -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'
}
}
}
Expand All @@ -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}

Expand Down
29 changes: 29 additions & 0 deletions cinder/tests/test_qos_specs.py
View file
Expand Up @@ -169,13 +169,25 @@ def fake_db_associate_get(context, id):
'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()
elif type_id == 'NotFound':
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')

Expand All @@ -188,14 +200,29 @@ def fake_db_associate(context, id, type_id):

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()
Expand All @@ -217,6 +244,8 @@ def fake_db_disassociate(context, id, type_id):

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')
Expand Down
13 changes: 8 additions & 5 deletions cinder/tests/test_volume_types.py
View file
Expand Up @@ -225,14 +225,17 @@ def test_get_volume_type_qos_specs(self):
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)
27 changes: 25 additions & 2 deletions cinder/volume/qos_specs.py
View file
Expand Up @@ -25,6 +25,7 @@
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
Expand Down Expand Up @@ -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 '
Expand All @@ -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)
Expand Down

0 comments on commit 5aa6ca2

Please sign in to comment.