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

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):