From ef1f1738f23909feb5c5b2a617b1cb88986989ee Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Sun, 25 Mar 2012 21:59:29 +0100 Subject: Clean up read_deleted support in host aggregates code tl;dr - this is a cleaner fix for bug #949038 It seems clear to me that all of the DB APIs should not explicitly pass read_deleted='no' or read_deleted='yes' to model_query without good reason. We want to allow callers to specify read_deleted via the context and that only works if we don't explicitly pass it to model_query(). If we don't explicitly specify it to model_query(), we use the value from the context which defaults to 'no'. Given all that, there is no need to support read_deleted to any of the DB API calls because they should support specifying the flag via the context. There should also be no need to pass read_deleted='no' because that is the default. Really, the only place there should be any mention of read_deleted is where we want read_deleted='yes' behaviour e.g. - In tests, where we want to check the operational_state of an aggregate after it has been deleted - Where we want to support undeleting an aggregate or aggreate host Change-Id: I916a8d189a33d7f30838cccb26531a024066ef96 --- nova/compute/api.py | 9 +++---- nova/db/api.py | 20 ++++++++-------- nova/db/sqlalchemy/api.py | 59 ++++++++++++++++++++-------------------------- nova/tests/test_compute.py | 4 ++-- nova/tests/test_db_api.py | 15 +++++------- 5 files changed, 46 insertions(+), 61 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 79e490395..e5513223d 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1730,7 +1730,7 @@ class AggregateAPI(base.Base): def get_aggregate_list(self, context): """Get all the aggregates for this zone.""" - aggregates = self.db.aggregate_get_all(context, read_deleted="no") + aggregates = self.db.aggregate_get_all(context) return [self._get_aggregate_info(context, a) for a in aggregates] def update_aggregate(self, context, aggregate_id, values): @@ -1761,8 +1761,7 @@ class AggregateAPI(base.Base): def delete_aggregate(self, context, aggregate_id): """Deletes the aggregate.""" - hosts = self.db.aggregate_host_get_all(context, aggregate_id, - read_deleted="no") + hosts = self.db.aggregate_host_get_all(context, aggregate_id) if len(hosts) > 0: raise exception.InvalidAggregateAction(action='delete', aggregate_id=aggregate_id, @@ -1827,9 +1826,7 @@ class AggregateAPI(base.Base): def _get_aggregate_info(self, context, aggregate): """Builds a dictionary with aggregate props, metadata and hosts.""" metadata = self.db.aggregate_metadata_get(context, aggregate.id) - hosts = self.db.aggregate_host_get_all(context, aggregate.id, - read_deleted="no") - + hosts = self.db.aggregate_host_get_all(context, aggregate.id) result = dict(aggregate.iteritems()) result["metadata"] = metadata result["hosts"] = hosts diff --git a/nova/db/api.py b/nova/db/api.py index 850c772c7..de2939fc2 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -1702,14 +1702,14 @@ def aggregate_create(context, values, metadata=None): return IMPL.aggregate_create(context, values, metadata) -def aggregate_get(context, aggregate_id, **kwargs): +def aggregate_get(context, aggregate_id): """Get a specific aggregate by id.""" - return IMPL.aggregate_get(context, aggregate_id, **kwargs) + return IMPL.aggregate_get(context, aggregate_id) -def aggregate_get_by_host(context, host, **kwargs): +def aggregate_get_by_host(context, host): """Get a specific aggregate by host""" - return IMPL.aggregate_get_by_host(context, host, **kwargs) + return IMPL.aggregate_get_by_host(context, host) def aggregate_update(context, aggregate_id, values): @@ -1723,9 +1723,9 @@ def aggregate_delete(context, aggregate_id): return IMPL.aggregate_delete(context, aggregate_id) -def aggregate_get_all(context, **kwargs): +def aggregate_get_all(context): """Get all aggregates.""" - return IMPL.aggregate_get_all(context, **kwargs) + return IMPL.aggregate_get_all(context) def aggregate_metadata_add(context, aggregate_id, metadata, set_delete=False): @@ -1733,9 +1733,9 @@ def aggregate_metadata_add(context, aggregate_id, metadata, set_delete=False): IMPL.aggregate_metadata_add(context, aggregate_id, metadata, set_delete) -def aggregate_metadata_get(context, aggregate_id, **kwargs): +def aggregate_metadata_get(context, aggregate_id): """Get metadata for the specified aggregate.""" - return IMPL.aggregate_metadata_get(context, aggregate_id, **kwargs) + return IMPL.aggregate_metadata_get(context, aggregate_id) def aggregate_metadata_delete(context, aggregate_id, key): @@ -1748,9 +1748,9 @@ def aggregate_host_add(context, aggregate_id, host): IMPL.aggregate_host_add(context, aggregate_id, host) -def aggregate_host_get_all(context, aggregate_id, **kwargs): +def aggregate_host_get_all(context, aggregate_id): """Get hosts for the specified aggregate.""" - return IMPL.aggregate_host_get_all(context, aggregate_id, **kwargs) + return IMPL.aggregate_host_get_all(context, aggregate_id) def aggregate_host_delete(context, aggregate_id, host): diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index ebb8b60e0..1c9381ece 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -4145,8 +4145,10 @@ def sm_volume_get_all(context): ################ -def _aggregate_get_query(context, model_class, id_field, id, **kwargs): - return model_query(context, model_class, **kwargs).filter(id_field == id) +def _aggregate_get_query(context, model_class, id_field, id, + session=None, read_deleted=None): + return model_query(context, model_class, session=session, + read_deleted=read_deleted).filter(id_field == id) @require_admin_context @@ -4176,12 +4178,11 @@ def aggregate_create(context, values, metadata=None): @require_admin_context -def aggregate_get(context, aggregate_id, **kwargs): +def aggregate_get(context, aggregate_id): aggregate = _aggregate_get_query(context, models.Aggregate, models.Aggregate.id, - aggregate_id, - **kwargs).first() + aggregate_id).first() if not aggregate: raise exception.AggregateNotFound(aggregate_id=aggregate_id) @@ -4190,18 +4191,16 @@ def aggregate_get(context, aggregate_id, **kwargs): @require_admin_context -def aggregate_get_by_host(context, host, **kwargs): +def aggregate_get_by_host(context, host): aggregate_host = _aggregate_get_query(context, models.AggregateHost, models.AggregateHost.host, - host, - **kwargs).first() + host).first() if not aggregate_host: raise exception.AggregateHostNotFound(host=host) - return aggregate_get(context, aggregate_host.aggregate_id, - **kwargs) + return aggregate_get(context, aggregate_host.aggregate_id) @require_admin_context @@ -4209,9 +4208,9 @@ def aggregate_update(context, aggregate_id, values): session = get_session() aggregate = _aggregate_get_query(context, models.Aggregate, - models.Aggregate.id, aggregate_id, - session=session, - read_deleted='no').first() + models.Aggregate.id, + aggregate_id, + session=session).first() if aggregate: metadata = values.get('metadata') if metadata is not None: @@ -4232,8 +4231,8 @@ def aggregate_update(context, aggregate_id, values): def aggregate_delete(context, aggregate_id): query = _aggregate_get_query(context, models.Aggregate, - models.Aggregate.id, aggregate_id, - read_deleted='no') + models.Aggregate.id, + aggregate_id) if query.first(): query.update({'deleted': True, 'deleted_at': utils.utcnow(), @@ -4244,20 +4243,16 @@ def aggregate_delete(context, aggregate_id): @require_admin_context -def aggregate_get_all(context, **kwargs): - if 'read_deleted' not in kwargs: - kwargs['read_deleted'] = 'yes' - return model_query(context, - models.Aggregate, - **kwargs).all() +def aggregate_get_all(context): + return model_query(context, models.Aggregate).all() @require_admin_context @require_aggregate_exists -def aggregate_metadata_get(context, aggregate_id, **kwargs): +def aggregate_metadata_get(context, aggregate_id): rows = model_query(context, - models.AggregateMetadata, - **kwargs).filter_by(aggregate_id=aggregate_id).all() + models.AggregateMetadata).\ + filter_by(aggregate_id=aggregate_id).all() return dict([(r['key'], r['value']) for r in rows]) @@ -4268,7 +4263,7 @@ def aggregate_metadata_delete(context, aggregate_id, key): query = _aggregate_get_query(context, models.AggregateMetadata, models.AggregateMetadata.aggregate_id, - aggregate_id, read_deleted='no').\ + aggregate_id).\ filter_by(key=key) if query.first(): query.update({'deleted': True, @@ -4281,8 +4276,7 @@ def aggregate_metadata_delete(context, aggregate_id, key): @require_admin_context @require_aggregate_exists -def aggregate_metadata_get_item(context, aggregate_id, key, - session=None): +def aggregate_metadata_get_item(context, aggregate_id, key, session=None): result = _aggregate_get_query(context, models.AggregateMetadata, models.AggregateMetadata.aggregate_id, @@ -4332,12 +4326,10 @@ def aggregate_metadata_add(context, aggregate_id, metadata, set_delete=False): @require_admin_context @require_aggregate_exists -def aggregate_host_get_all(context, aggregate_id, **kwargs): - if 'read_deleted' not in kwargs: - kwargs['read_deleted'] = 'yes' +def aggregate_host_get_all(context, aggregate_id): rows = model_query(context, - models.AggregateHost, - **kwargs).filter_by(aggregate_id=aggregate_id).all() + models.AggregateHost).\ + filter_by(aggregate_id=aggregate_id).all() return [r.host for r in rows] @@ -4348,8 +4340,7 @@ def aggregate_host_delete(context, aggregate_id, host): query = _aggregate_get_query(context, models.AggregateHost, models.AggregateHost.aggregate_id, - aggregate_id, - read_deleted='no').filter_by(host=host) + aggregate_id).filter_by(host=host) if query.first(): query.update({'deleted': True, 'deleted_at': utils.utcnow(), diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py index ecb980106..58b40666e 100644 --- a/nova/tests/test_compute.py +++ b/nova/tests/test_compute.py @@ -3287,8 +3287,8 @@ class ComputeAPIAggrTestCase(test.TestCase): aggr = self.api.create_aggregate(self.context, 'fake_aggregate', 'fake_zone') self.api.delete_aggregate(self.context, aggr['id']) - expected = db.aggregate_get(self.context, aggr['id'], - read_deleted='yes') + expected = db.aggregate_get(self.context.elevated(read_deleted='yes'), + aggr['id']) self.assertNotEqual(aggr['operational_state'], expected['operational_state']) diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py index 158282742..bee9a496e 100644 --- a/nova/tests/test_db_api.py +++ b/nova/tests/test_db_api.py @@ -434,11 +434,10 @@ class AggregateDBApiTestCase(test.TestCase): ctxt = context.get_admin_context() result = _create_aggregate(context=ctxt, metadata=None) db.aggregate_delete(ctxt, result['id']) - expected = db.aggregate_get_all(ctxt, read_deleted='no') + expected = db.aggregate_get_all(ctxt) self.assertEqual(0, len(expected)) - - ctxt = context.get_admin_context(read_deleted='yes') - aggregate = db.aggregate_get(ctxt, result['id']) + aggregate = db.aggregate_get(ctxt.elevated(read_deleted='yes'), + result['id']) self.assertEqual(aggregate["operational_state"], "dismissed") def test_aggregate_update(self): @@ -506,7 +505,7 @@ class AggregateDBApiTestCase(test.TestCase): values=values, metadata=None)) for c in xrange(1, remove_counter): db.aggregate_delete(ctxt, aggregates[c - 1].id) - results = db.aggregate_get_all(ctxt, read_deleted='no') + results = db.aggregate_get_all(ctxt) self.assertEqual(len(results), add_counter - remove_counter) def test_aggregate_metadata_add(self): @@ -564,8 +563,7 @@ class AggregateDBApiTestCase(test.TestCase): host = _get_fake_aggr_hosts()[0] db.aggregate_host_delete(ctxt, result.id, host) db.aggregate_host_add(ctxt, result.id, host) - expected = db.aggregate_host_get_all(ctxt, result.id, - read_deleted='no') + expected = db.aggregate_host_get_all(ctxt, result.id) self.assertEqual(len(expected), 1) def test_aggregate_host_add_duplicate_raise_conflict(self): @@ -602,8 +600,7 @@ class AggregateDBApiTestCase(test.TestCase): result = _create_aggregate_with_hosts(context=ctxt, metadata=None) db.aggregate_host_delete(ctxt, result.id, _get_fake_aggr_hosts()[0]) - expected = db.aggregate_host_get_all(ctxt, result.id, - read_deleted='no') + expected = db.aggregate_host_get_all(ctxt, result.id) self.assertEqual(0, len(expected)) def test_aggregate_host_delete_raise_not_found(self): -- cgit