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

diff --git a/cinder/api/contrib/qos_specs_manage.py b/cinder/api/contrib/qos_specs_manage.py
index b60a9ca6c4541598da64e1dec7ddd649f8acbd0d..89b309f08c4a3205724e91389d2c15989b3acbf0 100644 (file)
--- 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 200ad6d7acff093d4d8d857954749c59a7b542cf..df81e5ac83588db84ebe4d7fee24cd36b31e5928 100644 (file)
--- 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 be7028913c6012a4017420de86d6fa91d6bdb82f..34f4699bc82c4a4eebe41766b710df7c79e72532 100644 (file)
--- 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 e9ce2480e4648dff718d528e896c1931d0121b30..6ac7dd2629d0f100f53b54065e55e32e94842515 100644 (file)
--- 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 88357fc19b2984d4d6648cdec582394686999309..35c512e77f7a1c2611d48dda7c1a8c08f777bd20 100644 (file)
--- 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 3246edb42f7f2553ee8b4c1a7fe10d5ff9119ac1..12cf1f25a428bc5c08528a171d7be5291e8727fc 100644 (file)
--- 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 19fa3e0b45702b800c071d45289e7e1804fe185b..0965df14977fb9ca34e062445e9c1cb3b013a6d2 100644 (file)
--- 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)