From 824569a480f5c57ac1185319cbe65a60c766ce13 Mon Sep 17 00:00:00 2001 From: Anton Arefiev Date: Fri, 13 May 2016 13:51:57 +0300 Subject: [PATCH] Fix update inventory for multiple providers DB call method get_all_by_resource_provider_uuid queries all Inventories and filters them by provider's uuid from ResourceProvider, but query list is not valid (contains inventories for all providers), the correct way to load ResourceProvider is to use query join. Replace joinedload with contains_eager to avoid redundant joins. As result last updated provider overwrites all provider's inventories with wrong data. Closes-Bug: #1572555 Change-Id: I49fb1bf63400280635c202dea8f870d727a91e81 --- nova/db/sqlalchemy/api_models.py | 4 +- nova/objects/resource_provider.py | 9 ++- .../unit/objects/test_resource_provider.py | 77 +++++++++++++++++-- 3 files changed, 76 insertions(+), 14 deletions(-) diff --git a/nova/db/sqlalchemy/api_models.py b/nova/db/sqlalchemy/api_models.py index e8bee347d799..953499394b43 100644 --- a/nova/db/sqlalchemy/api_models.py +++ b/nova/db/sqlalchemy/api_models.py @@ -308,8 +308,8 @@ class Inventory(API_BASE): allocation_ratio = Column(Float, nullable=False) resource_provider = orm.relationship( "ResourceProvider", - primaryjoin=('and_(Inventory.resource_provider_id == ' - 'ResourceProvider.id)'), + primaryjoin=('Inventory.resource_provider_id == ' + 'ResourceProvider.id'), foreign_keys=resource_provider_id) diff --git a/nova/objects/resource_provider.py b/nova/objects/resource_provider.py index 04e32a32a434..98f934b8b0f4 100644 --- a/nova/objects/resource_provider.py +++ b/nova/objects/resource_provider.py @@ -11,7 +11,7 @@ # under the License. import six -from sqlalchemy.orm import joinedload +from sqlalchemy.orm import contains_eager from nova.db.sqlalchemy import api as db_api from nova.db.sqlalchemy import api_models as models @@ -223,9 +223,10 @@ class InventoryList(base.ObjectListBase, base.NovaObject): @staticmethod @db_api.api_context_manager.reader def _get_all_by_resource_provider(context, rp_uuid): - return context.session.query(models.Inventory).\ - options(joinedload('resource_provider')).\ - filter(models.ResourceProvider.uuid == rp_uuid).all() + return context.session.query(db_api.models.Inventory).\ + join(db_api.models.Inventory.resource_provider).\ + options(contains_eager('resource_provider')).\ + filter(db_api.models.ResourceProvider.uuid == rp_uuid).all() @base.remotable_classmethod def get_all_by_resource_provider_uuid(cls, context, rp_uuid): diff --git a/nova/tests/unit/objects/test_resource_provider.py b/nova/tests/unit/objects/test_resource_provider.py index bdf163d95264..04c1c870fbb6 100644 --- a/nova/tests/unit/objects/test_resource_provider.py +++ b/nova/tests/unit/objects/test_resource_provider.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import uuid + import mock from nova import exception @@ -177,18 +179,23 @@ class TestRemoteInventoryNoDB(test_objects._RemoteTest, class TestInventory(test_objects._LocalTest): - def _make_inventory(self): + def _make_inventory(self, rp_uuid=None): + uuid = rp_uuid or uuids.inventory_resource_provider + name = uuid db_rp = objects.ResourceProvider( - context=self.context, uuid=uuids.inventory_resource_provider, - name=_RESOURCE_PROVIDER_NAME) + context=self.context, uuid=uuid, name=name) db_rp.create() - updates = dict(_INVENTORY_DB, - resource_provider_id=db_rp.id) - updates.pop('id') - db_inventory = objects.Inventory._create_in_db( - self.context, updates) + db_inventory = self._create_inventory_in_db(db_rp.id) return db_rp, db_inventory + def _create_inventory_in_db(self, rp_id, **records): + updates = dict(_INVENTORY_DB, + resource_provider_id=rp_id) + updates.pop('id') + updates.update(records) + return objects.Inventory._create_in_db( + self.context, updates) + def test_create_in_db(self): updates = dict(_INVENTORY_DB) updates.pop('id') @@ -229,6 +236,60 @@ class TestInventory(test_objects._LocalTest): ) self.assertEqual(0, len(retrieved_inventories)) + def test_get_all_by_resource_provider_multiple_providers(self): + # TODO(cdent): This and other nearby tests are functional + # and should be moved. + # Create 2 resource providers with DISK_GB resources. And + # update total value for second one. + db_rp1, db_inv1 = self._make_inventory(str(uuid.uuid4())) + db_rp2, db_inv2 = self._make_inventory(str(uuid.uuid4())) + + objects.Inventory._update_in_db(self.context, + db_inv2.id, + {'total': 32}) + + # Create IPV4_ADDRESS resources for each provider. + IPV4_ADDRESS_ID = objects.fields.ResourceClass.index( + objects.fields.ResourceClass.IPV4_ADDRESS) + + self._create_inventory_in_db(db_rp1.id, + resource_provider_id=db_rp1.id, + resource_class_id=IPV4_ADDRESS_ID, + total=2) + self._create_inventory_in_db(db_rp2.id, + resource_provider_id=db_rp2.id, + resource_class_id=IPV4_ADDRESS_ID, + total=4) + + expected_inv1 = { + _RESOURCE_CLASS_ID: _INVENTORY_DB['total'], + IPV4_ADDRESS_ID: 2} + expected_inv2 = { + _RESOURCE_CLASS_ID: 32, + IPV4_ADDRESS_ID: 4} + + # Get inventories for each resource provider and validate + # that the inventory records for that resource provider uuid + # and match expected total value. + retrieved_inv = ( + objects.InventoryList._get_all_by_resource_provider( + self.context, db_rp1.uuid) + ) + for inv in retrieved_inv: + self.assertEqual(db_rp1.id, inv.resource_provider_id) + self.assertEqual(expected_inv1[inv.resource_class_id], + inv.total) + + retrieved_inv = ( + objects.InventoryList._get_all_by_resource_provider( + self.context, db_rp2.uuid) + ) + + for inv in retrieved_inv: + self.assertEqual(db_rp2.id, inv.resource_provider_id) + self.assertEqual(expected_inv2[inv.resource_class_id], + inv.total) + def test_create_requires_resource_provider(self): inventory_dict = dict(_INVENTORY_DB) inventory_dict.pop('id')