From ca85d237e236f46881dc2c57a589a33e4605917d Mon Sep 17 00:00:00 2001 From: Zhiteng Huang Date: Tue, 10 Sep 2013 16:39:52 +0800 Subject: [PATCH] Fix deleting qos specs key Previously deleting a key in certain qos specs was accomplished via 'update' API. Unfortunately, 'update' isn't able to tell the difference between setting a key with no value and deleting a key (and its value). This change adds an new API 'delete_keys' to qos_specs API extension. 'delete_keys' API allows client to specify a list of to-be-deleted keys in one single request (batch mode!), which can be handy when removing multiple keys in a qos specs. Example URL and request body for 'delete_keys' API: PUT to http://127.0.0.1:8776/v2/qos-specs/QOS_SPECS_UUID/delete_keys with body: {'keys': ['foo', 'bar', 'zoo']} Above example request will result in 'foo', 'bar', 'zoo' key/value pairs of QOS_SPECS_UUID be marked as deleted in DB. If QOS_SPECS_UUID doesn't exist, a 404 error will return; if any key in 'foo', 'bar', 'zoo' couldn't be found in QOS_SPECS_UUID, a 400 error will return with error message telling which key couldn't be found. Note that Cinder will puke 400 and stop trying the rest once it encounters a non-existing (or deleted) key amoung the given list of keys. This change also fixes 'list'/'show' API includes deleted keys in result. Fix bug: # 1223660 Fix bug: # 1223677 Change-Id: Ia3cb07e204d655a9b837b317ce7117feb3c86a2d --- cinder/api/contrib/qos_specs_manage.py | 43 ++++++++++++++-- cinder/db/api.py | 5 ++ cinder/db/sqlalchemy/api.py | 19 ++++--- .../api/contrib/test_qos_specs_manage.py | 42 +++++++++++++++- cinder/tests/db/test_qos_specs.py | 16 ++++++ cinder/tests/test_qos_specs.py | 49 +++++++++++++++++++ cinder/volume/qos_specs.py | 33 +++++++++---- 7 files changed, 186 insertions(+), 21 deletions(-) diff --git a/cinder/api/contrib/qos_specs_manage.py b/cinder/api/contrib/qos_specs_manage.py index b60a9ca6c..89b309f08 100644 --- a/cinder/api/contrib/qos_specs_manage.py +++ b/cinder/api/contrib/qos_specs_manage.py @@ -26,6 +26,7 @@ from cinder.api import xmlutil from cinder import exception from cinder.openstack.common import log as logging from cinder.openstack.common.notifier import api as notifier_api +from cinder.openstack.common import strutils from cinder.volume import qos_specs @@ -157,6 +158,7 @@ class QoSSpecsController(wsgi.Controller): 'qos_specs.update', notifier_err) raise webob.exc.HTTPInternalServerError(explanation=str(err)) + return body @wsgi.serializers(xml=QoSSpecsTemplate) @@ -179,11 +181,12 @@ class QoSSpecsController(wsgi.Controller): force = req.params.get('force', None) + #convert string to bool type in strict manner + force = strutils.bool_from_string(force) LOG.debug("Delete qos_spec: %(id)s, force: %(force)s" % {'id': id, 'force': force}) try: - qos_specs.get_qos_specs(context, id) qos_specs.delete(context, id, force) notifier_info = dict(id=id) notifier_api.notify(context, 'QoSSpecs', @@ -208,6 +211,40 @@ class QoSSpecsController(wsgi.Controller): return webob.Response(status_int=202) + def delete_keys(self, req, id, body): + """Deletes specified keys in qos specs.""" + context = req.environ['cinder.context'] + authorize(context) + + if not (body and 'keys' in body + and isinstance(body.get('keys'), list)): + raise webob.exc.HTTPBadRequest() + + keys = body['keys'] + LOG.debug("Delete_key spec: %(id)s, keys: %(keys)s" % + {'id': id, 'keys': keys}) + + try: + qos_specs.delete_keys(context, id, keys) + notifier_info = dict(id=id) + notifier_api.notify(context, 'QoSSpecs', + 'qos_specs.delete_keys', + notifier_api.INFO, notifier_info) + except exception.QoSSpecsNotFound as err: + notifier_err = dict(id=id, error_message=str(err)) + self._notify_qos_specs_error(context, + 'qos_specs.delete_keys', + notifier_err) + raise webob.exc.HTTPNotFound(explanation=str(err)) + except exception.QoSSpecsKeyNotFound as err: + notifier_err = dict(id=id, error_message=str(err)) + self._notify_qos_specs_error(context, + 'qos_specs.delete_keys', + notifier_err) + raise webob.exc.HTTPBadRequest(explanation=str(err)) + + return webob.Response(status_int=202) + @wsgi.serializers(xml=QoSSpecsTemplate) def associations(self, req, id): """List all associations of given qos specs.""" @@ -342,7 +379,6 @@ class QoSSpecsController(wsgi.Controller): LOG.debug("Disassociate qos_spec: %s from all." % id) try: - qos_specs.get_qos_specs(context, id) qos_specs.disassociate_all(context, id) notifier_info = dict(id=id) notifier_api.notify(context, 'QoSSpecs', @@ -380,7 +416,8 @@ class Qos_specs_manage(extensions.ExtensionDescriptor): member_actions={"associations": "GET", "associate": "GET", "disassociate": "GET", - "disassociate_all": "GET"}) + "disassociate_all": "GET", + "delete_keys": "PUT"}) resources.append(res) diff --git a/cinder/db/api.py b/cinder/db/api.py index 200ad6d7a..df81e5ac8 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -524,6 +524,11 @@ def qos_specs_delete(context, qos_specs_id): IMPL.qos_specs_delete(context, qos_specs_id) +def qos_specs_item_delete(context, qos_specs_id, key): + """Delete specified key in the qos_specs.""" + IMPL.qos_specs_item_delete(context, qos_specs_id, key) + + def qos_specs_update(context, qos_specs_id, specs): """Update qos specs. diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index be7028913..34f4699bc 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -2081,7 +2081,9 @@ def _dict_with_children_specs(specs): """Convert specs list to a dict.""" result = {} for spec in specs: - result.update({spec['key']: spec['value']}) + # Skip deleted keys + if not spec['deleted']: + result.update({spec['key']: spec['value']}) return result @@ -2203,12 +2205,15 @@ def qos_specs_disassociate_all(context, qos_specs_id): @require_admin_context def qos_specs_item_delete(context, qos_specs_id, key): - _qos_specs_get_item(context, qos_specs_id, key) - _qos_specs_get_ref(context, qos_specs_id, None). \ - filter_by(key=key). \ - update({'deleted': True, - 'deleted_at': timeutils.utcnow(), - 'updated_at': literal_column('updated_at')}) + session = get_session() + with session.begin(): + _qos_specs_get_item(context, qos_specs_id, key) + session.query(models.QualityOfServiceSpecs). \ + filter(models.QualityOfServiceSpecs.key == key). \ + filter(models.QualityOfServiceSpecs.specs_id == qos_specs_id). \ + update({'deleted': True, + 'deleted_at': timeutils.utcnow(), + 'updated_at': literal_column('updated_at')}) @require_admin_context diff --git a/cinder/tests/api/contrib/test_qos_specs_manage.py b/cinder/tests/api/contrib/test_qos_specs_manage.py index e9ce2480e..6ac7dd262 100644 --- a/cinder/tests/api/contrib/test_qos_specs_manage.py +++ b/cinder/tests/api/contrib/test_qos_specs_manage.py @@ -67,6 +67,15 @@ def return_qos_specs_delete(context, id, force): pass +def return_qos_specs_delete_keys(context, id, keys): + if id == "777": + raise exception.QoSSpecsNotFound(specs_id=id) + + if 'foo' in keys: + raise exception.QoSSpecsKeyNotFound(specs_id=id, + specs_key='foo') + + def return_qos_specs_update(context, id, specs): if id == "777": raise exception.QoSSpecsNotFound(specs_id=id) @@ -134,7 +143,7 @@ class QoSSpecManageApiTest(test.TestCase): host='fake', notification_driver=[test_notifier.__name__]) self.controller = qos_specs_manage.QoSSpecsController() - """to reset notifier drivers left over from other api/contrib tests""" + #reset notifier drivers left over from other api/contrib tests notifier_api._reset_drivers() test_notifier.NOTIFICATIONS = [] @@ -205,6 +214,37 @@ class QoSSpecManageApiTest(test.TestCase): req, '666') self.assertEqual(len(test_notifier.NOTIFICATIONS), 1) + def test_qos_specs_delete_keys(self): + self.stubs.Set(qos_specs, 'delete_keys', + return_qos_specs_delete_keys) + body = {"keys": ['bar', 'zoo']} + req = fakes.HTTPRequest.blank('/v2/fake/qos-specs/666/delete_keys') + self.assertEqual(len(test_notifier.NOTIFICATIONS), 0) + self.controller.delete_keys(req, '666', body) + self.assertEqual(len(test_notifier.NOTIFICATIONS), 1) + + def test_qos_specs_delete_keys_qos_notfound(self): + self.stubs.Set(qos_specs, 'delete_keys', + return_qos_specs_delete_keys) + body = {"keys": ['bar', 'zoo']} + req = fakes.HTTPRequest.blank('/v2/fake/qos-specs/777/delete_keys') + self.assertEqual(len(test_notifier.NOTIFICATIONS), 0) + self.assertRaises(webob.exc.HTTPNotFound, + self.controller.delete_keys, + req, '777', body) + self.assertEqual(len(test_notifier.NOTIFICATIONS), 1) + + def test_qos_specs_delete_keys_badkey(self): + self.stubs.Set(qos_specs, 'delete_keys', + return_qos_specs_delete_keys) + req = fakes.HTTPRequest.blank('/v2/fake/qos-specs/666/delete_keys') + body = {"keys": ['foo', 'zoo']} + self.assertEqual(len(test_notifier.NOTIFICATIONS), 0) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.delete_keys, + req, '666', body) + self.assertEqual(len(test_notifier.NOTIFICATIONS), 1) + def test_create(self): self.stubs.Set(qos_specs, 'create', return_qos_specs_create) diff --git a/cinder/tests/db/test_qos_specs.py b/cinder/tests/db/test_qos_specs.py index 88357fc19..35c512e77 100644 --- a/cinder/tests/db/test_qos_specs.py +++ b/cinder/tests/db/test_qos_specs.py @@ -136,6 +136,22 @@ class QualityOfServiceSpecsTableTestCase(test.TestCase): self.assertRaises(exception.QoSSpecsNotFound, db.qos_specs_get, self.ctxt, specs_id) + def test_qos_specs_item_delete(self): + name = str(int(time.time())) + value = dict(consumer='front-end', + foo='Foo', bar='Bar') + specs_id = self._create_qos_specs(name, value) + + del value['consumer'] + del value['foo'] + expected = {'name': name, + 'id': specs_id, + 'consumer': 'front-end', + 'specs': value} + db.qos_specs_item_delete(self.ctxt, specs_id, 'foo') + specs = db.qos_specs_get_by_name(self.ctxt, name) + self.assertDictMatch(specs, expected) + def test_associate_type_with_qos(self): self.assertRaises(exception.VolumeTypeNotFound, db.volume_type_qos_associate, diff --git a/cinder/tests/test_qos_specs.py b/cinder/tests/test_qos_specs.py index 3246edb42..12cf1f25a 100644 --- a/cinder/tests/test_qos_specs.py +++ b/cinder/tests/test_qos_specs.py @@ -145,6 +145,47 @@ class QoSSpecsTestCase(test.TestCase): # able to delete in-use qos specs if force=True qos_specs.delete(self.ctxt, 'InUse', force=True) + def test_delete_keys(self): + def fake_db_qos_delete_key(context, id, key): + if key == 'NotFound': + raise exception.QoSSpecsKeyNotFound(specs_id=id, + specs_key=key) + else: + pass + + def fake_qos_specs_get(context, id): + if id == 'NotFound': + raise exception.QoSSpecsNotFound(specs_id=id) + else: + pass + + value = dict(consumer='front-end', + foo='Foo', bar='Bar', zoo='tiger') + specs_id = self._create_qos_specs('QoSName', value) + qos_specs.delete_keys(self.ctxt, specs_id, ['foo', 'bar']) + del value['consumer'] + del value['foo'] + del value['bar'] + expected = {'name': 'QoSName', + 'id': specs_id, + 'consumer': 'front-end', + 'specs': value} + specs = qos_specs.get_qos_specs(self.ctxt, specs_id) + self.assertDictMatch(expected, specs) + + self.stubs.Set(qos_specs, 'get_qos_specs', fake_qos_specs_get) + self.stubs.Set(db, 'qos_specs_item_delete', fake_db_qos_delete_key) + self.assertRaises(exception.InvalidQoSSpecs, + qos_specs.delete_keys, self.ctxt, None, []) + self.assertRaises(exception.QoSSpecsNotFound, + qos_specs.delete_keys, self.ctxt, 'NotFound', []) + self.assertRaises(exception.QoSSpecsKeyNotFound, + qos_specs.delete_keys, self.ctxt, + 'Found', ['NotFound']) + self.assertRaises(exception.QoSSpecsKeyNotFound, + qos_specs.delete_keys, self.ctxt, 'Found', + ['foo', 'bar', 'NotFound']) + def test_get_associations(self): def fake_db_associate_get(context, id): if id == 'Trouble': @@ -259,6 +300,12 @@ class QoSSpecsTestCase(test.TestCase): raise db_exc.DBError() pass + def fake_qos_specs_get(context, id): + if id == 'NotFound': + raise exception.QoSSpecsNotFound(specs_id=id) + else: + pass + type1_ref = volume_types.create(self.ctxt, 'TypeName1') type2_ref = volume_types.create(self.ctxt, 'TypeName2') specs_id = self._create_qos_specs('QoSName') @@ -276,6 +323,8 @@ class QoSSpecsTestCase(test.TestCase): self.stubs.Set(db, 'qos_specs_disassociate_all', fake_db_disassociate_all) + self.stubs.Set(qos_specs, 'get_qos_specs', + fake_qos_specs_get) self.assertRaises(exception.QoSSpecsDisassociateFailed, qos_specs.disassociate_all, self.ctxt, 'Trouble') diff --git a/cinder/volume/qos_specs.py b/cinder/volume/qos_specs.py index 19fa3e0b4..0965df149 100644 --- a/cinder/volume/qos_specs.py +++ b/cinder/volume/qos_specs.py @@ -124,16 +124,28 @@ def delete(context, qos_specs_id, force=False): if qos_specs_id is None: msg = _("id cannot be None") raise exception.InvalidQoSSpecs(reason=msg) - else: - # check if there is any entity associated with this - # qos specs. - res = db.qos_specs_associations_get(context, qos_specs_id) - if res and not force: - raise exception.QoSSpecsInUse(specs_id=qos_specs_id) - elif force: - # remove all association - disassociate_all(context, qos_specs_id) - db.qos_specs_delete(context, qos_specs_id) + + # check if there is any entity associated with this qos specs + res = db.qos_specs_associations_get(context, qos_specs_id) + if res and not force: + raise exception.QoSSpecsInUse(specs_id=qos_specs_id) + elif res and force: + # remove all association + db.qos_specs_disassociate_all(context, qos_specs_id) + + db.qos_specs_delete(context, qos_specs_id) + + +def delete_keys(context, qos_specs_id, keys): + """Marks specified key of target qos specs as deleted.""" + if qos_specs_id is None: + msg = _("id cannot be None") + raise exception.InvalidQoSSpecs(reason=msg) + + # make sure qos_specs_id is valid + get_qos_specs(context, qos_specs_id) + for key in keys: + db.qos_specs_item_delete(context, qos_specs_id, key) def get_associations(context, specs_id): @@ -209,6 +221,7 @@ def disassociate_qos_specs(context, specs_id, type_id): def disassociate_all(context, specs_id): """Disassociate qos_specs from all entities.""" try: + get_qos_specs(context, specs_id) db.qos_specs_disassociate_all(context, specs_id) except db_exc.DBError as e: LOG.exception(_('DB error: %s') % e) -- 2.45.2