From 86759329a6a479e8acfd2152b0627a42cd8b0db8 Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Sun, 25 Mar 2012 21:59:29 +0100 Subject: [PATCH] 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/tests/test_compute.py | 4 ++-- nova/tests/test_db_api.py | 15 ++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) 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):