From 69bac2f99c01b3e5bc09179c03fd24606e12d041 Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Sun, 23 Oct 2016 10:58:25 +0200 Subject: [PATCH] placement: raise exc when resource class not found The ResourceProvider.set_inventory() method was not raising NotFound when an Inventory object in the supplied InventoryList object was using a resource class that did not exist in the resource class cache. This patch fixes that by having the string_from_id() and id_from_string() methods of the resource class cache raise ResourceClassNotFound instead of returning None. We raise specific ResourceClassNotFound and InventoryWithResourceClassNotFound exception subclasses to differentiate in the inventory API handler between an unknown resource class and a known resource class that doesn't exist in a given resource provider's inventory. We return a 400 Not Found when InventoryWithResourceClassNotFound is raised on updating a single inventory, but return a 409 Conflict when InventoryWithResourceClassNotFound is raised on updating a set of inventory records since in the latter scenario, it represents a race condition that frankly should not occur. Change-Id: I350b02dcdbaa9d30d885cd085f60daa6b53a7745 --- .../openstack/placement/handlers/inventory.py | 19 +++++++++ nova/db/sqlalchemy/resource_class_cache.py | 13 +++++- nova/exception.py | 8 ++++ nova/objects/resource_provider.py | 42 ++++++++++++------- .../placement/gabbits/allocations.yaml | 2 +- .../placement/gabbits/inventory.yaml | 26 +++++++++++- .../db/test_resource_class_cache.py | 20 +++++++-- .../functional/db/test_resource_provider.py | 28 ++++++++++++- 8 files changed, 135 insertions(+), 23 deletions(-) diff --git a/nova/api/openstack/placement/handlers/inventory.py b/nova/api/openstack/placement/handlers/inventory.py index 2a445cc05b2c..814475e50a0b 100644 --- a/nova/api/openstack/placement/handlers/inventory.py +++ b/nova/api/openstack/placement/handlers/inventory.py @@ -340,6 +340,19 @@ def set_inventories(req): try: resource_provider.set_inventory(inventories) + except exception.ResourceClassNotFound as exc: + raise webob.exc.HTTPBadRequest( + _('Unknown resource class in inventory for resource provider ' + '%(rp_uuid)s: %(error)s') % {'rp_uuid': resource_provider.uuid, + 'error': exc}, + json_formatter=util.json_error_formatter) + except exception.InventoryWithResourceClassNotFound as exc: + raise webob.exc.HTTPConflict( + _('Race condition detected when setting inventory. No inventory ' + 'record with resource class for resource provider ' + '%(rp_uuid)s: %(error)s') % {'rp_uuid': resource_provider.uuid, + 'error': exc}, + json_formatter=util.json_error_formatter) except (exception.ConcurrentUpdateDetected, exception.InventoryInUse, db_exc.DBDuplicateEntry) as exc: @@ -392,6 +405,12 @@ def update_inventory(req): raise webob.exc.HTTPConflict( _('update conflict: %(error)s') % {'error': exc}, json_formatter=util.json_error_formatter) + except exception.InventoryWithResourceClassNotFound as exc: + raise webob.exc.HTTPBadRequest( + _('No inventory record with resource class for resource provider ' + '%(rp_uuid)s: %(error)s') % {'rp_uuid': resource_provider.uuid, + 'error': exc}, + json_formatter=util.json_error_formatter) except exception.InvalidInventoryCapacity as exc: raise webob.exc.HTTPBadRequest( _('Unable to update inventory for resource provider ' diff --git a/nova/db/sqlalchemy/resource_class_cache.py b/nova/db/sqlalchemy/resource_class_cache.py index 97304311fea9..e1ce13da68e9 100644 --- a/nova/db/sqlalchemy/resource_class_cache.py +++ b/nova/db/sqlalchemy/resource_class_cache.py @@ -15,6 +15,7 @@ import sqlalchemy as sa from nova.db.sqlalchemy import api as db_api from nova.db.sqlalchemy import api_models as models +from nova import exception from nova.objects import fields _RC_TBL = models.ResourceClass.__table__ @@ -78,6 +79,8 @@ class ResourceClassCache(object): :returns integer identifier for the resource class, or None, if no such resource class was found in the list of standard resource classes or the resource_classes database table. + :raises `exception.ResourceClassNotFound` if rc_str cannot be found in + either the standard classes or the DB. """ if rc_str in self.id_cache: return self.id_cache[rc_str] @@ -88,7 +91,9 @@ class ResourceClassCache(object): else: # Otherwise, check the database table _refresh_from_db(self.ctx, self) - return self.id_cache.get(rc_str) + if rc_str in self.id_cache: + return self.id_cache[rc_str] + raise exception.ResourceClassNotFound(resource_class=rc_str) def string_from_id(self, rc_id): """The reverse of the id_from_string() method. Given a supplied numeric @@ -102,6 +107,8 @@ class ResourceClassCache(object): :returns string identifier for the resource class, or None, if no such resource class was found in the list of standard resource classes or the resource_classes database table. + :raises `exception.ResourceClassNotFound` if rc_id cannot be found in + either the standard classes or the DB. """ if rc_id in self.str_cache: return self.str_cache[rc_id] @@ -112,4 +119,6 @@ class ResourceClassCache(object): except IndexError: # Otherwise, check the database table _refresh_from_db(self.ctx, self) - return self.str_cache.get(rc_id) + if rc_id in self.str_cache: + return self.str_cache[rc_id] + raise exception.ResourceClassNotFound(resource_class=rc_id) diff --git a/nova/exception.py b/nova/exception.py index c9e52120557d..e56d2ad6ee41 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -2114,10 +2114,18 @@ class ConcurrentUpdateDetected(NovaException): "Please retry your update") +class ResourceClassNotFound(NotFound): + msg_fmt = _("No such resource class %(resource_class)s.") + + class ResourceProviderInUse(NovaException): msg_fmt = _("Resource provider has allocations.") +class InventoryWithResourceClassNotFound(NotFound): + msg_fmt = _("No inventory of class %(resource_class)s found.") + + class InvalidInventory(Invalid): msg_fmt = _("Inventory for '%(resource_class)s' on " "resource provider '%(resource_provider)s' invalid.") diff --git a/nova/objects/resource_provider.py b/nova/objects/resource_provider.py index 7d0f460907f7..778f0910c7b8 100644 --- a/nova/objects/resource_provider.py +++ b/nova/objects/resource_provider.py @@ -159,8 +159,8 @@ def _update_inventory_for_provider(conn, rp, inv_list, to_update): allocation_ratio=inv_record.allocation_ratio) res = conn.execute(upd_stmt) if not res.rowcount: - raise exception.NotFound( - 'No inventory of class %s found for update' % rc_str) + raise exception.InventoryWithResourceClassNotFound( + resource_class=rc_str) return exceeded @@ -191,12 +191,13 @@ def _increment_provider_generation(conn, rp): @db_api.api_context_manager.writer def _add_inventory(context, rp, inventory): - """Add one Inventory that wasn't already on the provider.""" + """Add one Inventory that wasn't already on the provider. + + :raises `exception.ResourceClassNotFound` if inventory.resource_class + cannot be found in either the standard classes or the DB. + """ _ensure_rc_cache(context) rc_id = _RC_CACHE.id_from_string(inventory.resource_class) - if rc_id is None: - raise exception.NotFound("No such resource class '%s'" % - inventory.resource_class) inv_list = InventoryList(objects=[inventory]) conn = context.session.connection() with conn.begin(): @@ -207,12 +208,13 @@ def _add_inventory(context, rp, inventory): @db_api.api_context_manager.writer def _update_inventory(context, rp, inventory): - """Update an inventory already on the provider.""" + """Update an inventory already on the provider. + + :raises `exception.ResourceClassNotFound` if inventory.resource_class + cannot be found in either the standard classes or the DB. + """ _ensure_rc_cache(context) rc_id = _RC_CACHE.id_from_string(inventory.resource_class) - if rc_id is None: - raise exception.NotFound("No such resource class '%s'" % - inventory.resource_class) inv_list = InventoryList(objects=[inventory]) conn = context.session.connection() with conn.begin(): @@ -224,7 +226,11 @@ def _update_inventory(context, rp, inventory): @db_api.api_context_manager.writer def _delete_inventory(context, rp, resource_class): - """Delete up to one Inventory of the given resource_class string.""" + """Delete up to one Inventory of the given resource_class string. + + :raises `exception.ResourceClassNotFound` if resource_class + cannot be found in either the standard classes or the DB. + """ _ensure_rc_cache(context) conn = context.session.connection() rc_id = _RC_CACHE.id_from_string(resource_class) @@ -251,6 +257,9 @@ def _set_inventory(context, rp, inv_list): the same resource provider's view of its inventory or allocations in between the time when this object was originally read and the call to set the inventory. + :raises `exception.ResourceClassNotFound` if any resource class in any + inventory in inv_list cannot be found in either the standard + classes or the DB. """ _ensure_rc_cache(context) conn = context.session.connection() @@ -853,16 +862,19 @@ class AllocationList(base.ObjectListBase, base.NovaObject): We must check that there is capacity for each allocation. If there is not we roll back the entire set. + + :raises `exception.ResourceClassNotFound` if any resource class in any + allocation in allocs cannot be found in either the standard + classes or the DB. """ _ensure_rc_cache(context) conn = context.session.connection() # Short-circuit out if there are any allocations with string - # resource class names that don't exist. + # resource class names that don't exist this will raise a + # ResourceClassNotFound exception. for alloc in allocs: - if _RC_CACHE.id_from_string(alloc.resource_class) is None: - raise exception.NotFound("No such resource class '%s'" % - alloc.resource_class) + _RC_CACHE.id_from_string(alloc.resource_class) # Before writing any allocation records, we check that the submitted # allocations do not cause any inventory capacity to be exceeded for diff --git a/nova/tests/functional/api/openstack/placement/gabbits/allocations.yaml b/nova/tests/functional/api/openstack/placement/gabbits/allocations.yaml index 13e392d75e6c..cf769a683e13 100644 --- a/nova/tests/functional/api/openstack/placement/gabbits/allocations.yaml +++ b/nova/tests/functional/api/openstack/placement/gabbits/allocations.yaml @@ -144,7 +144,7 @@ tests: COWS: 12 status: 400 response_strings: - - No such resource class 'COWS' + - No such resource class COWS - name: delete allocation DELETE: /allocations/599ffd2d-526a-4b2e-8683-f13ad25f9958 diff --git a/nova/tests/functional/api/openstack/placement/gabbits/inventory.yaml b/nova/tests/functional/api/openstack/placement/gabbits/inventory.yaml index 358474dc3ed4..237a22a8aab6 100644 --- a/nova/tests/functional/api/openstack/placement/gabbits/inventory.yaml +++ b/nova/tests/functional/api/openstack/placement/gabbits/inventory.yaml @@ -107,6 +107,17 @@ tests: response_strings: - resource provider generation conflict +- name: modify inventory no such resource class in inventory + PUT: /resource_providers/$ENVIRON['RP_UUID']/inventories/MEMORY_MB + request_headers: + content-type: application/json + data: + resource_provider_generation: 2 + total: 2048 + status: 400 + response_strings: + - No inventory record with resource class + - name: modify inventory invalid data desc: This should 400 because reserved is greater than total PUT: $LAST_URL @@ -163,7 +174,7 @@ tests: total: 2048 status: 400 response_strings: - - No such resource class 'NO_CLASS_14' + - No such resource class NO_CLASS_14 - name: post inventory duplicated resource class desc: DISK_GB was already created above @@ -302,6 +313,19 @@ tests: response_strings: - resource provider generation conflict +- name: put all inventory unknown resource class + PUT: /resource_providers/$ENVIRON['RP_UUID']/inventories + request_headers: + content-type: application/json + data: + resource_provider_generation: 6 + inventories: + HOUSE: + total: 253 + status: 400 + response_strings: + - Unknown resource class in inventory + # NOTE(cdent): The generation is 6 now, based on the activity at # the start of this file. - name: put all inventory bad capacity diff --git a/nova/tests/functional/db/test_resource_class_cache.py b/nova/tests/functional/db/test_resource_class_cache.py index aae9984d17c3..5c9858a266cf 100644 --- a/nova/tests/functional/db/test_resource_class_cache.py +++ b/nova/tests/functional/db/test_resource_class_cache.py @@ -13,6 +13,7 @@ import mock from nova.db.sqlalchemy import resource_class_cache as rc_cache +from nova import exception from nova import test from nova.tests import fixtures @@ -49,9 +50,12 @@ class TestResourceClassCache(test.TestCase): """ cache = rc_cache.ResourceClassCache(self.context) - # Haven't added anything to the DB yet, so should return None - self.assertIsNone(cache.string_from_id(1001)) - self.assertIsNone(cache.id_from_string("IRON_NFV")) + # Haven't added anything to the DB yet, so should raise + # ResourceClassNotFound + self.assertRaises(exception.ResourceClassNotFound, + cache.string_from_id, 1001) + self.assertRaises(exception.ResourceClassNotFound, + cache.id_from_string, "IRON_NFV") # Now add to the database and verify appropriate results... with self.context.session.connection() as conn: @@ -69,3 +73,13 @@ class TestResourceClassCache(test.TestCase): self.assertEqual('IRON_NFV', cache.string_from_id(1001)) self.assertEqual(1001, cache.id_from_string('IRON_NFV')) self.assertFalse(sel_mock.called) + + def test_rc_cache_miss(self): + """Test that we raise ResourceClassNotFound if an unknown resource + class ID or string is searched for. + """ + cache = rc_cache.ResourceClassCache(self.context) + self.assertRaises(exception.ResourceClassNotFound, + cache.string_from_id, 99999999) + self.assertRaises(exception.ResourceClassNotFound, + cache.id_from_string, 'UNKNOWN') diff --git a/nova/tests/functional/db/test_resource_provider.py b/nova/tests/functional/db/test_resource_provider.py index 5215df3d6782..614d6f8223b0 100644 --- a/nova/tests/functional/db/test_resource_provider.py +++ b/nova/tests/functional/db/test_resource_provider.py @@ -200,6 +200,32 @@ class ResourceProviderTestCase(ResourceProviderBaseCase): self.context, resource_provider.uuid)) self.assertEqual(33, reloaded_inventories[0].total) + def test_set_inventory_unknown_resource_class(self): + """Test attempting to set inventory to an unknown resource class raises + an exception. + """ + rp = objects.ResourceProvider( + context=self.context, + uuid=uuidsentinel.rp_uuid, + name='compute-host', + ) + rp.create() + + inv = objects.Inventory( + resource_provider=rp, + resource_class='UNKNOWN', + total=1024, + reserved=15, + min_unit=10, + max_unit=100, + step_size=10, + allocation_ratio=1.0, + ) + + inv_list = objects.InventoryList(objects=[inv]) + self.assertRaises(exception.ResourceClassNotFound, + rp.set_inventory, inv_list) + @mock.patch('nova.objects.resource_provider.LOG') def test_set_inventory_over_capacity(self, mock_log): rp = objects.ResourceProvider(context=self.context, @@ -419,7 +445,7 @@ class ResourceProviderTestCase(ResourceProviderBaseCase): disk_inv.obj_set_defaults() error = self.assertRaises(exception.NotFound, rp.update_inventory, disk_inv) - self.assertIn('No inventory of class DISK_GB found for update', + self.assertIn('No inventory of class DISK_GB found', str(error)) @mock.patch('nova.objects.resource_provider.LOG')